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

[luxufan via Phabricator via llvm-commits]

StephenFan added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1896
+                      CurrentDef->getDefiningAccess(),
+                      MemoryLocation::get(LoadI)));
+                  continue;
----------------
nikic wrote:
> 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?
I hope I understand what you mean. IMO, we will run into `storeIsNoop` Function with a MemoryDef, and then search from MemoryAccess which is returned from `getClobberingMemoryAccess(Def)`. So I think we won't run into problems because the `storeIsNoop` code already looked through MemoryPhis.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132657