Patch to fix PR61360

[Richard Sandiford]

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