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

[Kyrill Tkachov <kyrylo dot tkachov at arm dot 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