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

[Florian Hahn via Phabricator via llvm-commits]

fhahn requested changes to this revision.
fhahn added a comment.
This revision now requires changes to proceed.

Sorry for the long delay, this review dropped off my radar somehow.



================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1854
+      // Check all the operands.
+      for (unsigned PhiIndex = 0; PhiIndex < PhiAccess->getNumOperands();
+           ++PhiIndex)
----------------
Perhaps you could use the incoming_values iterator range here? `        for (Value *V : PhiAccess->incoming_values()) {`


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1869
+      // If the store is not dead, we need to keep checking.
+      continue;
+    }
----------------
I think we could also cover more cases here, by skipping no-aliasing MemoryDefs and checking that there are no aliasing reads.. But that's future work. Perhaps a comment/TODO would be good.


================
Comment at: llvm/test/Transforms/DeadStoreElimination/MSSA/simple.ll:294
-; CHECK-NEXT:    [[V:%.*]] = load i32, i32* [[P:%.*]], align 4
 ; CHECK-NEXT:    br i1 [[C:%.*]], label [[BB1:%.*]], label [[BB2:%.*]]
 ; CHECK:       bb1:
----------------
The changes in this test file are not legal transforms I think. In this example, we cannot remove the start as the store at line 311 might overwrite %p, if %p == %p2 and then the store is not dead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82588