[PATCH] D65326: Fix for a dangling point bug in DeadStoreElimination pass

[Florian Hahn via Phabricator via llvm-commits]

fhahn accepted this revision.
fhahn added a comment.
This revision is now accepted and ready to land.

LGTM with 2 optional nits, thanks!



================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:155
+  // Pop dead entries from back of ThrowableInst till we find an alive entry.
+  for (auto It = ThrowableInst.rbegin();
+       It != ThrowableInst.rend() && !It->second; ++It) {
----------------
Shouldn't we increment `It` early, as we remove the element `It` refers to? It does not seem to cause any issue, but FWIW, it seems that for SPEC and the test-suite, we always do at most 1 iteration of the loop.


================
Comment at: llvm/test/Transforms/DeadStoreElimination/DeleteThrowableInst.ll:25
+define void @test2(i8** %ptr, i8* %p1, i8* %p2) {
+  store i8* undef, i8** %ptr
+  call void @foo()
----------------
It might be better to store a concrete value instead undef (here and other places in the test casE) to make to test case more robust against future changes.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65326/new/

https://reviews.llvm.org/D65326