[Dominators] Teach LoopDeletion to use the new incremental API

[kuhar]

Summary

This patch makes LoopDeletion use the incremental DominatorTree API.

We modify LoopDeletion to perform the deletion in 5 steps:

1. Create a new dummy edge from the preheader to the exit, by adding a conditional branch.
2. Inform the DomTree about the new edge.
3. Remove the conditional branch and replace it with an unconditional edge to the exit. This removes the edge to the loop header, making it unreachable.
4. Inform the DomTree about the deleted edge.
5. Remove the unreachable block from the function.

Creating the dummy conditional branch is necessary to perform incremental DomTree update.
We should consider using the batch updater when it's ready.

[kuhar]

Add a comment explaining the special logic here.

[kuhar]

Make the edited for loop more concise.

[grosser]

Nice work! This mostly looks good to me, but I have some minor question. I mark this as request changes to be notified by Phabricator in case you reply. It seems some of the email notifications don't really work at the moment.

lib/Transforms/Scalar/LoopDeletion.cpp

303

preheader

308

I think it would be great to document this behavior in deleteEdge() and reference this behavior there! Also, do we happen to have a test case that documents this bulk removal of nodes in DT. I think this is an interesting, but not totally expected use of the deleteEdge API and it would be great if this has good test coverage.

Thanks again for working on this,

[kuhar]

I think that I came up better guidelines on chaining updates. I included explanations for that in the comments and I added two new unittests illustrating them.

I misunderstood how LoopDeletion changes CFG and the previous explanation was incorrect because of that. Please tell me if this one is clear enough.

[kuhar]

Zhendong and Qirun were able to generate a CFG that broke the previous update chained logic. It turns out, that it's not legal to chain a deletion and an insertion when there's irreducible control flow that appears in the temporarily deleted subtree. I added a regression test for that.

I think that this shows that the current incremental API is tricky to use when CFG and DomTree updates are not performed in lockstep, and we *really* need a proper batch update API.

[kuhar]

@grosser, @dberlin, @davide

Ping.
(This patch doesn't depend on the postdominator changes in review or on the batch updater.)

[• dberlin]

This looks fine to me.
I think it may make sense to try to add a few simple cfg drawings to show what machinations are really happening here, but if others undersrtand them just from the text descriptions, awesome.

[grosser]

Hi @kuhar,

thank you for updating the comment. I found a couple of minor typos. The code likely looks good, but I don't see any reference to the irreducible control flow issues you mentioned. I assume you are now connecting the loop header to its exit to avoid the PDT update issue before. It might make sense to briefly explain why first deleting and then adding a new edge will break.

In any case, these are at most minor issues. Feel free to commit.

Best,
Tobias

include/llvm/Support/GenericDomTree.h

492

That looks good to me.

lib/Transforms/Scalar/LoopDeletion.cpp

248

THE dominator tree update

260

Could you maybe move the " DT.insertEdge(Preheader, ExitBlock);" this makes clear that the DT update comes right after the CFG change. Now it is hidden after the PHI update.

297

dominator

unittests/IR/DominatorTreeTest.cpp

538

Remove "a"

539

Remove "the"
Remove "of them"

[kuhar]

Address Daniel's and Tobias' comments.

[grosser]

Amazing. Now this reads really nice.

unittests/IR/DominatorTreeTest.cpp

538

Still an "a" too much.

include/llvm/Support/GenericDomTree.h

void insertEdge(NodeT *From, NodeT *To) {
     assert(To);
     assert(From->getParent() == Parent);
     assert(To->getParent() == Parent);
     DomTreeBuilder::InsertEdge(*this, From, To);
   }
 
   /// Inform the dominator tree about a CFG edge deletion and update the tree.
   ///
-  /// This function has to be called just after making the update
-  /// on the actual CFG. There cannot be any other updates that the dominator
-  /// tree doesn't know about. The only exception is when the deletion that the
-  /// tree is informed about makes some (dominator) subtree unreachable -- in
-  /// this case, it is fine to perform deletions within this subtree.
+  /// This function has to be called just after making the update on the actual
+  /// CFG. An internal functions checks if the edge doesn't exist in the CFG in
+  /// DEBUG mode. There cannot be any other updates that the
+  /// dominator tree doesn't know about.
+  ///
+  /// However, it is fine to perform multiple CFG deletions that make different
+  /// subtrees forward-unreachable and to inform the DomTree about them all at
+  /// the same time, as the incremental algorithm doesn't walk the tree above
+  /// the NearestCommonDominator of a deleted edge
   ///
   /// Note that for postdominators it automatically takes care of deleting
   /// a reverse edge internally (so there's no need to swap the parameters).
   ///
   void deleteEdge(NodeT *From, NodeT *To) {
     assert(From);
     assert(To);
     assert(From->getParent() == Parent);

lib/Transforms/Scalar/LoopDeletion.cpp

static void deleteDeadLoop(Loop *L, DominatorTree &DT, ScalarEvolution &SE,
 
   // Tell ScalarEvolution that the loop is deleted. Do this before
   // deleting the loop so that ScalarEvolution can look at the loop
   // to determine what it needs to clean up.
   SE.forgetLoop(L);
 
   auto *ExitBlock = L->getUniqueExitBlock();
   assert(ExitBlock && "Should have a unique exit block!");
-
   assert(L->hasDedicatedExits() && "Loop should have dedicated exits!");
 
-  // Connect the preheader directly to the exit block.
+  auto *OldBr = dyn_cast<BranchInst>(Preheader->getTerminator());
+  assert(OldBr && "Preheader must end with a branch");
+  assert(OldBr->isUnconditional() && "Preheader must have a single successor");
+  // Connect the preheader to the exit block. Keep the old edge to the header
+  // around to perform dominator tree update in two separate steps -- insertion
+  // of the edge preheader->exit and deletion of the edge preheader->header.
+  //
   // Even when the loop is never executed, we cannot remove the edge from the
   // source block to the exit block. Consider the case where the unexecuted loop
   // branches back to an outer loop. If we deleted the loop and removed the edge
   // coming to this inner loop, this will break the outer loop structure (by
   // deleting the backedge of the outer loop). If the outer loop is indeed a
   // non-loop, it will be deleted in a future iteration of loop deletion pass.
-  Preheader->getTerminator()->replaceUsesOfWith(L->getHeader(), ExitBlock);
+  IRBuilder<> Builder(OldBr);
+  Builder.CreateCondBr(Builder.getFalse(), L->getHeader(), ExitBlock);
+  // Remove the old branch. The conditional branch becomes a new terminator.
+  OldBr->eraseFromParent();
 
   // Rewrite phis in the exit block to get their inputs from the Preheader
   // instead of the exiting block.
   BasicBlock::iterator BI = ExitBlock->begin();
   while (PHINode *P = dyn_cast<PHINode>(BI)) {
     // Set the zero'th element of Phi to be from the preheader and remove all
     // other incoming values. Given the loop has dedicated exits, all other
     // incoming values must be from the exiting blocks.
for (unsigned i = 0, e = P->getNumIncomingValues() - 1; i != e; ++i)
       P->removeIncomingValue(e-i, false);
 
     assert((P->getNumIncomingValues() == 1 &&
             P->getIncomingBlock(PredIndex) == Preheader) &&
            "Should have exactly one value and that's from the preheader!");
     ++BI;
   }
 
-  // Update the dominator tree and remove the instructions and blocks that will
-  // be deleted from the reference counting scheme.
-  SmallVector<DomTreeNode*, 8> ChildNodes;
-  for (Loop::block_iterator LI = L->block_begin(), LE = L->block_end();
-       LI != LE; ++LI) {
-    // Move all of the block's children to be children of the Preheader, which
-    // allows us to remove the domtree entry for the block.
-    ChildNodes.insert(ChildNodes.begin(), DT[*LI]->begin(), DT[*LI]->end());
-    for (DomTreeNode *ChildNode : ChildNodes) {
-      DT.changeImmediateDominator(ChildNode, DT[Preheader]);
-    }
+  // Update the dominator tree by informing it about the new edge from the
+  // preheader to the exit.
+  DT.insertEdge(Preheader, ExitBlock);
+
+  // Disconnect the loop body by branching directly to its exit.
+  Builder.SetInsertPoint(Preheader->getTerminator());
+  Builder.CreateBr(ExitBlock);
+  // Remove the old branch.
+  Preheader->getTerminator()->eraseFromParent();
 
-    ChildNodes.clear();
-    DT.eraseNode(*LI);
+  // Inform the domiantor tree about the removed edge.
+  DT.deleteEdge(Preheader, L->getHeader());
 
-    // Remove the block from the reference counting scheme, so that we can
-    // delete it freely later.
-    (*LI)->dropAllReferences();
-  }
+  for (auto *Block : L->blocks())
+    Block->dropAllReferences();
 
   // Erase the instructions and the blocks without having to worry
   // about ordering because we already dropped the references.
   // NOTE: This iteration is safe because erasing the block does not remove its
   // entry from the loop's block list.  We do that in the next section.
   for (Loop::block_iterator LI = L->block_begin(), LE = L->block_end();
        LI != LE; ++LI)
     (*LI)->eraseFromParent();
 
   // Finally, the blocks from loopinfo.  This has to happen late because
   // otherwise our loop iterators won't work.
 
   SmallPtrSet<BasicBlock *, 8> blocks;

test/Transforms/LoopDeletion/2017-07-11-incremental-dt.ll

+; RUN: opt < %s -loop-deletion -S
+; RUN: opt < %s -loop-deletion -analyze -domtree 2>&1 | FileCheck -check-prefix=DT %s
+; RUN: opt < %s -loop-deletion -analyze -verify-dom-info
+
+; CHECK: for.body
+; CHECK-NOT: for.cond1
+
+; Verify only the important parts of the DomTree.
+; DT: [1] %entry
+; DT:   [2] %for.cond
+; DT:     [3] %lbl63A679E5
+; DT:     [3] %for.cond9
+; DT:     [3] %lbl64774A9B
+; DT:     [3] %for.body
+; DT:       [4] %for.cond3.loopexit
+
+define i32 @fn1() {
+entry:
+  br label %for.cond
+
+for.cond:                                         ; preds = %entry
+  br i1 undef, label %lbl63A679E5, label %for.body
+
+for.body:                                         ; preds = %for.cond
+  br label %for.cond1
+
+for.cond1:                                        ; preds = %for.cond1, %for.body
+  br i1 undef, label %for.cond1, label %for.cond3.loopexit
+
+for.cond3.loopexit:                               ; preds = %for.cond1
+  br label %for.cond3
+
+for.cond3:                                        ; preds = %for.cond9, %for.cond3.loopexit
+  br i1 undef, label %for.body4, label %for.cond17
+
+for.body4:                                        ; preds = %for.cond3
+  br label %for.cond5
+
+for.cond5:                                        ; preds = %lbl63A679E5, %for.body4
+  br label %for.cond9
+
+lbl63A679E5:                                      ; preds = %for.cond
+  br label %for.cond5
+
+for.cond9:                                        ; preds = %for.end14.split, %for.cond5
+  br i1 undef, label %for.cond3, label %lbl64774A9B
+
+lbl64774A9B:                                      ; preds = %for.cond17, %for.cond9
+  br label %for.end14.split
+
+for.end14.split:                                  ; preds = %lbl64774A9B
+  br label %for.cond9
+
+for.cond17:                                       ; preds = %for.cond3
+  br label %lbl64774A9B
+}

unittests/IR/DominatorTreeTest.cpp

while ((LastUpdate = B.applyUpdate())) {
     BasicBlock *To = B.getOrAddBlock(LastUpdate->Edge.To);
     DT.deleteEdge(From, To);
     EXPECT_TRUE(DT.verify());
     PDT.deleteEdge(From, To);
     EXPECT_TRUE(PDT.verify());
   }
 }
 
+TEST(DominatorTree, DeletionsInSubtrees) {
+  CFGHolder Holder;
+  std::vector<CFGBuilder::Arc> Arcs = {{"0", "1"}, {"1", "2"}, {"1", "3"},
+                                       {"1", "6"}, {"3", "4"}, {"2", "5"},
+                                       {"5", "2"}};
+
+  // It is possible to perform a multiple deletions and
+  // inform the DominatorTree about them of them at the same time, if the all of
+  // the deletions happen in different subtrees.
+  std::vector<CFGBuilder::Update> Updates = {{Delete, {"1", "2"}},
+                                             {Delete, {"1", "3"}}};
+  CFGBuilder B(Holder.F, Arcs, Updates);
+  DominatorTree DT(*Holder.F);
+  EXPECT_TRUE(DT.verify());
+
+  Optional<CFGBuilder::Update> LastUpdate;
+  while ((LastUpdate = B.applyUpdate()))
+    ;
+
+  DT.deleteEdge(B.getOrAddBlock("1"), B.getOrAddBlock("2"));
+  DT.deleteEdge(B.getOrAddBlock("1"), B.getOrAddBlock("3"));
+
+  EXPECT_TRUE(DT.verify());
+  EXPECT_EQ(DT.getNode(B.getOrAddBlock("2")), nullptr);
+  EXPECT_EQ(DT.getNode(B.getOrAddBlock("3")), nullptr);
+  EXPECT_EQ(DT.getNode(B.getOrAddBlock("4")), nullptr);
+  EXPECT_EQ(DT.getNode(B.getOrAddBlock("5")), nullptr);
+  EXPECT_NE(DT.getNode(B.getOrAddBlock("6")), nullptr);
+}
+
 TEST(DominatorTree, InsertDelete) {
   std::vector<CFGBuilder::Arc> Arcs = {
       {"1", "2"}, {"2", "3"}, {"3", "4"},  {"4", "5"},  {"5", "6"},  {"5", "7"},
       {"3", "8"}, {"8", "9"}, {"9", "10"}, {"8", "11"}, {"11", "12"}};
 
   std::vector<CFGBuilder::Update> Updates = {
       {Insert, {"2", "4"}},  {Insert, {"12", "10"}}, {Insert, {"10", "9"}},
       {Insert, {"7", "6"}},  {Insert, {"7", "5"}},   {Delete, {"3", "8"}},