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

[Eli Friedman via llvm-commits]

eli.friedman added a comment.

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

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 think that wouldn't introduce any invalidation problems because we only visit reachable blocks.  That said, the outer loop actually catches more cases: if you switch up the stores a little, you can come up with a testcase which current DSE can't handle, but this version can.  Actually, you could even end up with cases where erasing a store in one block allows erasing a store in a different block... but iterating the whole DSE pass might be too expensive to be worthwhile.

Looks fine otherwise.


================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:557
@@ -534,3 +556,3 @@
       // DCE instructions only used to calculate that store.
-      deleteDeadInstruction(Dependency, *MD, *TLI);
+      deleteDeadInstruction(Dependency, *MD, *TLI, nullptr, DeadInstList);
       ++NumFastStores;
----------------
handleFree might need some changes to avoid iterator invalidation issues: I think you can end up with a problem with a block that branches to itself?  You can't do that without tripping over https://llvm.org/bugs/show_bug.cgi?id=28014, I guess, so not really important, but a FIXME would be nice.  (I think I was planning to do something in my version; I forget why I didn't.)


http://reviews.llvm.org/D21613