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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Sorry for the long delay! I had to go through the comments again.



================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:103
                       InstOverlapIntervalsTy &IOL, OrderedBasicBlock &OBB,
+                      MapVector<Instruction*, bool> &ThrowableInst,
                       SmallSetVector<const Value *, 16> *ValueSet = nullptr) {
----------------
Please run clang-format before committing. I think this should be `<Instruction *, bool>`.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:119
+    auto It = ThrowableInst.find(DeadInst);
+    if (It != ThrowableInst.end())
+      ThrowableInst.erase(It);
----------------
I think it would be better to not use erase, as you had originally, i.e. set ThrowableInst[DeadInst] = false. What I meant with my last comment was that we could pop false values from the back of ThrowableInst at the end of this function, until we reach one element that's true. Or do it at the place you had originally, just with the right reverse iterator.


================
Comment at: llvm/test/Transforms/DeadStoreElimination/DeleteThrowableInst.ll:1
+; RUN: opt < %s -basicaa -dse -S | FileCheck %s
+
----------------
Can you please also add the test I mentioned in the comment? 


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

https://reviews.llvm.org/D65326