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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
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.
----------------
kuhar wrote:
> nit: s/use it to update the dominator tree directly/update it directly
I simplified it a bit further: `If \p DT is not nullptr, 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.
----------------
kuhar wrote:
> nit: also here
updated, thanks!


================
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:
> 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
fixed, thanks!


================
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.
----------------
fhahn wrote:
> kuhar wrote:
> > 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
> fixed, thanks!
I updated the comment to hopefully be more clear. The key thing is that it assumes the dominator tree to be up-to-date for all predecessors:

```
 The function assumes the dominator tree is valid for all predecessors and sets \p BB's immediate dominator to the nearest common dominator of all predecessors. \p BB cannot be a predecessor of itself.
```


I also added an assertion that `BB` cannot have itself as predecessor; in that instance, the assumption that the DT is up-to-date for all predecessors doesn't hold, as we just asked to update the DT for BB.


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

I am not sure if that would help too much as the main requirement is that the DT is up-to-date for the predecessors, hopefully the comment update adds enough info.


================
Comment at: llvm/include/llvm/Transforms/Utils/Local.h:504
+/// removed or if the DT for its predecessors changed.
+void updateDominatorTreeUsingPredecessors(BasicBlock *BB, DominatorTree *DT);
+
----------------
kuhar wrote:
> Also, could we have some unit tests? I'm thinking about both having some concrete examples that we can look at and possibly death test to make sure that the assertions catch what we wanted to assume.
I added some small example tests in `llvm/unittests/Transforms/Utils/LocalTest.cpp`. Please let me know if you have other cases in mind. I also built various external binaries and clang bootstrap with DT verification and saw no issues.


================
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)
----------------
kuhar wrote:
> nit: either `BBNode->children()` in the loop directly or use `llvm::to_vector`?
Changing the immediate dominator of a child of BB might invalidate the children iterator I think, so we need a vector. Updated to use `to_vector`, thanks for point out that utility, I wasn't aware of it.


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


================
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.
----------------
kuhar wrote:
> nit: could you add comments with parameter names?
Done, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141487