[PATCH] D132657: [DSE] Eliminate noop store even through has clobbering between LoadI and StoreI

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1896
+                      CurrentDef->getDefiningAccess(),
+                      MemoryLocation::get(LoadI)));
+                  continue;
----------------
StephenFan wrote:
> nikic wrote:
> > This should probably use the Store location -- the location itself is the same, but they may differ in AAMDNodes.
> > 
> > Is this code correct in the presence of backedges? We are looking through MemoryPhis above, so we might be looking at a previous loop iteration. This should probably have a loop invariance check on the location.
> I think the loop invariance check is not needed. The loop invariance check is needed when we do optimization that depends on AliasAnalysis. In noop-stores detection, aliasing analysis is not needed. Although MemorySSA needs alias analysis, it will do the loop invariance check when it searches for clobbering memory access.
MemorySSA will only do the loop invariance check when it looks through a MemoryPhi though -- won't we run into problems because the DSE code already looked through MemoryPhis, so MemorySSA will not know that an invariance check has to be performed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132657