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

[Zoe Carver via Phabricator via llvm-commits]

zoecarver added inline comments.


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


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


================
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
----------------
fhahn wrote:
> 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
> }```
You're right. 

But, in the example, it would be valid to remove both stores. It is guaranteed that `store i32 %v` will always happen immediately after `store i32 0`.

Anyway, I've applied your suggested fix. I'll also add a similar test case. 


================
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;
----------------
fhahn wrote:
> 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.
Yep, you're right. We'll never get here. I originally thought that we may get here through an operand from a phi access but, it looks like those can only be memory defs (which makes sense, I don't know how you'd have a use as a definition without it being a... definition). 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82588