Re: [PATCH][combine] PR rtl-optimization/68381: Only restrict pure simplification in mult-extend subst case, allow other substitutions

[Kyrill Tkachov <kyrylo.tkachov@arm.com>]

On 19/11/15 14:41, Segher Boessenkool wrote:
> On Thu, Nov 19, 2015 at 01:38:53PM +0000, Kyrill Tkachov wrote:
>>> That is troublesome.  Could you look deeper?
>> Yes.
> Thanks.
>
>> So the bad case is when we're in subst and returning a CLOBBER of zero
>> and 'from' is (reg/v:SI 80 [ x ]) and 'to' is (zero_extend:SI (reg:HI 0 x0
>> [ x ])).
>> The call to subst comes from try_combine at line 3403:
>>
>>     if (added_sets_1)
>>      {
>>        rtx t = i1pat;
>>        if (i0_feeds_i1_n)
>>          t = subst (t, i0dest, i0src_copy ? i0src_copy : i0src, 0, 0, 0);
>>
>>        XVECEXP (newpat, 0, --total_sets) = t;
>>      }
>>
>> It uses t after calling subst on it without checking that it didn't return
>> a clobber.
>> If I change that snippet to check for CLOBBER:
>>    if (added_sets_1)
>>      {
>>        rtx t = i1pat;
>>        if (i0_feeds_i1_n)
>>          t = subst (t, i0dest, i0src_copy ? i0src_copy : i0src, 0, 0, 0);
>>
>>        if (GET_CODE (t) == CLOBBER)
>>          {
>>            undo_all ();
>>            return 0;
>>          }
>>        XVECEXP (newpat, 0, --total_sets) = t;
>>      }
>>
>> The testcase gets fixed.
>> But shouldn't the XVECEXP (newpat, 0, --total_sets) = t; line create an
>> uncrecognizable rtx
>> that would then get rejected by combine or something?
> Yes.  recog_for_combine_1 checks for a PARALLEL with such a CLOBBER
> right at the start; and of course having the clobber elsewhere will
> just not match.
>
>> If we don't check for clobber there and perform the "XVECEXP = ..."
>> the resulting newpat looks like:
>> (parallel [
>>          (set (reg:CC 66 cc)
>>              (compare:CC (const_int 0 [0])
>>                  (const_int 0 [0])))
>>          (nil)
>>          (clobber:DI (const_int 0 [0]))
>>      ])
>>
>> ah, so the clobber is put in a parallel with another insn and is thus
>> accepted by recog?
> No, recog_for_combine should refuse it because of that third arm.
> The second arm (the nil) looks very wrong, where is that coming
> from?  That isn't valid RTL.

Well, it came from a bit earlier before the subst call (around line 3390):
   /* If the actions of the earlier insns must be kept
      in addition to substituting them into the latest one,
      we must make a new PARALLEL for the latest insn
      to hold additional the SETs.  */
<snip>
       rtx old = newpat;
       total_sets = 1 + extra_sets;
       newpat = gen_rtx_PARALLEL (VOIDmode, rtvec_alloc (total_sets));
       XVECEXP (newpat, 0, 0) = old;


extra_sets is 2, so we have a parallel with 3 slots so after the subst call
we put the clobber into --total_sets, that is slot 2:
       XVECEXP (newpat, 0, --total_sets) = t;
A bit further below we fill slot 1 with another rtx, so all 3 parts of the PARALLEL
are filled.
I'll look further into why recog_for_combine doesn't kill the whole thing.

Kyrill



>
>> So, should we check 't' after subst for clobbers as above? Or should this
>> be fixed in
>> some other place?
> There is a bug somewhere, so that will need fixing.  Workarounds are
> last resort, and even then we really need to know what is going on.
>
>> Thanks. In light of the above I think this patch happens to avoid
>> the issue highlighted above but we should fix the above code separately?
> Yes, if your patch creates better code we want it (and fix the regression),
> but you exposed a separate bug as well :-)
>
>
> Segher
>