[PATCH] D28114: [StructurizeCfg] Update dominator info.

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff updated this revision to Diff 83602.
sepavloff added a comment.

Updated patch

As pointed out by @hfinkel and @dberlin, this code won't work with
post-dominator trees. It is possible to extend the fix to support
such trees also, but it is more complex. In the case of several exit
nodes we need to specify which root is replaced, and if the root is
virtual, this operation makes no sense. So the operation is now
confined to forward dominator trees by putting appropriate assertion
checks. There are some functions in DominatorTreeBase that impose
similar restriction.

Adding node as a new root is now implemented by a separate function
instead of reusing addNewBlock. It should prevent from accidental
root changes as a user have to explicitly specify the intent.

A small piece of unit test was added that is similar to the operation
made by StructurizeCfg pass.


https://reviews.llvm.org/D28114

Files:
  include/llvm/Support/GenericDomTree.h
  lib/Transforms/Scalar/StructurizeCFG.cpp
  test/Transforms/StructurizeCFG/no-branch-to-entry.ll
  unittests/IR/DominatorTreeTest.cpp


Index: unittests/IR/DominatorTreeTest.cpp
===================================================================
--- unittests/IR/DominatorTreeTest.cpp
+++ unittests/IR/DominatorTreeTest.cpp
@@ -203,6 +203,16 @@
         EXPECT_EQ(DT->getNode(BB4)->getDFSNumIn(), 5UL);
         EXPECT_EQ(DT->getNode(BB4)->getDFSNumOut(), 6UL);
 
+        // Change root node
+        DT->verifyDomTree();
+        BasicBlock *NewEntry = BasicBlock::Create(F.getContext(), "new_entry",
+                                                  &F, BB0);
+        BranchInst* NB0 = BranchInst::Create(BB0, NewEntry);
+        EXPECT_EQ(F.begin()->getName(), NewEntry->getName());
+        EXPECT_TRUE(&F.getEntryBlock() == NewEntry);
+        DT->setNewRoot(NewEntry);
+        DT->verifyDomTree();
+
         return false;
       }
       void getAnalysisUsage(AnalysisUsage &AU) const override {
Index: test/Transforms/StructurizeCFG/no-branch-to-entry.ll
===================================================================
--- test/Transforms/StructurizeCFG/no-branch-to-entry.ll
+++ test/Transforms/StructurizeCFG/no-branch-to-entry.ll
@@ -1,4 +1,4 @@
-; RUN: opt -S -o - -structurizecfg < %s | FileCheck %s
+; RUN: opt -S -o - -structurizecfg -verify-dom-info < %s | FileCheck %s
 
 ; CHECK-LABEL: @no_branch_to_entry_undef(
 ; CHECK: entry:
Index: lib/Transforms/Scalar/StructurizeCFG.cpp
===================================================================
--- lib/Transforms/Scalar/StructurizeCFG.cpp
+++ lib/Transforms/Scalar/StructurizeCFG.cpp
@@ -792,6 +792,7 @@
                          LoopFunc,
                          LoopStart);
     BranchInst::Create(LoopStart, NewEntry);
+    DT->setNewRoot(NewEntry);
   }
 
   // Create an extra loop end node
Index: include/llvm/Support/GenericDomTree.h
===================================================================
--- include/llvm/Support/GenericDomTree.h
+++ include/llvm/Support/GenericDomTree.h
@@ -571,9 +571,15 @@
   // API to update (Post)DominatorTree information based on modifications to
   // the CFG...
 
-  /// addNewBlock - Add a new node to the dominator tree information.  This
-  /// creates a new node as a child of DomBB dominator node,linking it into
-  /// the children list of the immediate dominator.
+  /// Add a new node to the dominator tree information.
+  ///
+  /// This creates a new node as a child of DomBB dominator node, linking it
+  /// into the children list of the immediate dominator.
+  ///
+  /// \param BB New node in CFG.
+  /// \param DomBB CFG node that is dominator for BB.
+  /// \returns New dominator tree node that represents new CFG node.
+  ///
   DomTreeNodeBase<NodeT> *addNewBlock(NodeT *BB, NodeT *DomBB) {
     assert(getNode(BB) == nullptr && "Block already in dominator tree!");
     DomTreeNodeBase<NodeT> *IDomNode = getNode(DomBB);
@@ -583,6 +589,31 @@
                 llvm::make_unique<DomTreeNodeBase<NodeT>>(BB, IDomNode))).get();
   }
 
+  /// Add a new node to the forward dominator tree and make it a new root.
+  ///
+  /// \param BB New node in CFG.
+  /// \returns New dominator tree node that represents new CFG node.
+  ///
+  DomTreeNodeBase<NodeT> *setNewRoot(NodeT *BB) {
+    assert(getNode(BB) == nullptr && "Block already in dominator tree!");
+    assert(!isPostDominator() && "Cannot change root of post-dominator tree");
+
+    DFSInfoValid = false;
+    auto &Roots = DominatorBase<NodeT>::Roots;
+    DomTreeNodeBase<NodeT> *NewNode = (DomTreeNodes[BB] =
+      llvm::make_unique<DomTreeNodeBase<NodeT>>(BB, nullptr)).get();
+    if (Roots.empty()) {
+      addRoot(BB);
+    } else {
+      assert(Roots.size() == 1);
+      NodeT *OldRoot = Roots.front();
+      DomTreeNodes[OldRoot] =
+        NewNode->addChild(std::move(DomTreeNodes[OldRoot]));
+      Roots[0] = BB;
+    }
+    return RootNode = NewNode;
+  }
+
   /// changeImmediateDominator - This method is used to update the dominator
   /// tree information when a node's immediate dominator changes.
   ///


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D28114.83602.patch
Type: text/x-patch
Size: 3994 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170109/2739d762/attachment.bin>