[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:
> A StoreInst will never be a self read.
If that's the case, then we should be able to remove the isSelfRead check in handleNonLocalDependency.

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:808
@@ +807,3 @@
+      continue;
+    if (!DT->isReachableFromEntry(Pred) || DT->dominates(BB, Pred))
+      continue;
----------------
ivanbaev wrote:
> By design, DSE - local, non-local, other parts - depends on MemoryDependenceAnalysis, and only occasionally relies on alias analysis. 
I was using the term alias analysis generically, referring to either AA or MD.
If usage of those interfaces is suspect in the circumstances of the cycle.ll test,
I'd like to understand why exactly.


================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:887
@@ +886,3 @@
+          !isRemovable(Dependency) ||
+          isPossibleSelfRead(Inst, Loc, Dependency, *TLI, *AA))
+        break;
----------------
Inst here is the original StoreInst.  If StoreInst's can never be a self read, this check will always be false.  Do we need an additional check to ensure that Dependency does not read any of Inst's memory, analagous to the check "if (AA->getModRefInfo(DepWrite, Loc) & MRI_Ref)"
in DSE::runOnBasicBlock?

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:896
@@ +895,3 @@
+        if (Succ == PB)
+          break;
+      }
----------------
This break is just exiting the succ iterator loop.  We need to exit the containing while loop.


http://reviews.llvm.org/D13363