[Dominators] Always recalculate postdominators when update yields different roots

Summary:
This patch makes postdominators always recalculate the tree when an update causes to change the tree roots.
As @dmgreen noticed in [[ https://reviews.llvm.org/D41298 | 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.

Reviewers: dmgreen, dberlin, davide, brzycki, grosser

Reviewed By: brzycki, grosser

Subscribers: llvm-commits, dmgreen

Differential Revision: https://reviews.llvm.org/D43140

llvm-svn: 324962

Author: kuhar

llvm/include/llvm/Support/GenericDomTreeConstruction.h

@@ -698,24 +698,20 @@ struct SemiNCAInfo {
       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 node 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 @@ bool Verify(const DomTreeT &DT, typename DomTreeT::VerificationLevel VL) {
   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");