[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#465710, @eli.friedman wrote:

> Oh, I see what you mean about the http://reviews.llvm.org/rL273141 testcase; I thought the stores in the testcase were ordered differently. You run into the issue I was describing if you reorder the last two stores, like this:
>
>   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 i16 0, i16* @g_118, align 1, !dbg !43
>   store i32 0, i32* @g_31, align 1, !dbg !31
>   
>
>  ---


Yes, I believe we'll miss this case if we defer the deletion of aliasing loads/stores.  Let me see if I can take your suggested approach of deleting non-phi nodes, @eli.friedman.  Also, can you please explain why the phi nodes must stick around (at least in the short term)? I don't follow..

Alternatively, I'm wondering if we could use @dberlin's suggestion from http://reviews.llvm.org/D21076 and utilize the iterator that is returned by BB->eraseFromParent() to address the iterator problem.

> The primary point of http://reviews.llvm.org/rL273141 is that DSE shouldn't change behavior based on the presence of debug info... so as long as you're not breaking that invariant, it should be fine to modify/delete the testcase as necessary.


AFAICT, this implementation won't suffer from this problem, so I'm thinking it would be okay to drop the test.


http://reviews.llvm.org/D21613