[PATCH] D79391: [DSE] Remove noop stores in MSSA.

[Florian Hahn via Phabricator via llvm-commits]

fhahn requested changes to this revision.
fhahn added reviewers: asbirlea, george.burgess.iv.
fhahn added a comment.
This revision now requires changes to proceed.

Thanks for working on this! Some comments inline.

Another transform that might be worth exploring would be propagating a stored value to loads (users of the MemoryDef that exactly alias the def). Removing loads might enable further store elimination, if done early

I usually try to evaluate the impact by comparing the stats for DeadStoreElimination on the test suite with and without the patch. It would probably be good to do this for this patch as well :)



================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1815
+    if (auto *Load = dyn_cast<LoadInst>(SI->getOperand(0))) {
+      if (Load->getPointerOperand() == SI->getOperand(1)) {
+        State.deleteDeadInstruction(SI);
----------------
I think the check is not strict enough at the moment. I think we have to ensure that there are no memory-writes between the load and the store that may alias them.

In the simple example below, IIUC the patch would currently incorrectly eliminate  ` store i32 %a, i32* %Q`.

```
define void @test(i32* %Q) {
  %a = load i32, i32* %Q
  store i32 10, i32* %Q
  store i32 %a, i32* %Q
  ret void
}```

In terms of MemorySSA, I think we are looking for loads that use the same MemoryDef as `KillingDef`. One simple way to check this would be getting the memoryDef of KillingDef and check if it dominates the load I think.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79391