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

[Zoe Carver via Phabricator via llvm-commits]

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

Just an issue with `arc`. Sorry for the confusion. I also applied you're other suggested edits (using `%c` and moving `test47`). Thanks for the review!



================
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:
> 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.
When I tested it we removed both (albeit for the wrong reason). Maybe because we visit them in reverse order. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82588