[llvm] [DomTree] Make addChild/removeChild private, update NewGVN caller (PR #218164)

[Fangrui Song via llvm-commits]

https://github.com/MaskRay created https://github.com/llvm/llvm-project/pull/218164

NewGVN's value numbering loop converges more quickly when blocks are
processed in a reverse post order. The code additionally ensures a
property the pass does not need (commit 6658cc9ead67 in 2016): in a
preorder of the dominator tree the instructions dominated by a block are
contiguous.

Use the default RPO and drop addChild/removeChild callers, so that the
two members can be made private. `opt -passes=newgvn` slightly
decreases.


>From 5f60d7241a10de56544e4a10c86941b966f13ab5 Mon Sep 17 00:00:00 2001
From: Fangrui Song <i at maskray.me>
Date: Sat, 22 Aug 2026 12:36:41 -0700
Subject: [PATCH] [DomTree] Make addChild/removeChild private, update NewGVN
 caller

NewGVN's value numbering loop converges more quickly when blocks are
processed in a reverse post order. The code additionally ensures a
property the pass does not need (commit 6658cc9ead67 in 2016): in a
preorder of the dominator tree the instructions dominated by a block are
contiguous.

Use the default RPO and drop addChild/removeChild callers, so that the
two members can be made private. `opt -passes=newgvn` slightly
decreases.
---
 llvm/include/llvm/Support/GenericDomTree.h | 46 +++++++++++-----------
 llvm/lib/Transforms/Scalar/NewGVN.cpp      | 34 ++--------------
 2 files changed, 25 insertions(+), 55 deletions(-)

diff --git a/llvm/include/llvm/Support/GenericDomTree.h b/llvm/include/llvm/Support/GenericDomTree.h
index 413dd80a85f14..cc06c6e486aaf 100644
--- a/llvm/include/llvm/Support/GenericDomTree.h
+++ b/llvm/include/llvm/Support/GenericDomTree.h
@@ -113,30 +113,6 @@ template <class NodeT> class DomTreeNodeBase {
   DomTreeNodeBase *getIDom() const { return IDom; }
   unsigned getLevel() const { return Level; }
 
-  // TODO: make these private once NewGVN doesn't require these anymore.
-  void addChild(DomTreeNodeBase *C) {
-    assert(!C->Sibling && "cannot add child that already has siblings");
-    assert(!*AppendPtr && "sibling of last child must be nullptr");
-    *AppendPtr = C;
-    AppendPtr = &C->Sibling;
-  }
-
-  // TODO: make these private once NewGVN doesn't require these anymore.
-  void removeChild(DomTreeNodeBase *C) {
-    DomTreeNodeBase **It = &FirstChild;
-    while (*It != C) {
-      assert(*It != nullptr && "Not in immediate dominator children list!");
-      It = &(*It)->Sibling;
-    }
-    assert(!*AppendPtr && "sibling of last child must be nullptr");
-    assert(C->Sibling || AppendPtr == &C->Sibling);
-    *It = C->Sibling;
-    if (C->Sibling)
-      C->Sibling = nullptr;
-    else
-      AppendPtr = It;
-  }
-
   bool isLeaf() const { return FirstChild == nullptr; }
 
   bool compare(const DomTreeNodeBase *Other) const {
@@ -177,6 +153,28 @@ template <class NodeT> class DomTreeNodeBase {
   unsigned getDFSNumOut() const { return DFSNumOut; }
 
 private:
+  void addChild(DomTreeNodeBase *C) {
+    assert(!C->Sibling && "cannot add child that already has siblings");
+    assert(!*AppendPtr && "sibling of last child must be nullptr");
+    *AppendPtr = C;
+    AppendPtr = &C->Sibling;
+  }
+
+  void removeChild(DomTreeNodeBase *C) {
+    DomTreeNodeBase **It = &FirstChild;
+    while (*It != C) {
+      assert(*It != nullptr && "Not in immediate dominator children list!");
+      It = &(*It)->Sibling;
+    }
+    assert(!*AppendPtr && "sibling of last child must be nullptr");
+    assert(C->Sibling || AppendPtr == &C->Sibling);
+    *It = C->Sibling;
+    if (C->Sibling)
+      C->Sibling = nullptr;
+    else
+      AppendPtr = It;
+  }
+
   // Return true if this node is dominated by other. Use this only if DFS info
   // is valid.
   bool DominatedBy(const DomTreeNodeBase *other) const {
diff --git a/llvm/lib/Transforms/Scalar/NewGVN.cpp b/llvm/lib/Transforms/Scalar/NewGVN.cpp
index 1adbfb9c2c7d6..9913a4d23c1a1 100644
--- a/llvm/lib/Transforms/Scalar/NewGVN.cpp
+++ b/llvm/lib/Transforms/Scalar/NewGVN.cpp
@@ -3441,43 +3441,15 @@ bool NewGVN::runGVN() {
   unsigned ICount = 1;
   // Add an empty instruction to account for the fact that we start at 1
   DFSToInstr.emplace_back(nullptr);
-  // Note: We want ideal RPO traversal of the blocks, which is not quite the
-  // same as dominator tree order, particularly with regard whether backedges
-  // get visited first or second, given a block with multiple successors.
-  // If we visit in the wrong order, we will end up performing N times as many
+  // Note: Number the blocks in RPO to put every definition before its uses,
+  // except for a PHI operand arriving along a back edge. A wrong order costs
   // iterations.
-  // The dominator tree does guarantee that, for a given dom tree node, it's
-  // parent must occur before it in the RPO ordering. Thus, we only need to sort
-  // the siblings.
   ReversePostOrderTraversal<Function *> RPOT(&F);
   unsigned Counter = 0;
-  for (auto &B : RPOT) {
+  for (BasicBlock *B : RPOT) {
     auto *Node = DT->getNode(B);
     assert(Node && "RPO and Dominator tree should have same reachability");
     RPOOrdering[Node] = ++Counter;
-  }
-  // Sort dominator tree children arrays into RPO.
-  // TODO: this code shouldn't rely on domtree internals. It also most probably
-  // shouldn't rely on the order of nodes in the tree...
-  for (auto &B : RPOT) {
-    auto *Node = DT->getNode(B);
-    if (Node->isLeaf())
-      continue;
-    SmallVector<DomTreeNode *> Children;
-    while (!Node->isLeaf()) {
-      Children.push_back(*Node->begin());
-      Node->removeChild(*Node->begin());
-    }
-    llvm::sort(Children, [&](const DomTreeNode *A, const DomTreeNode *B) {
-      return RPOOrdering[A] < RPOOrdering[B];
-    });
-    for (DomTreeNode *Child : Children)
-      Node->addChild(Child);
-  }
-
-  // Now a standard depth first ordering of the domtree is equivalent to RPO.
-  for (auto *DTN : depth_first(DT->getRootNode())) {
-    BasicBlock *B = DTN->getBlock();
     const auto &BlockRange = assignDFSNumbers(B, ICount);
     BlockInstRange.insert({B, BlockRange});
     ICount += BlockRange.second - BlockRange.first;