[PATCH] D13363: [DeadStoreElimination] Add support for DSE across blocks

[David Majnemer via llvm-commits]

majnemer added a subscriber: majnemer.

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:88
@@ -82,2 +87,3 @@
     bool HandleFree(CallInst *F);
+    bool HandleNonLocalDependency(Instruction *Inst);
     bool handleEndBlock(BasicBlock &BB);
----------------
Functions should start with a lower case letter.

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:616
@@ +615,3 @@
+            // shortening it to not vector size is likely to be slower
+            MemIntrinsic* DepIntrinsic = cast<MemIntrinsic>(DepWrite);
+            unsigned DepWriteAlign = DepIntrinsic->getAlignment();
----------------
Pointers go on the right side, please use `auto *` to avoid repetition.

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:627-630
@@ +626,6 @@
+
+              Value* DepWriteLength = DepIntrinsic->getLength();
+              Value* TrimmedLength = ConstantInt::get(DepWriteLength->getType(),
+                                                      InstWriteOffset -
+                                                      DepWriteOffset);
+              DepIntrinsic->setLength(TrimmedLength);
----------------
Pointers lean right.

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:653-656
@@ +652,6 @@
+      }
+    }
+
+    // DSE across BB
+    else if (InstDep.isNonLocal()) {
+      if (++NumNonLocalAttempts < MaxNonLocalAttempts)
----------------
Please keep the `else if` on the same line as the closing `}`

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:804
@@ +803,3 @@
+static void
+FindSafePreds(SmallVectorImpl<BasicBlock *> &PredBlocks,
+              SmallSetVector<BasicBlock*, 8> &SafeBlocks,
----------------
Functions should start with a lowercase letter.

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:807-808
@@ +806,4 @@
+              BasicBlock *BB, DominatorTree *DT) {
+  for (pred_iterator I = pred_begin(BB), E = pred_end(BB); I != E; ++I) {
+    BasicBlock *Pred = *I;
+    if (Pred == BB)
----------------
Can you use a range-base for loop with `predecessors` ?

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:812-820
@@ +811,11 @@
+    TerminatorInst *PredTI = Pred->getTerminator();
+    if ((PredTI->getNumSuccessors() == 1) ||
+        ((PredTI->getNumSuccessors() == 2) &&
+         ((PredTI->getSuccessor(0) == Pred) ||
+          (PredTI->getSuccessor(1) == Pred) ||
+          (SafeBlocks.count(PredTI->getSuccessor(0)) &&
+           SafeBlocks.count(PredTI->getSuccessor(1)))))) {
+       if (DT->isReachableFromEntry(Pred) && !DT->dominates(BB, Pred))
+        PredBlocks.push_back(Pred);
+    }
+  }
----------------
It would be nicer to split this into more `if (X) continue;` to reduce the heavy nesting.

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:831-834
@@ +830,6 @@
+
+  for (SmallVectorImpl<Value *>::iterator I = Pointers.begin(),
+                                          E = Pointers.end();
+       I != E; ++I) {
+    Value *BObj = *I;
+    if (!AA.isNoAlias(AObj, DL.getTypeStoreSize(AObj->getType()), BObj,
----------------
Could you use a range based for loop here?

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:846
@@ +845,3 @@
+bool DSE::HandleNonLocalDependency(Instruction *Inst) {
+  StoreInst *SI = dyn_cast<StoreInst>(Inst);
+  if (!SI)
----------------
Please use `auto *`.

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:857
@@ +856,3 @@
+  BasicBlock *BB = Inst->getParent();
+  const DataLayout &DL = BB->getModule()->getDataLayout();
+
----------------
Could this be cached in the constructor?

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:876
@@ +875,3 @@
+      // Filter out false dependency from a load to SI looking through phis.
+      if (LoadInst *LI = dyn_cast<LoadInst>(Dependency)) {
+        if (underlyingObjectsDoNotAlias(SI, LI, DL, *AA)) {
----------------
Please use `auto *`.


http://reviews.llvm.org/D13363