[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.

>From the comments it seems like you updated the patch, but is it possible that something went wrong with uploading it to reviews.llvm.org? It seems like the patch has not been updated here.



================
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
----------------
zoecarver wrote:
> 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. 
> 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.

yes, but I think with the current ordering of transformations (removing noop stores first) we would remove ` store i32 %v, i32* %p, align 4` and leave `store i32 0, i32* %p`. There are a few cases like this, where the result depends on the exact ordering in which we visit stores & apply transformations.


================
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;
----------------
zoecarver wrote:
> 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). 
>  like those can only be memory defs 
and memory phis, but never plain memory uses


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82588