[PATCH] D82588: [DSE] Look through memory PHI arguments when removing noop stores in MSSA.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Thanks for updating the patch! Some more comments inline, but I think it is almost there.



================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:2405
+        bool IsDead = LoadAccess == Def->getDefiningAccess();
+        if (IsDead)
+          return true;
----------------
I think it would be easier to get rid of the `IsDead` variable and just have 

```if (LoadAccess == Def->getDefiningAccess()) return true```


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:2421
+            // Check all the operands.
+            for (auto *V : PhiAccess->incoming_values())
+              ToCheck.insert(V);
----------------
I think this won't build. I think you'd need to use `auto &Use : ` and `insert(cast<MemoryAccess>(&Use))`.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:2432
+            // that is the same definition as the load, then this is a noop.
+            IsDead = LoadAccess == Current;
+            // Either way, we need to keep checking to make sure there aren't
----------------
I think this may lead to incorrect results, if the first incoming value is not the LoadAccess but the second one is. For example, consider the function below. I think we can just return `false` if `LoadAccess != Current`. And it should be safe to assert that current is a MemoryDef here.

```
define i32 @test(i1 %c, i32* %p) {
entry:
  %v = load i32, i32* %p, align 4
  br i1 undef, label %bb0, label %bb0.0

bb0:
  store i32 0, i32* %p
  br label %bb1

bb0.0:
  br label %bb1

bb1:
  store i32 %v, i32* %p, align 4
  br label %bb2
bb2:
  ret i32 0
}```


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:2440
+          // memory is the same as the load's defining access.
+          if (cast<MemoryUse>(Current)->getDefiningAccess() == LoadAccess)
+            IsDead = true;
----------------
I don't think we can reach this code? Both the incoming values and `getDefiningAccess` should return either a MemoryDef or a MemoryPhi, but never a MemoryUse. I don't think this code is needed.


================
Comment at: llvm/test/Transforms/DeadStoreElimination/MSSA/noop-stores.ll:119
+  store i32 %v, i32* %p, align 4
+  br i1 undef, label %bb1, label %bb2
+bb2:
----------------
It would be good to avoid using branches on `undef`, because that's UB. Instead you can use `i1` arguments for example. In this case, you could use the `%c` argument, (Same for other tests)


================
Comment at: llvm/test/Transforms/DeadStoreElimination/MSSA/simple-todo.ll:8
+; Remove both redundant stores if loaded value is in another block inside a loop.
+define i32 @test47(i1 %c, i32* %p, i32 %i) {
+; CHECK-LABEL: @test47(
----------------
IIUC we should now handle the original case correctly? Can you move it to `simple.ll` and perhaps add the additional case to `noop-stores.ll`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82588