Vladimir Makarov <vmaka...@redhat.com> writes:
> On 09/18/2014 01:36 PM, Richard Sandiford wrote:
>> Jakub Jelinek <ja...@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
