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

[Ankit via Phabricator via llvm-commits]

quic_aankit marked 4 inline comments as done.
quic_aankit added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1164
+      // instruction not removed by call to deleteDeadInstruction.
+      Instruction *LastThrowing = nullptr;
+      for(auto it : ThrowableInst) {
----------------
fhahn wrote:
> I think we could move the code to pop the removed instructions from the map to deleteDeadInstructions. That would keep the logic to manage the map in one place and here we can simply use the last instruction in the map as LastThrowing I think.
I've moved the code up in deleteDeadInstructions. I had this here to be able to use inexpensive pop_back instead of the expensive erase. But I think your suggestion makes more sense.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1165
+      Instruction *LastThrowing = nullptr;
+      for(auto it : ThrowableInst) {
+        if (it.second) {
----------------
fhahn wrote:
> AFAIK we are looking for the last throwing instruction we encountered, but this will set it to the first? Shouldn't check the map in reverse insertion order?
> 
> Otherwise we remove the store between the @foo calls, which we would have to preserve, as @foo may throw.
> 
> ```
> declare i8* @_Znwj(i32) local_unnamed_addr
> 
> declare void @foo() readnone
> ; CHECK-LABEL: @test(
> ; CHECK-NEXT: store
> ; CHECK-NEXT: ret void
> define void @test(i8** %ptr, i8* %p1, i8* %p2) {
>   store i8* undef, i8** %ptr
>   call void @foo()
>   store i8* %p1, i8** %ptr
>   call void @foo()
>   store i8* %p2, i8** %ptr
>   %call = call i8* @_Znwj(i32 1)
>   store i8* %call, i8** %ptr
>   store i8* undef, i8** %ptr
>   ret void
> }
> ```
I'm sorry. This had to be a reverse iterator. I've corrected it in the new patch


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

https://reviews.llvm.org/D65326