[PATCH] D13363: [DeadStoreElimination] Add support for non-local DSE

[Mitch Bodart via llvm-commits]

mbodart added inline comments.

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:653
@@ +652,3 @@
+      }
+    } else if (InstDep.isNonLocal()) { // DSE across BB
+      if (++NumNonLocalAttempts < MaxNonLocalAttempts)
----------------
ivanbaev wrote:
> mbodart wrote:
> > The self read issue has not been addressed.
> It looks like that some comments are not aligned to the corresponding lines of code.
> The self read issue has been addressed in line 888 by adding there a call to: 
>      isPossibleSelfRead(Inst, Loc, Dependency, *TLI, *AA)
> It is similar to the local case.
It seems useful for compile time to check for a self-read right here, as in that case there is no point in processing non-local dependences.   Such a check is only complicated by the fact that isPossibleSelfRead requires a "DepWrite" parameter.  We would need to modify that routine to allow a nullptr for DepWrite.

What I'm unsure of, however, is whether this being a non-self read is already implied.
Is it the case that a StoreInst will never be a self read, for example?

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:808
@@ +807,3 @@
+      continue;
+    if (!DT->isReachableFromEntry(Pred) || DT->dominates(BB, Pred))
+      continue;
----------------
ivanbaev wrote:
> mbodart wrote:
> > I still don't understand why we need to check DT->dominates(BB, Pred) here.
> > I'm guessing you're trying to guard against an infinite loop, which of course
> > we need to do.  But I think that would be automatically handled by instead checking
> > for !SafeBlocks.count(Pred).  In addition to allowing the search to proceed further, this
> > would eliminate the need for the special check of Pred == BB, since BB must be in SafeBlocks.
> We need  DT->dominates(BB, Pred) check to avoid situations similar to the one in cycle.ll test below. This test is a simplified version of a larger function found in a SPEC benchmark.
If nonlocal DSE would otherwise remove the store in the cycle.ll test, it seems to me that's a problem with how we are using alias analysis.  The other store in that loop is not guaranteed to overwrite the removed store's location on every loop iteration.

Perhaps it's simply my unfamiliarity with LLVM's alias analysis interfaces, but it seems like there would be some support for checking this definitive overlap, without requiring clients to also factor in dominance information.  I'd be interested in hearing what the other reviewers have to say.


http://reviews.llvm.org/D13363