| Message ID | 541B028E.9010308@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Sep 18, 2014 at 12:04:30PM -0400, Vladimir Makarov wrote: > The following patch fixes the PR. The details can be found on > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61360 > > The patch was bootstrapped and tested on x86/x86-64. > > Committed as rev. 215358. What effect does this have on compile time? > 2014-09-18 Vladimir Makarov <vmakarov@redhat.com> > > PR target/61360 > * lra.c (lra): Call recog_init. > > 2014-09-18 Vladimir Makarov <vmakarov@redhat.com> > > PR target/61360 > * gcc.target/i386/pr61360.c: New. Jakub
On 09/18/2014 12:10 PM, Jakub Jelinek wrote: > On Thu, Sep 18, 2014 at 12:04:30PM -0400, Vladimir Makarov wrote: >> The following patch fixes the PR. The details can be found on >> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61360 >> >> The patch was bootstrapped and tested on x86/x86-64. >> >> Committed as rev. 215358. > What effect does this have on compile time? > > It is hard to measure real time but < 0.05% according to valgrind lackey on combine.i for -O2
Jakub Jelinek <jakub@redhat.com> writes: > On Thu, Sep 18, 2014 at 12:04:30PM -0400, Vladimir Makarov wrote: >> The following patch fixes the PR. The details can be found on >> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61360 >> >> The patch was bootstrapped and tested on x86/x86-64. >> >> Committed as rev. 215358. > > What effect does this have on compile time? Regardless of compile time, I strongly object to this kind of hack. (a) it will in practice never go away. (b) (more importantly) it makes no conceptual sense. It means that passes before lra use the old, cached "enabled" attribute while "lra" and after will uew "fresh" values. The only reason the call has been put here is because lra was the only pass that checks for and asserted on inconsistent values. Passes before lra will still see the same inconsistent values but they happen not to assert. I.e. we've put the call here to shut up a valid assert rather than because it's the right place to do it. (c) the "enabled" attribute was never supposed to be used in this way. I really think the patch should be reverted. Thanks, Richard
On 09/18/2014 01:36 PM, Richard Sandiford wrote: > Jakub Jelinek <jakub@redhat.com> writes: >> On Thu, Sep 18, 2014 at 12:04:30PM -0400, Vladimir Makarov wrote: >>> The following patch fixes the PR. The details can be found on >>> >>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61360 >>> >>> The patch was bootstrapped and tested on x86/x86-64. >>> >>> Committed as rev. 215358. >> What effect does this have on compile time? > Regardless of compile time, I strongly object to this kind of hack. > > (a) it will in practice never go away. > > (b) (more importantly) it makes no conceptual sense. It means that > passes before lra use the old, cached "enabled" attribute while > "lra" and after will uew "fresh" values. > > The only reason the call has been put here is because lra was the > only pass that checks for and asserted on inconsistent values. > Passes before lra will still see the same inconsistent values but > they happen not to assert. > > I.e. we've put the call here to shut up a valid assert rather than > because it's the right place to do it. > > (c) the "enabled" attribute was never supposed to be used in this way. > > I really think the patch should be reverted. > > Richard, I waited > 4 months that somebody fixes this in md file (and people tried to do this without success). Instead I was asked numerous times from people interesting in fixing these crashes to fix it in LRA. After a recent request, I gave up. So I could revert it transferring blame on you but I don't think this hack is so bad to do this (may be I am wrong).
Vladimir Makarov <vmakarov@redhat.com> writes: > On 09/18/2014 01:36 PM, Richard Sandiford wrote: >> Jakub Jelinek <jakub@redhat.com> writes: >>> On Thu, Sep 18, 2014 at 12:04:30PM -0400, Vladimir Makarov wrote: >>>> The following patch fixes the PR. The details can be found on >>>> >>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61360 >>>> >>>> The patch was bootstrapped and tested on x86/x86-64. >>>> >>>> Committed as rev. 215358. >>> What effect does this have on compile time? >> Regardless of compile time, I strongly object to this kind of hack. >> >> (a) it will in practice never go away. >> >> (b) (more importantly) it makes no conceptual sense. It means that >> passes before lra use the old, cached "enabled" attribute while >> "lra" and after will uew "fresh" values. >> >> The only reason the call has been put here is because lra was the >> only pass that checks for and asserted on inconsistent values. >> Passes before lra will still see the same inconsistent values but >> they happen not to assert. >> >> I.e. we've put the call here to shut up a valid assert rather than >> because it's the right place to do it. >> >> (c) the "enabled" attribute was never supposed to be used in this way. >> >> I really think the patch should be reverted. >> >> > Richard, I waited > 4 months that somebody fixes this in md file (and > people tried to do this without success). Instead I was asked numerous > times from people interesting in fixing these crashes to fix it in LRA. > After a recent request, I gave up. > > So I could revert it transferring blame on you but I don't think this > hack is so bad to do this (may be I am wrong). I suppose I'm not being constructive, but the .md pattern in question is: (set (attr "enabled") (cond [(eq_attr "alternative" "0") (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"))) ]) Even without the LRA patch to make it work again the complicated phase test and ??? comment show that the pattern is already a hack. So how about just dropping the optimistion until it can be implemented cleanly? AFAICT by fixing the LRA assert we're regressing the sched1 part of the test. Normally (i.e. when a target attribute isn't used) recog_init is only called during start-up. So I think after your patch it will be called by: start-up code LRA for first function LRA for second function ... And LRA calls recog_init with lra_in_progress set to 1: 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 (); So for all but the first function, the lra_in_progress part of the test is going evaluate to true for all passes, even pre-RA ones. I.e. the !(... || lra_in_progress)/"might this be sched1?" part of the test is only ever going to affect the first compiled function. If the decision is to stick with the patch then I think we need at least one more recog_init call at the start of function compilation, with lra_in_progress and the reload variables set back to 0. But again that doesn't really make much conceptual sense to me -- it seems like both calls would be there purely for the sake of this one pattern. Thanks, Richard
Index: lra.c =================================================================== --- lra.c (revision 215337) +++ lra.c (working copy) @@ -2135,6 +2135,11 @@ 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; Index: testsuite/gcc.target/i386/pr61360.c =================================================================== --- testsuite/gcc.target/i386/pr61360.c (revision 0) +++ testsuite/gcc.target/i386/pr61360.c (working copy) @@ -0,0 +1,12 @@ +/* { dg-do compile } */ +/* { dg-options "-march=amdfam10 -O2" } */ +int a, b, c, e, f, g, h; +long *d; +__attribute__((cold)) void fn1() { + int i = g | 1; + for (; g; h++) { + for (; a; e++) d[0] = c; + if (0.002 * i) break; + for (; b; f++) d[h] = 0; + } +}
The following patch fixes the PR. The details can be found on https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61360 The patch was bootstrapped and tested on x86/x86-64. Committed as rev. 215358. 2014-09-18 Vladimir Makarov <vmakarov@redhat.com> PR target/61360 * lra.c (lra): Call recog_init. 2014-09-18 Vladimir Makarov <vmakarov@redhat.com> PR target/61360 * gcc.target/i386/pr61360.c: New.