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

[Bin.Cheng]

On Tue, Sep 2, 2014 at 1:50 AM, Segher Boessenkool
<segher@kernel.crashing.org> wrote:
> 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.
Agreed, otherwise it would be another uninitialized use problem.
Maybe the check is too strict here?  Do you have some archived page
address for that, just saving us some time for digging.

My only concern is, logic in dictribute_notes should also be revisited
under this BZ.  I think the issue will be hidden by changes we are
talking about in can_combine_p.

Thanks,
bin
>
>> >Another question is why is r84 set twice in the first place?
>> Various transformations can set that kind of situation up.
>
> Sure -- but also lazy expanders can reuse a register instead of doing
> gen_reg_rtx.  Which is why I asked :-)
>
>> One could
>> argue that a local pass ssa-like-ize things like this would be good
>> independent of this BZ.  The web code would probably help here, but
>> seems awful heavyweight for an opportunity like this.
>
> Not worth the effort I'd say.
>
>
> Segher