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

[Jeff Law <law@redhat.com>]

On 08/27/14 23:04, Bin.Cheng wrote:
> On Wed, Aug 27, 2014 at 6:34 PM, Richard Earnshaw <rearnsha@arm.com> wrote:
>> On 27/08/14 11:08, Bin Cheng wrote:
>>> Hi
>>> As reported in bug pr62151, combine pass may wrongly delete necessary
>>> instruction in function distribute_notes thus leaving register
>>> uninitialized.  This patch is to fix the issue by checking if i2 immediately
>>> modifies the register in REG_DEAD note.  If yes, set tem_insn accordingly to
>>> start finding right place for note distribution from i2.
>>>
>>> I once sent a RFC patch at
>>> https://gcc.gnu.org/ml/gcc-patches/2014-08/msg01718.html, but got no
>>> comments,  here I added some comments in this patch to make it a formal one.
>>>
>>>
>>> I tested the original patch, and will re-test it against the latest code
>>> later.  So is it OK?  Any comments will be appreciated.
>>>
>>
>> Isn't this the sort of sequence that combinable_i3pat is supposed to reject?
> Hi Richard,
> I think it's not.  combinable_i3pat checks cases in which i1 feeds to
> i3 directly, while i2 kills reg_use in i1.  For this case the feeding
> chain is "i0->i1->i2->i3", the combination is valid and beneficial.
> What needs to be handled is if i2dest is anticipated after i3.  If
> not, then i2 could be deleted; if yes, i2 should be preserved.
> Moreover, following constant propagation could delete i2 after
> propagating the new i2src into i4.  Note combine pass already handles
> this kind of case using variable added_sets_2 in function try_combine.
> The problem is in distribute_notes which mis-deleted i2.
>
> I added one test case in the updated patch.
One could argue that this mess is a result of trying to optimize a reg 
that is set more than once.    Though I guess that might be a bit of a 
big hammer.

I haven't thought real hard, but isn't it the case that for a pseudo 
with multiple sets that we never want to move a REG_DEAD note across a 
set of that pseudo?  It would seem that in these cases we want to drop 
the REG_DEAD note completely.

Note that i0..i4 need not be consecutive insns, so you'd have to walk 
the chain from the location with the death note to the proposed death 
note site.  If between those locations there's another set of the same 
pseudo, then drop the note.  Since this may be an expensive check it 
should probably be conditionalized on REG_N_SETS (pseudo) > 1

Jeff