[PATCH] D141487: [LoopUnroll] Directly update DT instead of DTU.

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added a comment.

Some thoughts:

1. If possible, it'd be great if we could have a repro to use as a benchmark for the updater. And experiment with. In the past, I found that manually instrumenting the construction & updater algorithms with event counters (e.g., number of nodes visited by DFS, number of updates in-flight, initial/final tree size) to give more insight than perf traces.
2. I find it very hard to prove that manual updates are correct. We found a lot of very rare corner cases that required combination of things like infinite loops, multi-edges, irreducible loops, etc., that broke seemingly correct manual update functions. This makes me strongly prefer not to add more manual update code, unless we have a very good justification. Here, 200s -> 2s seems convincing for me if we cannot find other workarounds.
3. I haven't looked into this specific case closely, but I'll list some generic alternative ideas:
  - See if we can determine to recalculate in this case and if this would be faster than running the updater.
  - Detect the unreachable node case in the updater and handle it there.
4. Could you add assertions to verify the tree just before and after applying the updates manually? This can be guarded with `EXPENSIVE_CHECKS`.



================
Comment at: llvm/include/llvm/Transforms/Utils/BasicBlockUtils.h:93
 /// BB, and BB will still be merged into its predecessor and removed.
+/// If \p DT is not nullptr, use it to update the dominator tree directly; in
+/// that case, DTU must be nullptr.
----------------
nit: s/use it to update the dominator tree directly/update it directly


================
Comment at: llvm/include/llvm/Transforms/Utils/Local.h:345
 /// instruction, making it and the rest of the code in the block dead.
+/// If \p DT is not nullptr, use it to update the dominator tree directly; in
+/// that case, DTU must be nullptr.
----------------
nit: also here


================
Comment at: llvm/include/llvm/Transforms/Utils/Local.h:501
 
+/// Update the \p DT for \p BB using it's predecessors. This can be used to
+/// update the dominator tree if predecessors of  \p BB have been added or
----------------
kuhar wrote:
> It's not clear to me what the exact assumptions are here. I'm thinking about things like predecessors having edges to other predecessors and other corner cases.
> 
> In addition, could you add some ascii diagrams to illustrate these two cases (new/deleted)?
nit: s/it's/its


================
Comment at: llvm/include/llvm/Transforms/Utils/Local.h:501-503
+/// Update the \p DT for \p BB using it's predecessors. This can be used to
+/// update the dominator tree if predecessors of  \p BB have been added or
+/// removed or if the DT for its predecessors changed.
----------------
It's not clear to me what the exact assumptions are here. I'm thinking about things like predecessors having edges to other predecessors and other corner cases.

In addition, could you add some ascii diagrams to illustrate these two cases (new/deleted)?


================
Comment at: llvm/lib/Transforms/Utils/BasicBlockUtils.cpp:242
+      assert(BBNode && "PredNode unreachable but BBNode reachable?");
+      SmallVector<DomTreeNode *> Children(BBNode->children());
+      for (DomTreeNode *C : Children)
----------------
nit: either `BBNode->children()` in the loop directly or use `llvm::to_vector`?


================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:697
+    updateDominatorTreeUsingPredecessors(DeadSucc, DT);
+    SmallVector<DomTreeNode *> Children(DT->getNode(Src)->children());
+    for (auto *C : Children) {
----------------
nit: also here


================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:777-778
       BasicBlock *Fold = Dest->getUniquePredecessor();
-      if (MergeBlockIntoPredecessor(Dest, &DTU, LI)) {
+      if (MergeBlockIntoPredecessor(Dest, nullptr, LI, nullptr, nullptr, false,
+                                    DT)) {
         // Dest has been folded into Fold. Update our worklists accordingly.
----------------
nit: could you add comments with parameter names?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141487/new/

https://reviews.llvm.org/D141487