| Message ID | 5420CC52.8030707@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Sep 23, 2014 at 3:26 AM, Vladimir Makarov <vmakarov@redhat.com> wrote: > The previous patch to solve PR61360 fixed the problem in IRA (it was > easier for me to do as I know the code well) > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61360 > > Although imo it was an ok fix, Richard expressed concerns with the patch > and the practice to have different enable attribute values depending on the > current pass. > > I don't understand why "x,m" alternative is better to "x,r" and "x,r" > should be disabled. Even if the path from general regs to sse regs is slow > (usually such slow path is implemented internally by micro-architecture > through cache). "x,r" alternative results in only smaller insns (including > number of insns) with probably the same time for the movement. So "x,r" > should be at least no slower, insn cache should have more locality, and less > overhead for decoding/translating insns. > > Here I propose another solution avoiding to have different enable > attribute values. > > The patch was successfully bootstrapped on x86/x86-64 and tested with and > without -march=amdfam10 (actually the patch results in 2 less failures when > -march=amdfam10 were used). > > Uros, is i386.md change ok for the trunk? I don't think so. This would be a regression, since 4.8 (and later versions until Richard's patch) were able to handle this functionality just fine. Please also note that there are a couple of other patterns with the same problem, that is using ("nonimmediate_operand" "m") constraint. Please see PR 60704, comment 7 [1]. If LRA is able to fixup ("nonimmediate_operand" "m") to a memory from a register, then other RTL infrastructure should also be updated for this functionality. IMO, recog should be fixed/enhanced, so pseudo registers will also satisfy ("nonimmediate_operand" "m") constraint before LRA. [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60704#c7 Uros.
Index: config/i386/i386.md =================================================================== --- config/i386/i386.md (revision 215337) +++ config/i386/i386.md (working copy) @@ -4795,14 +4795,6 @@ (symbol_ref "TARGET_MIX_SSE_I387 && X87_ENABLE_FLOAT (<MODEF:MODE>mode, <SWI48:MODE>mode)") - (eq_attr "alternative" "1") - /* ??? For sched1 we need constrain_operands to be able to - select an alternative. Leave this enabled before RA. */ - (symbol_ref "TARGET_INTER_UNIT_CONVERSIONS - || optimize_function_for_size_p (cfun) - || !(reload_completed - || reload_in_progress - || lra_in_progress)") ] (symbol_ref "true"))) ]) Index: lra.c =================================================================== --- lra.c (revision 215358) +++ lra.c (working copy) @@ -2135,11 +2135,6 @@ lra (FILE *f) lra_in_progress = 1; - /* The enable attributes can change their values as LRA starts - although it is a bad practice. To prevent reuse of the outdated - values, clear them. */ - recog_init (); - lra_live_range_iter = lra_coalesce_iter = 0; lra_constraint_iter = lra_constraint_iter_after_spill = 0; lra_inheritance_iter = lra_undo_inheritance_iter = 0;
The previous patch to solve PR61360 fixed the problem in IRA (it was easier for me to do as I know the code well) https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61360 Although imo it was an ok fix, Richard expressed concerns with the patch and the practice to have different enable attribute values depending on the current pass. I don't understand why "x,m" alternative is better to "x,r" and "x,r" should be disabled. Even if the path from general regs to sse regs is slow (usually such slow path is implemented internally by micro-architecture through cache). "x,r" alternative results in only smaller insns (including number of insns) with probably the same time for the movement. So "x,r" should be at least no slower, insn cache should have more locality, and less overhead for decoding/translating insns. Here I propose another solution avoiding to have different enable attribute values. The patch was successfully bootstrapped on x86/x86-64 and tested with and without -march=amdfam10 (actually the patch results in 2 less failures when -march=amdfam10 were used). Uros, is i386.md change ok for the trunk? 2014-09-22 Vladimir Makarov <vmakarov@redhat.com> PR target/61360 * lra.c (lra): Remove call of recog_init. * config/i386/i386.md (*float<SWI48:mode><MODEF:mode>2_sse): Always enable first alternative.