From f92233652ebedd95ff49bc487cbf99dca90ea428 Mon Sep 17 00:00:00 2001 From: Jakub Kuderski Date: Thu, 29 Jun 2017 17:45:51 +0000 Subject: [PATCH] [Dominators] Add parent and sibling property verification (non-hacky) Summary: This patch adds an additional level of verification - it checks parent and sibling properties of a tree. By definition, every tree with these two properties is a dominator tree. It is possible to run those check by running llvm with `-verify-dom-info=1`. Bootstrapping clang and building the llvm test suite with this option enabled doesn't yield any errors. Reviewers: dberlin, sanjoy, chandlerc Reviewed By: dberlin Subscribers: llvm-commits Differential Revision: https://reviews.llvm.org/D34482 llvm-svn: 306711 --- llvm/include/llvm/IR/Dominators.h | 10 ++ llvm/include/llvm/Support/GenericDomTree.h | 17 +- .../llvm/Support/GenericDomTreeConstruction.h | 149 ++++++++++++++++-- llvm/lib/IR/Dominators.cpp | 17 +- 4 files changed, 174 insertions(+), 19 deletions(-) diff --git a/llvm/include/llvm/IR/Dominators.h b/llvm/include/llvm/IR/Dominators.h index 9be6acc33591..e10d14c19793 100644 --- a/llvm/include/llvm/IR/Dominators.h +++ b/llvm/include/llvm/IR/Dominators.h @@ -36,12 +36,22 @@ class raw_ostream; extern template class DomTreeNodeBase; extern template class DominatorTreeBase; +namespace DomTreeBuilder { extern template void Calculate( DominatorTreeBaseByGraphTraits> &DT, Function &F); + extern template void Calculate>( DominatorTreeBaseByGraphTraits>> &DT, Function &F); +extern template bool Verify( + const DominatorTreeBaseByGraphTraits> &DT); + +extern template bool Verify>( + const DominatorTreeBaseByGraphTraits>> + &DT); +} // namespace DomTreeBuilder + using DomTreeNode = DomTreeNodeBase; class BasicBlockEdge { diff --git a/llvm/include/llvm/Support/GenericDomTree.h b/llvm/include/llvm/Support/GenericDomTree.h index 777600cb3466..e8f87fc142ec 100644 --- a/llvm/include/llvm/Support/GenericDomTree.h +++ b/llvm/include/llvm/Support/GenericDomTree.h @@ -204,12 +204,16 @@ void PrintDomTree(const DomTreeNodeBase *N, raw_ostream &O, namespace DomTreeBuilder { template struct SemiNCAInfo; -} // namespace DomTreeBuilder // The calculate routine is provided in a separate header but referenced here. template void Calculate(DominatorTreeBaseByGraphTraits> &DT, FuncT &F); +// The verify function is provided in a separate header but referenced here. +template +bool Verify(const DominatorTreeBaseByGraphTraits> &DT); +} // namespace DomTreeBuilder + /// \brief Core dominator tree base class. /// /// This class is a generic template over graph nodes. It is instantiated for @@ -723,16 +727,23 @@ public: NodeT *entry = TraitsTy::getEntryNode(&F); addRoot(entry); - Calculate(*this, F); + DomTreeBuilder::Calculate(*this, F); } else { // Initialize the roots list for (auto *Node : nodes(&F)) if (TraitsTy::child_begin(Node) == TraitsTy::child_end(Node)) addRoot(Node); - Calculate>(*this, F); + DomTreeBuilder::Calculate>(*this, F); } } + + /// verify - check parent and sibling property + bool verify() const { + return this->isPostDominator() + ? DomTreeBuilder::Verify>(*this) + : DomTreeBuilder::Verify(*this); + } }; // These two functions are declared out of line as a workaround for building diff --git a/llvm/include/llvm/Support/GenericDomTreeConstruction.h b/llvm/include/llvm/Support/GenericDomTreeConstruction.h index ff7f7db3ecb3..7899a81751a6 100644 --- a/llvm/include/llvm/Support/GenericDomTreeConstruction.h +++ b/llvm/include/llvm/Support/GenericDomTreeConstruction.h @@ -30,8 +30,8 @@ #include "llvm/Support/GenericDomTree.h" namespace llvm { - namespace DomTreeBuilder { + // Information record used by Semi-NCA during tree construction. template struct SemiNCAInfo { @@ -47,11 +47,13 @@ struct SemiNCAInfo { NodePtr IDom = nullptr; }; - DomTreeT &DT; std::vector NumToNode; DenseMap NodeToInfo; - SemiNCAInfo(DomTreeT &DT) : DT(DT) {} + void clear() { + NumToNode.clear(); + NodeToInfo.clear(); + } NodePtr getIDom(NodePtr BB) const { auto InfoIt = NodeToInfo.find(BB); @@ -60,7 +62,7 @@ struct SemiNCAInfo { return InfoIt->second.IDom; } - TreeNodePtr getNodeForBlock(NodePtr BB) { + TreeNodePtr getNodeForBlock(NodePtr BB, DomTreeT &DT) { if (TreeNodePtr Node = DT.getNode(BB)) return Node; // Haven't calculated this node yet? Get or calculate the node for the @@ -68,7 +70,7 @@ struct SemiNCAInfo { NodePtr IDom = getIDom(BB); assert(IDom || DT.DomTreeNodes[nullptr]); - TreeNodePtr IDomNode = getNodeForBlock(IDom); + TreeNodePtr IDomNode = getNodeForBlock(IDom, DT); // Add a new tree node for this NodeT, and link it as a child of // IDomNode @@ -121,7 +123,7 @@ struct SemiNCAInfo { return N; } - unsigned runDFS(NodePtr V, unsigned N) { + unsigned runForwardDFS(NodePtr V, unsigned N) { auto DFStorage = getStorage(); for (auto I = df_ext_begin(V, DFStorage), E = df_ext_end(V, DFStorage); @@ -178,7 +180,7 @@ struct SemiNCAInfo { } template - void runSemiNCA(unsigned NumBlocks) { + void runSemiNCA(DomTreeT &DT, unsigned NumBlocks) { unsigned N = 0; NumToNode.push_back(nullptr); @@ -198,7 +200,7 @@ struct SemiNCAInfo { i != e; ++i) N = runReverseDFS(DT.Roots[i], N); } else { - N = runDFS(DT.Roots[0], N); + N = runForwardDFS(DT.Roots[0], N); } // It might be that some blocks did not get a DFS number (e.g., blocks of @@ -268,7 +270,7 @@ struct SemiNCAInfo { assert(ImmDom || DT.DomTreeNodes[nullptr]); // Get or calculate the node for the immediate dominator - TreeNodePtr IDomNode = getNodeForBlock(ImmDom); + TreeNodePtr IDomNode = getNodeForBlock(ImmDom, DT); // Add a new tree node for this BasicBlock, and link it as a child of // IDomNode @@ -278,8 +280,115 @@ struct SemiNCAInfo { DT.updateDFSNumbers(); } + + void doFullDFSWalk(const DomTreeT &DT) { + unsigned Num = 0; + for (auto *Root : DT.Roots) + if (!DT.isPostDominator()) + Num = runForwardDFS(Root, Num); + else + Num = runReverseDFS(Root, Num); + } + + static void PrintBlockOrNullptr(raw_ostream &O, NodePtr Obj) { + if (!Obj) + O << "nullptr"; + else + Obj->printAsOperand(O, false); + } + + // Checks if the tree contains all reachable nodes in the input graph. + bool verifyReachability(const DomTreeT &DT) { + clear(); + doFullDFSWalk(DT); + + for (auto &NodeToTN : DT.DomTreeNodes) { + const TreeNodePtr TN = NodeToTN.second.get(); + const NodePtr BB = TN->getBlock(); + if (!BB) continue; + + if (NodeToInfo.count(BB) == 0) { + errs() << "DomTree node "; + PrintBlockOrNullptr(errs(), BB); + errs() << " not found by DFS walk!\n"; + errs().flush(); + + return false; + } + } + + return true; + } + + // Checks if the tree has the parent property: if for all edges from V to W in + // the input graph, such that V is reachable, the parent of W in the tree is + // an ancestor of V in the tree. + // + // This means that if a node gets disconnected from the graph, then all of + // the nodes it dominated previously will now become unreachable. + bool verifyParentProperty(const DomTreeT &DT) { + for (auto &NodeToTN : DT.DomTreeNodes) { + const TreeNodePtr TN = NodeToTN.second.get(); + const NodePtr BB = TN->getBlock(); + if (!BB || TN->getChildren().empty()) continue; + + clear(); + NodeToInfo.insert({BB, {}}); + doFullDFSWalk(DT); + + for (TreeNodePtr Child : TN->getChildren()) + if (NodeToInfo.count(Child->getBlock()) != 0) { + errs() << "Child "; + PrintBlockOrNullptr(errs(), Child->getBlock()); + errs() << " reachable after its parent "; + PrintBlockOrNullptr(errs(), BB); + errs() << " is removed!\n"; + errs().flush(); + + return false; + } + } + + return true; + } + + // Check if the tree has sibling property: if a node V does not dominate a + // node W for all siblings V and W in the tree. + // + // This means that if a node gets disconnected from the graph, then all of its + // siblings will now still be reachable. + bool verifySiblingProperty(const DomTreeT &DT) { + for (auto &NodeToTN : DT.DomTreeNodes) { + const TreeNodePtr TN = NodeToTN.second.get(); + const NodePtr BB = TN->getBlock(); + if (!BB || TN->getChildren().empty()) continue; + + const auto &Siblings = TN->getChildren(); + for (const TreeNodePtr N : Siblings) { + clear(); + NodeToInfo.insert({N->getBlock(), {}}); + doFullDFSWalk(DT); + + for (const TreeNodePtr S : Siblings) { + if (S == N) continue; + + if (NodeToInfo.count(S->getBlock()) == 0) { + errs() << "Node "; + PrintBlockOrNullptr(errs(), S->getBlock()); + errs() << " not reachable when its sibling "; + PrintBlockOrNullptr(errs(), N->getBlock()); + errs() << " is removed!\n"; + errs().flush(); + + return false; + } + } + } + } + + return true; + } }; -} // namespace DomTreeBuilder template void Calculate(DominatorTreeBaseByGraphTraits> &DT, @@ -287,11 +396,23 @@ void Calculate(DominatorTreeBaseByGraphTraits> &DT, using NodePtr = typename GraphTraits::NodeRef; static_assert(std::is_pointer::value, "NodePtr should be a pointer type"); - - DomTreeBuilder::SemiNCAInfo::type> - SNCA(DT); - SNCA.template runSemiNCA(GraphTraits::size(&F)); + SemiNCAInfo::type> SNCA; + SNCA.template runSemiNCA(DT, GraphTraits::size(&F)); } + +template +bool Verify(const DominatorTreeBaseByGraphTraits> &DT) { + using NodePtr = typename GraphTraits::NodeRef; + static_assert(std::is_pointer::value, + "NodePtr should be a pointer type"); + SemiNCAInfo::type> SNCA; + + return SNCA.verifyReachability(DT) && SNCA.verifyParentProperty(DT) && + SNCA.verifySiblingProperty(DT); + +} + +} // namespace DomTreeBuilder } // namespace llvm #endif diff --git a/llvm/lib/IR/Dominators.cpp b/llvm/lib/IR/Dominators.cpp index 37e735251fdf..6ac3c5e4632c 100644 --- a/llvm/lib/IR/Dominators.cpp +++ b/llvm/lib/IR/Dominators.cpp @@ -63,15 +63,22 @@ bool BasicBlockEdge::isSingleEdge() const { template class llvm::DomTreeNodeBase; template class llvm::DominatorTreeBase; -template void llvm::Calculate( +template void llvm::DomTreeBuilder::Calculate( DominatorTreeBase< typename std::remove_pointer::NodeRef>::type> &DT, Function &F); -template void llvm::Calculate>( +template void llvm::DomTreeBuilder::Calculate>( DominatorTreeBase>::NodeRef>::type> &DT, Function &F); +template bool llvm::DomTreeBuilder::Verify( + const DominatorTreeBase< + typename std::remove_pointer::NodeRef>::type> + &DT); +template bool llvm::DomTreeBuilder::Verify>( + const DominatorTreeBase>::NodeRef>::type> &DT); bool DominatorTree::invalidate(Function &F, const PreservedAnalyses &PA, FunctionAnalysisManager::Invalidator &) { @@ -285,6 +292,12 @@ bool DominatorTree::isReachableFromEntry(const Use &U) const { } void DominatorTree::verifyDomTree() const { + if (!verify()) { + errs() << "\n~~~~~~~~~~~\n\t\tDomTree verification failed!\n~~~~~~~~~~~\n"; + print(errs()); + abort(); + } + Function &F = *getRoot()->getParent(); DominatorTree OtherDT;