[PATCH] D41302: [LoopSimplify] Preserve Post Dom Trees across Loop Simplify

Jakub Kuderski via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jan 24 10:56:33 PST 2018


kuhar added a comment.

Looks nice, I think that using DeferredDominance as a single DomTree updated object is the right direction.
I just quickly went though the patch and left some comments, but I think this approach is good enough for the first step. Ideally, DDT should update DT first and use the reachability information to update PDT faster. I will comment more on this later when I find some more time.



================
Comment at: include/llvm/Analysis/PostDominators.h:33
+  PostDominatorTree() = default;
+  explicit PostDominatorTree(Function &F) { recalculate(F); }
+
----------------
Nice!


================
Comment at: lib/Transforms/Utils/BasicBlockUtils.cpp:324
+  if (DDT) {
+    std::vector<DominatorTree::UpdateType> Updates;
+    Updates.push_back({DominatorTree::UpdateKind::Insert, NewBB, OldBB});
----------------
We can reserve `Updates` here. `1 + 2 * Preds.size()`?


================
Comment at: lib/Transforms/Utils/BasicBlockUtils.cpp:350
     // as true and make the NewBB the header of some loop. This breaks LI.
-    if (!DT->isReachableFromEntry(Pred))
+    if (!DT.isReachableFromEntry(Pred))
       continue;
----------------
Do we always have to flush DDT when we get to this condition here? I think that it's possible to tell that Pred is not reachable if it's enqueued for deletion. 


================
Comment at: lib/Transforms/Utils/BasicBlockUtils.cpp:350
     // as true and make the NewBB the header of some loop. This breaks LI.
-    if (!DT->isReachableFromEntry(Pred))
+    if (!DT.isReachableFromEntry(Pred))
       continue;
----------------
kuhar wrote:
> Do we always have to flush DDT when we get to this condition here? I think that it's possible to tell that Pred is not reachable if it's enqueued for deletion. 
Just eyeballing the function, this seems to be the only use od DT in the rest of this function. I'm not sure if always flushing it above is not too conservative/costly.


================
Comment at: lib/Transforms/Utils/BasicBlockUtils.cpp:538
+  }
+  return SplitBlockPredecessors(BB, Preds, Suffix, (DeferredDominance *)nullptr,
+                                LI, PreserveLCSSA);
----------------
`SplitBlockPredecessors(BB, Preds, Suffix, DT ? DDT(*DT) : nullptr, LI, PreserveLCSSA);`?


================
Comment at: lib/Transforms/Utils/LoopSimplify.cpp:160
+  }
+  return InsertPreheaderForLoop(L, (DeferredDominance *)nullptr, LI,
+                                PreserveLCSSA);
----------------
Like above.


================
Comment at: lib/Transforms/Utils/LoopSimplify.cpp:469
   // Update dominator information
-  DT->splitBlock(BEBlock);
+  std::vector<DominatorTree::UpdateType> Updates;
+  Updates.push_back({DominatorTree::UpdateKind::Insert, BEBlock, Header});
----------------
Nit: we can reserve here as well.


================
Comment at: lib/Transforms/Utils/LoopSimplify.cpp:632
+      BasicBlock *Predecessor = ExitingBlock->getSinglePredecessor();
+      if (!Predecessor) continue;
       BranchInst *BI = dyn_cast<BranchInst>(ExitingBlock->getTerminator());
----------------
I would appreciate an extra line here, the continue blends in and it's easy to miss it.


================
Comment at: lib/Transforms/Utils/LoopSimplify.cpp:666
+      // Get a list of required Dominator Tree edge updates
+      std::vector<DominatorTree::UpdateType> DTUpdates;
+      TerminatorInst *PredecessorTerm = Predecessor->getTerminator();
----------------
SmallVector?


================
Comment at: lib/Transforms/Utils/LoopSimplify.cpp:668
+      TerminatorInst *PredecessorTerm = Predecessor->getTerminator();
+      BasicBlock *PredOtherBB = ExitingBlock == PredecessorTerm->getSuccessor(0)
+                                    ? PredecessorTerm->getNumSuccessors() == 1
----------------
Can you rewrite it as a series of if's? It's difficult to parse for me.


================
Comment at: lib/Transforms/Utils/LoopSimplify.cpp:763
+  }
+  return simplifyLoop(L, (DeferredDominance *)nullptr, LI, SE, AC,
+                      PreserveLCSSA);
----------------
Same as above.


================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:1136
+  }
+  return formDedicatedExitBlocks(L, (DeferredDominance *)nullptr, LI,
+                                 PreserveLCSSA);
----------------
Same.


================
Comment at: test/Transforms/LoopSimplify/2004-04-13-LoopSimplifyUpdateDomFrontier.ll:1
-; RUN: opt < %s -sroa -loop-simplify -licm -disable-output -verify-dom-info -verify-loop-info
+; RUN: opt < %s -sroa -postdomtree -loop-simplify -licm -disable-output -verify-dom-info -verify-loop-info
 
----------------
Is PDT required? Maybe we should test both with and without it?


https://reviews.llvm.org/D41302





More information about the llvm-commits mailing list