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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

In D65326#1605875 <https://reviews.llvm.org/D65326#1605875>, @quic_aankit wrote:

> In D65326#1603693 <https://reviews.llvm.org/D65326#1603693>, @fhahn wrote:
>
> > I think `SetVector` would be more suitable and lead to slightly simpler code (http://llvm.org/doxygen/classllvm_1_1SetVector.html)
> >
> > Also, could you upload the patch with more context? (`git show HEAD -U999999`)
>
>
> Uploaded the patch with more context.
>
> The main motivation to use MapVector is for searching the deletedInstruction in the list of lastThrowing Instructions (Line120). With SetVector this step will be expensive.


Right, the interface to SetVector is a bit unfortunate in that respect :( It's probably not worth generalizing that for now, especially if we can keep the logic to clean up the map in deleteDeadInstructions.

Some comments inline. Would it be possible to add a test where multiple entries are removed from the mapping in a single deleteDeadInstructions call? Not sure if that can be the case, IIUC that would require deleting a function that may throw with an argument that may throw as well, but can be removed once it is dead.



================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:118
+    // Mark the DeadInst as dead in the list of throwable instructions.
+    auto it = ThrowableInst.find(DeadInst);
+    if (it != ThrowableInst.end())
----------------
LLVM convention is to have upper case names for variables (same below)


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1164
+      // instruction not removed by call to deleteDeadInstruction.
+      Instruction *LastThrowing = nullptr;
+      for(auto it : ThrowableInst) {
----------------
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.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1165
+      Instruction *LastThrowing = nullptr;
+      for(auto it : ThrowableInst) {
+        if (it.second) {
----------------
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
}
```


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

https://reviews.llvm.org/D65326