Re: [PATCH PR62151]Fix uninitialized register issue caused by distribute_notes in combine pass

["Ulrich Weigand" <uweigand at de dot ibm dot com>]

Segher Boessenkool wreote:
> On Mon, Sep 01, 2014 at 10:39:10AM -0600, Jeff Law wrote:
> > On 09/01/14 05:38, Segher Boessenkool wrote:
> > >On Mon, Sep 01, 2014 at 11:36:07AM +0800, Bin.Cheng wrote:
> > >>>In the testcase (and comment in the proposed patch), why is combine
> > >>>combining four insns at all?  That means it rejected combining just the
> > >>>first three.  Why did it do that?
> > >>It is explicitly reject by below code in can_combine_p.
> > >>
> > >>   if (GET_CODE (PATTERN (i3)) == PARALLEL)
> > >>     for (i = XVECLEN (PATTERN (i3), 0) - 1; i >= 0; i--)
> > >>       if (GET_CODE (XVECEXP (PATTERN (i3), 0, i)) == CLOBBER)
> > >>     {
> > >>       /* Don't substitute for a register intended as a clobberable
> > >>          operand.  */
> > >>       rtx reg = XEXP (XVECEXP (PATTERN (i3), 0, i), 0);
> > >>       if (rtx_equal_p (reg, dest))
> > >>         return 0;
> > >>
> > >>Since insn i2 in the list of i0/i1/i2 as below contains parallel
> > >>clobber of dest_of_insn76/use_of_insn77.
> > >>    32: r84:SI=0
> > >>    76: flags:CC=cmp(r84:SI,0x1)
> > >>       REG_DEAD r84:SI
> > >>    77: {r84:SI=-ltu(flags:CC,0);clobber flags:CC;}
> > >>       REG_DEAD flags:CC
> > >>       REG_UNUSED flags:CC
> > >
> > >Archaeology suggests this check is because the clobber might be an
> > >earlyclobber.  Which seems silly: how can it be a valid insn at all
> > >in that case?  It seems to me the check can just be removed.  That
> > >will hide your issue, maybe even solve it (but I doubt it).
> > Silly for other reasons, namely that earlyclobber doesn't come into play 
> > until after combine (register allocation and later).
> 
> The last change to this code was by Ulrich (cc:ed); in that thread (June
> 2004, mostly not threaded in the mail archive, broken MUAs :-( ) it was
> said that any clobber should be considered an earlyclobber (an RTL insn
> can expand to multiple machine instructions, for example).  But I don't
> see how that can matter for "dest" here (the dest of "insn", that's 76
> in the example), only for "src".
> 
> The version of "flags" set in 76 obviously dies in 77 (it clobbers the
> reg after all), but there is no way it could clobber it before it uses
> it, that just makes no sense.  And in the combined insn that version of
> flags does not exist at all.

This seems the time period where the email archive is not fully complete;
some of the mails of that 2004 thread apparently were not linked into the
monthly thread list.  This archive seems to have them all:
http://marc.info/?t=108747834900012&r=1&w=2

In any case, this test in can_combine_p rejects a combination for *two*
different issues.  One is the earlyclobber problem, which is what that
2004 thread was about, and which my patch back then relaxed for fixed
hard register.

However, this doesn't seem to apply to the example above; that is really
about the second problem: don't substitute into a clobber.

I understand the reason why this particular substitution is rejected is
simply that if it weren't, we'd be substituting flags:CC=cmp(r84:SI,0x1)
into clobber flags:CC, resulting in clobber cmp(r84:SI,0x1), which is
invalid RTL.

Now I guess this check could be relaxed if somewhere else in combine we'd
recognize the substitution into a clobber and simply omit it in that case.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com