[PATCH] D21613: [DSE] Avoid iterator invalidation bugs by deferring deletion.

[Chad Rosier via llvm-commits]

mcrosier added a comment.

In http://reviews.llvm.org/D21613#464843, @eli.friedman wrote:

> It's no problem that you're stealing some of my changes. :)


Thanks! :D

> Instead of adding an outer loop, you could solve the http://reviews.llvm.org/rL273141 testcase by only delaying the erasure of PHI nodes rather than the erasure of all operands.


I'm not sure I follow.  Test case for reference:

  define i16 @S0() !dbg !17 {
  bb1:
    store i32 4, i32* @g_31, align 1, !dbg !20
    %_tmp16.i.i = load volatile i16*, i16** @g_30, align 1, !dbg !28
    %_tmp17.i.i = load i16, i16* %_tmp16.i.i, align 1, !dbg !28
    store i16 %_tmp17.i.i, i16* @g_118, align 1, !dbg !20
    store i32 0, i32* @g_31, align 1, !dbg !31
    tail call void @llvm.dbg.value(metadata i32 0, i64 0, metadata !40, metadata !41), !dbg !42
    store i16 0, i16* @g_118, align 1, !dbg !43
    br label %bb2.i, !dbg !44
  
  bb2.i:
    br label %bb2.i, !dbg !44
  }

IIUC, the first store (store i32 4, i32* @g_31) is clobbered by the 3rd store (store i32 0, i32* @g_31), but DSE is unable to realize this because of an aliasing load (%_tmp17.i.i = load i16, i16* %_tmp16.i.i).  The second store (and the loads that alias with the third store) is/are removed when DSE realizes the fourth store (store i16 0, i16* @g_118) clobbers the second.  We fail to catch the http://reviews.llvm.org/rL273141 test case because this change doesn't do any type of back tracking like what is currently done.

Did I miss something, @eli.friedman/@Henric?  I'm not sure how deleting non-phi nodes resolves this issue, Eli.

FWIW, I'd prefer to *not* have the outer loop, but I'm not sure I can resolve http://reviews.llvm.org/rL273141's test case in another way.  My long term goal is to have DSE use MemorySSA and eventually revive http://reviews.llvm.org/rL255247, which was reverted due to compile-time issues; I don't think having the outer loop is inline with my current goals.

One might also argue that regressing http://reviews.llvm.org/rL273141's test case is acceptable, if we're fixing/avoiding iterator invalidation issues.  Obviously, I'd like the best of both worlds, but I may need some direction on how to achieve that goal.


http://reviews.llvm.org/D21613