DomTree: Make PostDomTree immune to block successors swap

This is another fix for the bug 46098 where PostDominatorTree
is unexpectedly changed by InstCombine's branch swapping
transformation.
This patch fixes PostDomTree builder. While looking for
the furthest away node in a reverse unreachable subgraph
this patch runs DFS with successors in their function order.
This order is indifferent to the order of successors, so is
the furthest away node.

Reviewers: kuhar, nikic, lebedev.ri
Differential Revision: https://reviews.llvm.org/D84763

diff --git a/llvm/include/llvm/Support/GenericDomTreeConstruction.h b/llvm/include/llvm/Support/GenericDomTreeConstruction.h
index 6a9d38b..3c85caf 100644 (file)
--- a/llvm/include/llvm/Support/GenericDomTreeConstruction.h
+++ b/llvm/include/llvm/Support/GenericDomTreeConstruction.h
@@ -151,6 +151,8 @@ struct SemiNCAInfo {
     }
   };
 
+  using NodeOrderMap = DenseMap<NodePtr, unsigned>;
+
   // Custom DFS implementation which can skip nodes based on a provided
   // predicate. It also collects ReverseChildren so that we don't have to spend
   // time getting predecessors in SemiNCA.
@@ -158,9 +160,13 @@ struct SemiNCAInfo {
   // If IsReverse is set to true, the DFS walk will be performed backwards
   // relative to IsPostDom -- using reverse edges for dominators and forward
   // edges for postdominators.
+  //
+  // If SuccOrder is specified then in this order the DFS traverses the children
+  // otherwise the order is implied by the results of getChildren().
   template <bool IsReverse = false, typename DescendCondition>
   unsigned runDFS(NodePtr V, unsigned LastNum, DescendCondition Condition,
-                  unsigned AttachToNum) {
+                  unsigned AttachToNum,
+                  const NodeOrderMap *SuccOrder = nullptr) {
     assert(V);
     SmallVector<NodePtr, 64> WorkList = {V};
     if (NodeToInfo.count(V) != 0) NodeToInfo[V].Parent = AttachToNum;
@@ -176,7 +182,14 @@ struct SemiNCAInfo {
       NumToNode.push_back(BB);
 
       constexpr bool Direction = IsReverse != IsPostDom;  // XOR.
-      for (const NodePtr Succ : getChildren<Direction>(BB, BatchUpdates)) {
+      auto Successors = getChildren<Direction>(BB, BatchUpdates);
+      if (SuccOrder && Successors.size() > 1)
+        llvm::sort(
+            Successors.begin(), Successors.end(), [=](NodePtr A, NodePtr B) {
+              return SuccOrder->find(A)->second < SuccOrder->find(B)->second;
+            });
+
+      for (const NodePtr Succ : Successors) {
         const auto SIT = NodeToInfo.find(Succ);
         // Don't visit nodes more than once but remember to collect
         // ReverseChildren.
@@ -372,6 +385,34 @@ struct SemiNCAInfo {
     // nodes.
     if (Total + 1 != Num) {
       HasNonTrivialRoots = true;
+
+      // SuccOrder is the order of blocks in the function. It is needed to make
+      // the calculation of the FurthestAway node and the whole PostDomTree
+      // immune to swap successors transformation (e.g. canonicalizing branch
+      // predicates). SuccOrder is initialized lazily only for successors of
+      // reverse unreachable nodes.
+      Optional<NodeOrderMap> SuccOrder;
+      auto InitSuccOrderOnce = [&]() {
+        SuccOrder = NodeOrderMap();
+        for (const auto Node : nodes(DT.Parent))
+          if (SNCA.NodeToInfo.count(Node) == 0)
+            for (const auto Succ : getChildren<false>(Node, SNCA.BatchUpdates))
+              SuccOrder->try_emplace(Succ, 0);
+
+        // Add mapping for all entries of SuccOrder.
+        unsigned NodeNum = 0;
+        for (const auto Node : nodes(DT.Parent)) {
+          ++NodeNum;
+          auto Order = SuccOrder->find(Node);
+          if (Order != SuccOrder->end()) {
+            assert(Order->second == 0);
+            Order->second = NodeNum;
+            LLVM_DEBUG(dbgs() << "\t\t\tSuccOrder " << NodeNum << ": "
+                              << Node->getName() << "\n");
+          }
+        }
+      };
+
       // Make another DFS pass over all other nodes to find the
       // reverse-unreachable blocks, and find the furthest paths we'll be able
       // to make.
@@ -396,7 +437,12 @@ struct SemiNCAInfo {
           // expensive and does not always lead to a minimal set of roots.
           LLVM_DEBUG(dbgs() << "\t\t\tRunning forward DFS\n");
 
-          const unsigned NewNum = SNCA.runDFS<true>(I, Num, AlwaysDescend, Num);
+          if (!SuccOrder)
+            InitSuccOrderOnce();
+          assert(SuccOrder);
+
+          const unsigned NewNum =
+              SNCA.runDFS<true>(I, Num, AlwaysDescend, Num, &*SuccOrder);
           const NodePtr FurthestAway = SNCA.NumToNode[NewNum];
           LLVM_DEBUG(dbgs() << "\t\t\tFound a new furthest away node "
                             << "(non-trivial root): "

diff --git a/llvm/test/Transforms/InstCombine/infinite-loop-postdom.ll b/llvm/test/Transforms/InstCombine/infinite-loop-postdom.ll
new file mode 100644 (file)
index 0000000..a6ce1fa
--- /dev/null
+++ b/llvm/test/Transforms/InstCombine/infinite-loop-postdom.ll
@@ -0,0 +1,222 @@
+; RUN: opt %s -disable-output -branch-prob -instcombine -block-freq -verify-dom-info\r
+; RUN: opt %s -postdomtree -analyze | FileCheck --check-prefixes=CHECK-POSTDOM %s\r
+; RUN: opt %s -passes='print<postdomtree>' 2>&1 | FileCheck --check-prefixes=CHECK-POSTDOM %s\r
+\r
+; Demonstrate that Predicate Canonicalization (InstCombine) does not invalidate PostDomTree\r
+; if the basic block is post-dom unreachable.\r
+\r
+define void @test1(i24 %a, i24 %b) {\r
+entry:\r
+  br label %LOOP\r
+\r
+LOOP:\r
+  %f = icmp uge i24 %a, %b\r
+  br i1 %f, label %B1, label %B2\r
+\r
+B1:\r
+  %x = add i24 %a, %b\r
+  br label %B2\r
+\r
+B2:\r
+  br label %LOOP\r
+}\r
+\r
+; The same as @test1 except the LOOP condition canonicalized (as by instcombine).\r
+define void @test1-canonicalized(i24 %a, i24 %b) {\r
+entry:\r
+  br label %LOOP\r
+\r
+LOOP:\r
+  %f.not = icmp ult i24 %a, %b\r
+  br i1 %f.not, label %B2, label %B1\r
+\r
+B1:\r
+  %x = add i24 %a, %b\r
+  br label %B2\r
+\r
+B2:\r
+  br label %LOOP\r
+}\r
+\r
+; The same as @test1 but different order of B1 and B2 in the function.\r
+; The different order makes PostDomTree different in presense of postdom\r
+; unreachable blocks.\r
+define void @test2(i24 %a, i24 %b) {\r
+entry:\r
+  br label %LOOP\r
+\r
+LOOP:\r
+  %f = icmp uge i24 %a, %b\r
+  br i1 %f, label %B1, label %B2\r
+\r
+B2:\r
+  br label %LOOP\r
+\r
+B1:\r
+  %x = add i24 %a, %b\r
+  br label %B2\r
+}\r
+\r
+; The same as @test2 except the LOOP condition canonicalized (as by instcombine).\r
+define void @test2-canonicalized(i24 %a, i24 %b) {\r
+entry:\r
+  br label %LOOP\r
+\r
+LOOP:\r
+  %f.not = icmp ult i24 %a, %b\r
+  br i1 %f.not, label %B2, label %B1\r
+\r
+B2:\r
+  br label %LOOP\r
+\r
+B1:\r
+  %x = add i24 %a, %b\r
+  br label %B2\r
+}\r
+\r
+; Two reverse unreachable subgraphs with RU1* and RU2* basic blocks respectively.\r
+define void @test3(i24 %a, i24 %b, i32 %flag) {\r
+entry:\r
+  switch i32 %flag, label %EXIT [\r
+    i32 1, label %RU1\r
+    i32 2, label %RU2\r
+    i32 3, label %RU2_B1\r
+  ]\r
+\r
+RU1:\r
+  %f = icmp uge i24 %a, %b\r
+  br label %RU1_LOOP\r
+\r
+RU1_LOOP:\r
+  br i1 %f, label %RU1_B1, label %RU1_B2\r
+\r
+RU1_B1:\r
+  %x = add i24 %a, %b\r
+  br label %RU1_B2\r
+\r
+RU1_B2:\r
+  br label %RU1_LOOP\r
+\r
+RU2:\r
+  %f2 = icmp uge i24 %a, %b\r
+  br i1 %f2, label %RU2_B1, label %RU2_B2\r
+\r
+RU2_B1:\r
+  br label %RU2_B2\r
+\r
+RU2_B2:\r
+  br label %RU2_B1\r
+\r
+EXIT:\r
+  ret void\r
+}\r
+\r
+; The same as @test3 except the icmp conditions are canonicalized (as by instcombine).\r
+define void @test3-canonicalized(i24 %a, i24 %b, i32 %flag) {\r
+entry:\r
+  switch i32 %flag, label %EXIT [\r
+    i32 1, label %RU1\r
+    i32 2, label %RU2\r
+    i32 3, label %RU2_B1\r
+  ]\r
+\r
+RU1:\r
+  %f.not = icmp ult i24 %a, %b\r
+  br label %RU1_LOOP\r
+\r
+RU1_LOOP:\r
+  br i1 %f.not, label %RU1_B2, label %RU1_B1\r
+\r
+RU1_B1:\r
+  %x = add i24 %a, %b\r
+  br label %RU1_B2\r
+\r
+RU1_B2:\r
+  br label %RU1_LOOP\r
+\r
+RU2:\r
+  %f2.not = icmp ult i24 %a, %b\r
+  br i1 %f2.not, label %RU2_B2, label %RU2_B1\r
+\r
+RU2_B1:\r
+  br label %RU2_B2\r
+\r
+RU2_B2:\r
+  br label %RU2_B1\r
+\r
+EXIT:\r
+  ret void\r
+}\r
+\r
+; PostDomTrees of @test1(), @test2() and @test3() are different.\r
+; PostDomTrees of @testX() and @testX-canonicalize() are the same.\r
+\r
+; CHECK-POSTDOM-LABEL: test1\r
+; CHECK-POSTDOM-NEXT: =============================--------------------------------\r
+; CHECK-POSTDOM-NEXT: Inorder PostDominator Tree: DFSNumbers invalid: 0 slow queries.\r
+; CHECK-POSTDOM-NEXT:   [1]  <<exit node>>\r
+; CHECK-POSTDOM-NEXT:     [2] %B1\r
+; CHECK-POSTDOM-NEXT:       [3] %LOOP\r
+; CHECK-POSTDOM-NEXT:         [4] %entry\r
+; CHECK-POSTDOM-NEXT:         [4] %B2\r
+; CHECK-POSTDOM-NEXT: Roots: %B1\r
+\r
+; CHECK-POSTDOM-LABEL: test1-canonicalized\r
+; CHECK-POSTDOM-NEXT: =============================--------------------------------\r
+; CHECK-POSTDOM-NEXT: Inorder PostDominator Tree: DFSNumbers invalid: 0 slow queries.\r
+; CHECK-POSTDOM-NEXT:   [1]  <<exit node>>\r
+; CHECK-POSTDOM-NEXT:     [2] %B1\r
+; CHECK-POSTDOM-NEXT:       [3] %LOOP\r
+; CHECK-POSTDOM-NEXT:         [4] %entry\r
+; CHECK-POSTDOM-NEXT:         [4] %B2\r
+; CHECK-POSTDOM-NEXT: Roots: %B1
+
+; CHECK-POSTDOM-LABEL: test2
+; CHECK-POSTDOM-NEXT: =============================--------------------------------
+; CHECK-POSTDOM-NEXT: Inorder PostDominator Tree: DFSNumbers invalid: 0 slow queries.
+; CHECK-POSTDOM-NEXT:   [1]  <<exit node>>
+; CHECK-POSTDOM-NEXT:     [2] %B2
+; CHECK-POSTDOM-NEXT:       [3] %LOOP
+; CHECK-POSTDOM-NEXT:         [4] %entry
+; CHECK-POSTDOM-NEXT:       [3] %B1
+; CHECK-POSTDOM-NEXT: Roots: %B2
+
+; CHECK-POSTDOM-LABEL: test2-canonicalized
+; CHECK-POSTDOM-NEXT: =============================--------------------------------
+; CHECK-POSTDOM-NEXT: Inorder PostDominator Tree: DFSNumbers invalid: 0 slow queries.
+; CHECK-POSTDOM-NEXT:   [1]  <<exit node>>
+; CHECK-POSTDOM-NEXT:     [2] %B2
+; CHECK-POSTDOM-NEXT:       [3] %LOOP
+; CHECK-POSTDOM-NEXT:         [4] %entry
+; CHECK-POSTDOM-NEXT:       [3] %B1
+; CHECK-POSTDOM-NEXT: Roots: %B2
+
+; CHECK-POSTDOM-LABEL: test3
+; CHECK-POSTDOM-NEXT:=============================--------------------------------
+; CHECK-POSTDOM-NEXT:Inorder PostDominator Tree: DFSNumbers invalid: 0 slow queries.
+; CHECK-POSTDOM-NEXT:  [1]  <<exit node>>
+; CHECK-POSTDOM-NEXT:    [2] %EXIT
+; CHECK-POSTDOM-NEXT:    [2] %entry
+; CHECK-POSTDOM-NEXT:    [2] %RU1_B1
+; CHECK-POSTDOM-NEXT:      [3] %RU1_LOOP
+; CHECK-POSTDOM-NEXT:        [4] %RU1
+; CHECK-POSTDOM-NEXT:        [4] %RU1_B2
+; CHECK-POSTDOM-NEXT:    [2] %RU2_B1
+; CHECK-POSTDOM-NEXT:      [3] %RU2
+; CHECK-POSTDOM-NEXT:      [3] %RU2_B2
+; CHECK-POSTDOM-NEXT:Roots: %EXIT %RU1_B1 %RU2_B1
+
+; CHECK-POSTDOM-LABEL: test3-canonicalized
+; CHECK-POSTDOM-NEXT:=============================--------------------------------
+; CHECK-POSTDOM-NEXT:Inorder PostDominator Tree: DFSNumbers invalid: 0 slow queries.
+; CHECK-POSTDOM-NEXT:  [1]  <<exit node>>
+; CHECK-POSTDOM-NEXT:    [2] %EXIT
+; CHECK-POSTDOM-NEXT:    [2] %entry
+; CHECK-POSTDOM-NEXT:    [2] %RU1_B1
+; CHECK-POSTDOM-NEXT:      [3] %RU1_LOOP
+; CHECK-POSTDOM-NEXT:        [4] %RU1
+; CHECK-POSTDOM-NEXT:        [4] %RU1_B2
+; CHECK-POSTDOM-NEXT:    [2] %RU2_B1
+; CHECK-POSTDOM-NEXT:      [3] %RU2
+; CHECK-POSTDOM-NEXT:      [3] %RU2_B2
+; CHECK-POSTDOM-NEXT:Roots: %EXIT %RU1_B1 %RU2_B1