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

[Ivan Baev via llvm-commits]

ivanbaev marked 2 inline comments as done.

================
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)
----------------
majnemer wrote:
> This has yet to be addressed.
This is similar to the code for iterating over predecessors in existing void FindUnconditionalPreds() function. I suggest to change both in a separate patch.

================
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);
+  }
----------------
majnemer wrote:
> 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`.
if PredTI->getNumSuccessors() == 1 is true, then we exit the if statement checks at this point, so we will not access PredTI->getSuccessor(0), etc.

Do you think that the following is better? 
if (PredTI->getNumSuccessors() == 1)  {
  PredBlocks.push_back(Pred);
  continue;
}
if (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);


================
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,
----------------
majnemer wrote:
> This hasn't been addressed.
There are about 8 places in DeadStoreElimination.cpp where we can use range based for loops. I suggest these changes to be made in a separate patch.

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:854
@@ +853,3 @@
+  BasicBlock *BB = Inst->getParent();
+  const DataLayout &DL = BB->getModule()->getDataLayout();
+
----------------
majnemer wrote:
> This hasn't been addressed.
That is a good point. There are many places in DSE where DataLayout is used or passed as argument. However, it seems that most transformations in Transform/*, e.g. Vectorize/LoopVectorize.cpp, Scalar/SROA.cpp, Scalar/GVN.cpp, favor accessing DataLayout this way. 


http://reviews.llvm.org/D13363