[PATCH] D82588: [DSE] Look through memory PHI arguments when removing noop stores in MSSA.
Zoe Carver via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Sep 30 19:37:46 PDT 2020
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
More information about the llvm-commits
mailing list