[DomTree] Fix root attachment in runDFS()

[nikic]

Currently, runDFS() only sets the Parent of the DFS root if it is already in the NodeToInfo map. This doesn't matter if we're running DFS on the DT root, which doesn't have a parent anyway. However, when running on PDT roots, this means we end up keeping the parent at 0, rather than setting it to 1 for the virtual PDT root. Because the virtual root (nullptr) has the same value as the dummy value in NumToNode (nullptr) this happens to work out by accident right now.

I believe we should always be setting the parent in runDFS(), and adjust AttachToNum in doFullDFSWalk() to be 1 (the virtual root) for PDTs.

[llvmbot]

@llvm/pr-subscribers-llvm-support

Author: Nikita Popov (nikic)

Changes

Currently, runDFS() only sets the Parent of the DFS root if it is already in the NodeToInfo map. This works out okay if we're running DFS on the DT root, which doesn't have a parent anyway. However, when running on PDT roots, this means we end up keeping the parent at 0, rather than setting it to 1 for the virtual PDT root. Because the virtual root (nullptr) has the same value as the dummy value in NumToNode (nullptr) this happens to work out by accident right now.

I believe we should always be setting the parent in runDFS(), and adjust AttachToNum in doFullDFSWalk() to be 1 (the virtual root) for PDTs.

---

Full diff: https://github.com/llvm/llvm-project/pull/73148.diff

1 Files Affected:

• (modified) llvm/include/llvm/Support/GenericDomTreeConstruction.h (+3-2)

diff --git a/llvm/include/llvm/Support/GenericDomTreeConstruction.h b/llvm/include/llvm/Support/GenericDomTreeConstruction.h
index 6564f98ab0234b0..0a70648c9eab36e 100644
--- a/llvm/include/llvm/Support/GenericDomTreeConstruction.h
+++ b/llvm/include/llvm/Support/GenericDomTreeConstruction.h
@@ -181,7 +181,7 @@ struct SemiNCAInfo {
                   const NodeOrderMap *SuccOrder = nullptr) {
     assert(V);
     SmallVector<NodePtr, 64> WorkList = {V};
-    if (NodeToInfo.count(V) != 0) NodeToInfo[V].Parent = AttachToNum;
+    NodeToInfo[V].Parent = AttachToNum;
 
     while (!WorkList.empty()) {
       const NodePtr BB = WorkList.pop_back_val();
@@ -306,6 +306,7 @@ struct SemiNCAInfo {
     for (unsigned i = 2; i < NextDFSNum; ++i) {
       const NodePtr W = NumToNode[i];
       auto &WInfo = NodeToInfo[W];
+      assert(WInfo.Semi != 0);
       const unsigned SDomNum = NodeToInfo[NumToNode[WInfo.Semi]].DFSNum;
       NodePtr WIDomCandidate = WInfo.IDom;
       while (NodeToInfo[WIDomCandidate].DFSNum > SDomNum)
@@ -552,7 +553,7 @@ struct SemiNCAInfo {
 
     addVirtualRoot();
     unsigned Num = 1;
-    for (const NodePtr Root : DT.Roots) Num = runDFS(Root, Num, DC, 0);
+    for (const NodePtr Root : DT.Roots) Num = runDFS(Root, Num, DC, 1);
   }
 
   static void CalculateFromScratch(DomTreeT &DT, BatchUpdatePtr BUI) {

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff edf5cae7391cdb097a090ea142dfa7ac6ac03555 a5909c31590bd13eb46c81c8dd3dfae7911f489c -- llvm/include/llvm/Support/GenericDomTreeConstruction.h

View the diff from clang-format here.

diff --git a/llvm/include/llvm/Support/GenericDomTreeConstruction.h b/llvm/include/llvm/Support/GenericDomTreeConstruction.h
index a85cd6ca50..1f2eb8f155 100644
--- a/llvm/include/llvm/Support/GenericDomTreeConstruction.h
+++ b/llvm/include/llvm/Support/GenericDomTreeConstruction.h
@@ -553,7 +553,8 @@ struct SemiNCAInfo {
 
     addVirtualRoot();
     unsigned Num = 1;
-    for (const NodePtr Root : DT.Roots) Num = runDFS(Root, Num, DC, 1);
+    for (const NodePtr Root : DT.Roots)
+      Num = runDFS(Root, Num, DC, 1);
   }
 
   static void CalculateFromScratch(DomTreeT &DT, BatchUpdatePtr BUI) {

[dwblaikie]

Does this need test coverage? Or is the assert essentially adding test coverage? (eg: would adding the assert have fired at ToT without the other changes?)

[nikic]

Does this need test coverage? Or is the assert essentially adding test coverage? (eg: would adding the assert have fired at ToT without the other changes?)

Yes, the assert would fire without the changes. The change is intended to be NFC in terms of externally observable behavior, it only fixes internal state that is incorrect in a way that ends up not mattering with the current implementation. I ran into it when implementing #73097.

[dwblaikie]

Yes, the assert would fire without the changes. The change is intended to be NFC in terms of externally observable behavior, it only fixes internal state that is incorrect in a way that ends up not mattering with the current implementation.

Ah, cool - thanks for the explanation!

[kuhar]

LGTM % making it clear how last num used and num for attachment are related