[Dominators] Include infinite loops in PostDominatorTree

Summary:
This patch teaches PostDominatorTree about infinite loops. It is built on top of D29705 by @dberlin which includes a very detailed motivation for this change.

What's new is that the patch also teaches the incremental updater how to deal with reverse-unreachable regions and how to properly maintain and verify tree roots. Before that, the incremental algorithm sometimes ended up preserving reverse-unreachable regions after updates that wouldn't appear in the tree if it was constructed from scratch on the same CFG.

This patch makes the following assumptions:
- A sequence of updates should produce the same tree as a recalculating it.
- Any sequence of the same updates should lead to the same tree.
- Siblings and roots are unordered.

The last two properties are essential to efficiently perform batch updates in the future.
When it comes to the first one, we can decide later that the consistency between freshly built tree and an updated one doesn't matter match, as there are many correct ways to pick roots in infinite loops, and to relax this assumption. That should enable us to recalculate postdominators less frequently.

This patch is pretty conservative when it comes to incremental updates on reverse-unreachable regions and ends up recalculating the whole tree in many cases. It should be possible to improve the performance in many cases, if we decide that it's important enough.
That being said, my experiments showed that reverse-unreachable are very rare in the IR emitted by clang when bootstrapping  clang. Here are the statistics I collected by analyzing IR between passes and after each removePredecessor call:

```
# functions:  52283
# samples:  337609
# reverse unreachable BBs:  216022
# BBs:  247840796
Percent reverse-unreachable:  0.08716159869015269 %
Max(PercRevUnreachable) in a function:  87.58620689655172 %
# > 25 % samples:  471 ( 0.1395104988314885 % samples )
... in 145 ( 0.27733680163724345 % functions )
```

Most of the reverse-unreachable regions come from invalid IR where it wouldn't be possible to construct a PostDomTree anyway.

I would like to commit this patch in the next week in order to be able to complete the work that depends on it before the end of my internship, so please don't wait long to voice your concerns :).

Reviewers: dberlin, sanjoy, grosser, brzycki, davide, chandlerc, hfinkel

Reviewed By: dberlin

Subscribers: nhaehnle, javed.absar, kparzysz, uabelho, jlebar, hiraditya, llvm-commits, dberlin, david2050

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

llvm-svn: 310940

1 files changed, 92 insertions, 67 deletions

diff --git a/llvm/unittests/IR/DominatorTreeTest.cpp b/llvm/unittests/IR/DominatorTreeTest.cpp
index ed8053ef5ab..bf5aced4928 100644
--- a/llvm/unittests/IR/DominatorTreeTest.cpp
+++ b/llvm/unittests/IR/DominatorTreeTest.cpp
@@ -327,7 +327,8 @@ TEST(DominatorTree, NonUniqueEdges) {
 }
 
 // Verify that the PDT is correctly updated in case an edge removal results
-// in a new unreachable CFG node.
+// in a new unreachable CFG node. Also make sure that the updated PDT is the
+// same as a freshly recalculated one.
 //
 // For the following input code and initial PDT:
 //
@@ -348,27 +349,18 @@ TEST(DominatorTree, NonUniqueEdges) {
 //          CFG'               PDT-updated
 //
 //           A                    Exit
-//           |                     |
-//           B                     D
+//           |                   / | \
+//           B                  C  B  D
 //           | \                   |
-//           v  \                  B
-//          /    D                  \
-//         C      \                  A
-//        |       v
+//           v  \                  A
+//          /    D
+//         C      \
+//         |       \
 // unreachable    Exit
 //
-// WARNING: PDT-updated is inconsistent with PDT-recalculated, which is
-//          constructed from CFG' when recalculating the PDT from scratch.
-//
-//         PDT-recalculated
+// Both the blocks that end with ret and with unreachable become trivial
+// PostDomTree roots, as they have no successors.
 //
-//            Exit
-//           / | \
-//          C  B  D
-//             |
-//             A
-//
-// TODO: document the wanted behavior after resolving this inconsistency.
 TEST(DominatorTree, DeletingEdgesIntroducesUnreachables) {
   StringRef ModuleString =
       "define void @f() {\n"
@@ -395,7 +387,9 @@ TEST(DominatorTree, DeletingEdgesIntroducesUnreachables) {
         BasicBlock *C = &*FI++;
         BasicBlock *D = &*FI++;
 
-        assert(PDT->dominates(PDT->getNode(D), PDT->getNode(B)));
+        ASSERT_TRUE(PDT->dominates(PDT->getNode(D), PDT->getNode(B)));
+        EXPECT_TRUE(DT->verify());
+        EXPECT_TRUE(PDT->verify());
 
         C->getTerminator()->eraseFromParent();
         new UnreachableInst(C->getContext(), C);
@@ -403,18 +397,23 @@ TEST(DominatorTree, DeletingEdgesIntroducesUnreachables) {
         DT->deleteEdge(C, B);
         PDT->deleteEdge(C, B);
 
-        EXPECT_TRUE(PDT->dominates(PDT->getNode(D), PDT->getNode(B)));
-        EXPECT_EQ(PDT->getNode(C), nullptr);
-
-        PDT->recalculate(F);
+        EXPECT_TRUE(DT->verify());
+        EXPECT_TRUE(PDT->verify());
 
         EXPECT_FALSE(PDT->dominates(PDT->getNode(D), PDT->getNode(B)));
         EXPECT_NE(PDT->getNode(C), nullptr);
+
+        DominatorTree NDT(F);
+        EXPECT_EQ(DT->compare(NDT), 0);
+
+        PostDomTree NPDT(F);
+        EXPECT_EQ(PDT->compare(NPDT), 0);
       });
 }
 
 // Verify that the PDT is correctly updated in case an edge removal results
-// in an infinite loop.
+// in an infinite loop. Also make sure that the updated PDT is the
+// same as a freshly recalculated one.
 //
 // Test case:
 //
@@ -434,27 +433,26 @@ TEST(DominatorTree, DeletingEdgesIntroducesUnreachables) {
 // After deleting the edge C->B, C is part of an infinite reverse-unreachable
 // loop:
 //
-//          CFG'                   PDT'
+//          CFG'                  PDT'
 //
 //           A                    Exit
-//           |                     |
-//           B                     D
+//           |                   / | \
+//           B                  C  B  D
 //           | \                   |
-//           v  \                  B
-//          /    D                  \
-//         C      \                  A
+//           v  \                  A
+//          /    D
+//         C      \
 //        / \      v
 //       ^  v      Exit
 //        \_/
 //
-// In PDT, D post-dominates B. We verify that this post-dominance
-// relation is preserved _after_ deleting the edge C->B from CFG.
-//
-// As C now becomes reverse-unreachable, it is not anymore part of the
-// PDT. We also verify this property.
+// As C now becomes reverse-unreachable, it forms a new non-trivial root and
+// gets connected to the virtual exit.
+// D does not postdominate B anymore, because there are two forward paths from
+// B to the virtual exit:
+//  - B -> C -> VirtualExit
+//  - B -> D -> VirtualExit.
 //
-// TODO: Can we change the PDT definition such that C remains part of the
-//       CFG?
 TEST(DominatorTree, DeletingEdgesIntroducesInfiniteLoop) {
   StringRef ModuleString =
       "define void @f() {\n"
@@ -483,20 +481,25 @@ TEST(DominatorTree, DeletingEdgesIntroducesInfiniteLoop) {
         BasicBlock *C = &*FI++;
         BasicBlock *D = &*FI++;
 
-        assert(PDT->dominates(PDT->getNode(D), PDT->getNode(B)));
+        ASSERT_TRUE(PDT->dominates(PDT->getNode(D), PDT->getNode(B)));
+        EXPECT_TRUE(DT->verify());
+        EXPECT_TRUE(PDT->verify());
 
         auto SwitchC = cast<SwitchInst>(C->getTerminator());
         SwitchC->removeCase(SwitchC->case_begin());
         DT->deleteEdge(C, B);
+        EXPECT_TRUE(DT->verify());
         PDT->deleteEdge(C, B);
+        EXPECT_TRUE(PDT->verify());
 
-        EXPECT_TRUE(PDT->dominates(PDT->getNode(D), PDT->getNode(B)));
-        EXPECT_EQ(PDT->getNode(C), nullptr);
+        EXPECT_FALSE(PDT->dominates(PDT->getNode(D), PDT->getNode(B)));
+        EXPECT_NE(PDT->getNode(C), nullptr);
 
-        PDT->recalculate(F);
+        DominatorTree NDT(F);
+        EXPECT_EQ(DT->compare(NDT), 0);
 
-        EXPECT_TRUE(PDT->dominates(PDT->getNode(D), PDT->getNode(B)));
-        EXPECT_EQ(PDT->getNode(C), nullptr);
+        PostDomTree NPDT(F);
+        EXPECT_EQ(PDT->compare(NPDT), 0);
       });
 }
 
@@ -509,9 +512,9 @@ TEST(DominatorTree, DeletingEdgesIntroducesInfiniteLoop) {
 //
 //           A                    Exit
 //           |                   / | \
-//           B--                C  B  D
-//           |  \                  |
-//           v   \                 A
+//           B--               C2  B  D
+//           |  \              /   |
+//           v   \            C    A
 //          /     D
 //         C--C2   \
 //        / \  \    v
@@ -521,27 +524,22 @@ TEST(DominatorTree, DeletingEdgesIntroducesInfiniteLoop) {
 // After deleting the edge C->E, C is part of an infinite reverse-unreachable
 // loop:
 //
-//          CFG'                   PDT'
+//          CFG'                  PDT'
 //
 //           A                    Exit
-//           |                     |
-//           B                     D
+//           |                   / | \
+//           B                  C  B  D
 //           | \                   |
-//           v  \                  B
-//          /    D                  \
-//         C      \                  A
+//           v  \                  A
+//          /    D
+//         C      \
 //        / \      v
 //       ^  v      Exit
 //        \_/
 //
-// In PDT, D does not post-dominate B. After the edge C->E is removed, a new
-// post-dominance relation is introduced.
-//
-// As C now becomes reverse-unreachable, it is not anymore part of the
-// PDT. We also verify this property.
+// In PDT, D does not post-dominate B. After the edge C -> C2 is removed,
+// C becomes a new nontrivial PDT root.
 //
-// TODO: Can we change the PDT definition such that C remains part of the
-//       CFG, at best without loosing the dominance relation D postdom B.
 TEST(DominatorTree, DeletingEdgesIntroducesInfiniteLoop2) {
   StringRef ModuleString =
       "define void @f() {\n"
@@ -573,24 +571,30 @@ TEST(DominatorTree, DeletingEdgesIntroducesInfiniteLoop2) {
         BasicBlock *C2 = &*FI++;
         BasicBlock *D = &*FI++;
 
+        EXPECT_TRUE(DT->verify());
+        EXPECT_TRUE(PDT->verify());
+
         auto SwitchC = cast<SwitchInst>(C->getTerminator());
         SwitchC->removeCase(SwitchC->case_begin());
         DT->deleteEdge(C, C2);
         PDT->deleteEdge(C, C2);
-        C2->eraseFromParent();
+        C2->removeFromParent();
 
         EXPECT_EQ(DT->getNode(C2), nullptr);
         PDT->eraseNode(C2);
+        delete C2;
+
+        EXPECT_TRUE(DT->verify());
+        EXPECT_TRUE(PDT->verify());
 
-        EXPECT_TRUE(PDT->dominates(PDT->getNode(D), PDT->getNode(B)));
-        EXPECT_EQ(PDT->getNode(C), nullptr);
-        EXPECT_EQ(PDT->getNode(C2), nullptr);
+        EXPECT_FALSE(PDT->dominates(PDT->getNode(D), PDT->getNode(B)));
+        EXPECT_NE(PDT->getNode(C), nullptr);
 
-        PDT->recalculate(F);
+        DominatorTree NDT(F);
+        EXPECT_EQ(DT->compare(NDT), 0);
 
-        EXPECT_TRUE(PDT->dominates(PDT->getNode(D), PDT->getNode(B)));
-        EXPECT_EQ(PDT->getNode(C), nullptr);
-        EXPECT_EQ(PDT->getNode(C2), nullptr);
+        PostDomTree NPDT(F);
+        EXPECT_EQ(PDT->compare(NPDT), 0);
       });
 }
 
@@ -686,6 +690,27 @@ TEST(DominatorTree, InsertUnreachable) {
   }
 }
 
+TEST(DominatorTree, InsertFromUnreachable) {
+  CFGHolder Holder;
+  std::vector<CFGBuilder::Arc> Arcs = {{"1", "2"}, {"2", "3"}, {"3", "4"}};
+
+  std::vector<CFGBuilder::Update> Updates = {{Insert, {"3", "5"}}};
+  CFGBuilder B(Holder.F, Arcs, Updates);
+  PostDomTree PDT(*Holder.F);
+  EXPECT_TRUE(PDT.verify());
+
+  Optional<CFGBuilder::Update> LastUpdate = B.applyUpdate();
+  EXPECT_TRUE(LastUpdate);
+
+  EXPECT_EQ(LastUpdate->Action, Insert);
+  BasicBlock *From = B.getOrAddBlock(LastUpdate->Edge.From);
+  BasicBlock *To = B.getOrAddBlock(LastUpdate->Edge.To);
+  PDT.insertEdge(From, To);
+  EXPECT_TRUE(PDT.verify());
+  EXPECT_TRUE(PDT.getRoots().size() == 2);
+  EXPECT_NE(PDT.getNode(B.getOrAddBlock("5")), nullptr);
+}
+
 TEST(DominatorTree, InsertMixed) {
   CFGHolder Holder;
   std::vector<CFGBuilder::Arc> Arcs = {