[PATCH] D43140: [Dominators] Always recalculate postdominators when update yields different roots

Jakub Kuderski via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Feb 9 13:22:29 PST 2018


kuhar created this revision.
kuhar added reviewers: dmgreen, dberlin, davide, brzycki.
Herald added a reviewer: grosser.

This patch makes postdominators always recalculate the tree when an update causes to change the tree roots.
As @dmgreen noticed in D41298 <https://reviews.llvm.org/D41298>, the previous implementation was not conservative enough and it was possible to end up with a PostDomTree that was different than a freshly computed one.
The patch also compares postdominators with a freshly computed tree at the end of full verification to make sure we don't hit similar issues in the future.

This should (ideally) be also backported to 6.0 before the release, although I don't have any reports of this causing an observable error.  It should be safe to do it even if it's late in the release, as the change only makes the current behavior more conservative.


Repository:
  rL LLVM

https://reviews.llvm.org/D43140

Files:
  include/llvm/Support/GenericDomTreeConstruction.h


Index: include/llvm/Support/GenericDomTreeConstruction.h
===================================================================
--- include/llvm/Support/GenericDomTreeConstruction.h
+++ include/llvm/Support/GenericDomTreeConstruction.h
@@ -698,24 +698,20 @@
       return;
 
     // Recalculate the set of roots.
-    DT.Roots = FindRoots(DT, BUI);
-    for (const NodePtr R : DT.Roots) {
-      const TreeNodePtr TN = DT.getNode(R);
-      // A CFG node was selected as a tree root, but the corresponding tree node
-      // is not connected to the virtual root. This is because the incremental
-      // algorithm does not really know or use the set of roots and can make a
-      // different (implicit) decision about which nodes within an infinite loop
-      // becomes a root.
-      if (TN && !DT.isVirtualRoot(TN->getIDom())) {
-        DEBUG(dbgs() << "Root " << BlockNamePrinter(R)
-                     << " is not virtual root's child\n"
-                     << "The entire tree needs to be rebuilt\n");
-        // It should be possible to rotate the subtree instead of recalculating
-        // the whole tree, but this situation happens extremely rarely in
-        // practice.
-        CalculateFromScratch(DT, BUI);
-        return;
-      }
+    auto Roots = FindRoots(DT, BUI);
+    if (DT.Roots.size() != Roots.size() ||
+        !std::is_permutation(DT.Roots.begin(), DT.Roots.end(), Roots.begin())) {
+      // The roots chosen in the CFG have changed. This is because the
+      // incremental algorithm does not really know or use the set of roots and
+      // can make a different (implicit) decision about which nodes within an
+      // infinite loop becomes a root.
+
+      DEBUG(dbgs() << "Roots are different in updated trees\n"
+                   << "The entire tree needs to be rebuilt\n");
+      // It may be possible to update the tree without recalculating it, but
+      // we do not know yet how to do it, and it happens rarely in practise.
+      CalculateFromScratch(DT, BUI);
+      return;
     }
   }
 
@@ -1660,8 +1656,17 @@
   case DomTreeT::VerificationLevel::Basic:
     return SNCA.verifyParentProperty(DT) && SNCA.IsSameAsFreshTree(DT);
 
-  case DomTreeT::VerificationLevel::Full:
-    return SNCA.verifyParentProperty(DT) && SNCA.verifySiblingProperty(DT);
+  case DomTreeT::VerificationLevel::Full: {
+    bool FullRes
+        = SNCA.verifyParentProperty(DT) && SNCA.verifySiblingProperty(DT);
+
+    // Postdominators depend on root selection, make sure that a fresh tree
+    // looks the same.
+    if (DT.isPostDominator())
+      FullRes &= SNCA.IsSameAsFreshTree(DT);
+
+    return FullRes;
+  }
   }
 
   llvm_unreachable("Unhandled DomTree VerificationLevel");


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D43140.133678.patch
Type: text/x-patch
Size: 2725 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180209/ccc65055/attachment.bin>


More information about the llvm-commits mailing list