On Sat, Aug 30, 2014 at 1:58 PM, Jeff Law <l...@redhat.com> wrote: > On 08/27/14 23:04, Bin.Cheng wrote: >> >> On Wed, Aug 27, 2014 at 6:34 PM, Richard Earnshaw <rearn...@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.
Hi Jeff, Thanks very much for the review. > > 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. As noted by Segher, it's a quite useful optimization which we don't want to disable. > > 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 Here is the complicated part. The from_insn is i1, i2/i3 are the following instructions. The original logic seems to me is scanning from i3 for one insn for distribution of REG_DEAD note in from_insn. Since the last use is in from_insn, it makes no sense to scan from i3 (after from_insn). What we need to do is scanning from from_insn in backward trying to find a place for note distribution. If we run into a useless set of the note reg, we can just delete that insn or add REG_UNUSED to it. It just seems not right to do this on instructions after from_insn, which causes the wrong code in this specific case. Any comments? Thanks, bin
