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

[Ivan Baev via llvm-commits]

ivanbaev marked 15 inline comments as done.

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:572-583
@@ -582,24 +571,14 @@
 
-      // If we find a write that is a) removable (i.e., non-volatile), b) is
-      // completely obliterated by the store to 'Loc', and c) which we know that
-      // 'Inst' doesn't load from, then we can remove it.
-      if (isRemovable(DepWrite) &&
-          !isPossibleSelfRead(Inst, Loc, DepWrite, *TLI, *AA)) {
-        int64_t InstWriteOffset, DepWriteOffset;
-        OverwriteResult OR =
-            isOverwrite(Loc, DepLoc, DL, *TLI, DepWriteOffset, InstWriteOffset);
-        if (OR == OverwriteComplete) {
-          DEBUG(dbgs() << "DSE: Remove Dead Store:\n  DEAD: "
-                << *DepWrite << "\n  KILLER: " << *Inst << '\n');
-
-          // Delete the store and now-dead instructions that feed it.
-          DeleteDeadInstruction(DepWrite, *MD, *TLI);
-          ++NumFastStores;
-          MadeChange = true;
-
-          // DeleteDeadInstruction can delete the current instruction in loop
-          // cases, reset BBI.
-          BBI = Inst;
-          if (BBI != BB.begin())
-            --BBI;
+      while (InstDep.isDef() || InstDep.isClobber()) {
+        // Get the memory clobbered by the instruction we depend on.  MemDep will
+        // skip any instructions that 'Loc' clearly doesn't interact with.  If we
+        // end up depending on a may- or must-aliased load, then we can't optimize
+        // away the store and we bail out.  However, if we depend on on something
+        // that overwrites the memory location we *can* potentially optimize it.
+        //
+        // Find out what memory location the dependent instruction stores.
+        Instruction *DepWrite = InstDep.getInst();
+        MemoryLocation DepLoc = getLocForWrite(DepWrite, *AA);
+        // If we didn't get a useful location, or if it isn't a size, bail out.
+        if (!DepLoc.Ptr)
           break;
----------------
The new patch is more aggressive in searching for redundant stores. Let's defer your suggestion for a subsequent patch.

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:654
@@ +653,3 @@
+    } else if (InstDep.isNonLocal()) { // DSE across BB
+      if (++NumNonLocalAttempts < MaxNonLocalAttempts)
+        MadeChange |= handleNonLocalDependency(Inst);
----------------
MemoryDependenceAnalysis has BlockScanLimit = 100 and NumResultsLimit = 100. With these and the new MaxNonLocalAttempts = 100 we have enough handles to control compile-time. 

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:821
@@ +820,3 @@
+    }
+    if (PredIsSafe)
+      PredBlocks.push_back(Pred);
----------------
If there is a client outside DSE we can move this helper into a utility directory

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:892
@@ +891,3 @@
+      // Don't remove a store within single-block loops;
+      // we need more analysis: e.g. looking for an interferring load
+      // above the store within the loop, etc.
----------------
The idea is to allow the search continue through simple loops, but not to remove stores within loops. 


http://reviews.llvm.org/D13363