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

[Segher Boessenkool]

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.

> 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