* Patch to fix PR61360
@ 2014-09-18 16:04 Vladimir Makarov
2014-09-18 16:10 ` Jakub Jelinek
0 siblings, 1 reply; 6+ messages in thread
From: Vladimir Makarov @ 2014-09-18 16:04 UTC (permalink / raw)
To: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 435 bytes --]
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.
[-- Attachment #2: pr61360.patch --]
[-- Type: text/x-patch, Size: 1063 bytes --]
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;
+ }
+}
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Patch to fix PR61360
2014-09-18 16:04 Patch to fix PR61360 Vladimir Makarov
@ 2014-09-18 16:10 ` Jakub Jelinek
2014-09-18 16:53 ` Vladimir Makarov
2014-09-18 17:36 ` Richard Sandiford
0 siblings, 2 replies; 6+ messages in thread
From: Jakub Jelinek @ 2014-09-18 16:10 UTC (permalink / raw)
To: Vladimir Makarov; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Patch to fix PR61360
2014-09-18 16:10 ` Jakub Jelinek
@ 2014-09-18 16:53 ` Vladimir Makarov
2014-09-18 17:36 ` Richard Sandiford
1 sibling, 0 replies; 6+ messages in thread
From: Vladimir Makarov @ 2014-09-18 16:53 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Patch to fix PR61360
2014-09-18 16:10 ` Jakub Jelinek
2014-09-18 16:53 ` Vladimir Makarov
@ 2014-09-18 17:36 ` Richard Sandiford
2014-09-18 18:10 ` Vladimir Makarov
1 sibling, 1 reply; 6+ messages in thread
From: Richard Sandiford @ 2014-09-18 17:36 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Vladimir Makarov, gcc-patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Patch to fix PR61360
2014-09-18 17:36 ` Richard Sandiford
@ 2014-09-18 18:10 ` Vladimir Makarov
2014-09-18 21:50 ` Richard Sandiford
0 siblings, 1 reply; 6+ messages in thread
From: Vladimir Makarov @ 2014-09-18 18:10 UTC (permalink / raw)
To: Jakub Jelinek, gcc-patches, rdsandiford
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).
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Patch to fix PR61360
2014-09-18 18:10 ` Vladimir Makarov
@ 2014-09-18 21:50 ` Richard Sandiford
0 siblings, 0 replies; 6+ messages in thread
From: Richard Sandiford @ 2014-09-18 21:50 UTC (permalink / raw)
To: Vladimir Makarov; +Cc: Jakub Jelinek, gcc-patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-09-18 21:50 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-18 16:04 Patch to fix PR61360 Vladimir Makarov
2014-09-18 16:10 ` Jakub Jelinek
2014-09-18 16:53 ` Vladimir Makarov
2014-09-18 17:36 ` Richard Sandiford
2014-09-18 18:10 ` Vladimir Makarov
2014-09-18 21:50 ` Richard Sandiford
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).