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

[David Majnemer via llvm-commits]

majnemer added inline comments.

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:804-805
@@ +803,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)
----------------
This has yet to be addressed.

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:809-817
@@ +808,11 @@
+    if (!DT->isReachableFromEntry(Pred) || DT->dominates(BB, Pred))
+      continue;
+    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))))))
+      PredBlocks.push_back(Pred);
+  }
----------------
The nesting here is still very large.  I'm also not entirely convinced that this code is correct.  What if `PredTI->getNumSuccessors() == 1` is true but `PredTI->getSuccessor(0) == Pred` is false?  In that case, won't you access `PredTI->getSuccessor(1) == Pred` ?  It would be safer and more clear to simply loop over `successors`.

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:828-831
@@ +827,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,
----------------
This hasn't been addressed.

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:854
@@ +853,3 @@
+  BasicBlock *BB = Inst->getParent();
+  const DataLayout &DL = BB->getModule()->getDataLayout();
+
----------------
This hasn't been addressed.


http://reviews.llvm.org/D13363