<div dir="ltr">Reverting now.<br></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jun 30, 2017 at 3:40 PM, Rafael Avila de Espindola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">This is crashing on<br>
<br>
CodeGen/ARM/2011-02-07-<wbr>AntidepClobber.ll<br>
<br>
Cheers,<br>
Rafael<br>
<div class="HOEnZb"><div class="h5"><br>
Jakub Kuderski via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> writes:<br>
<br>
> Author: kuhar<br>
> Date: Fri Jun 30 14:51:40 2017<br>
> New Revision: 306892<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=306892&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=306892&view=rev</a><br>
> Log:<br>
> [Dominators] Keep tree level in DomTreeNode and use it to find NCD and answer dominance queries<br>
><br>
> Summary:<br>
> This patch makes DomTreeNodes keep their level (depth) in the DomTree. By having this information always available, it is possible to speedup and simplify findNearestCommonDominator and certain dominance queries.<br>
><br>
> In the future, level information will be also needed to perform incremental updates.<br>
><br>
> My testing doesn't show any noticeable performance differences after applying this patch. There may be some improvements when other passes are thought to use the level information.<br>
><br>
> Reviewers: dberlin, sanjoy, chandlerc, grosser<br>
><br>
> Reviewed By: dberlin<br>
><br>
> Subscribers: llvm-commits<br>
><br>
> Differential Revision: <a href="https://reviews.llvm.org/D34548" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D34548</a><br>
><br>
> Modified:<br>
> llvm/trunk/include/llvm/<wbr>Support/GenericDomTree.h<br>
> llvm/trunk/include/llvm/<wbr>Support/<wbr>GenericDomTreeConstruction.h<br>
> llvm/trunk/unittests/IR/<wbr>DominatorTreeTest.cpp<br>
><br>
> Modified: llvm/trunk/include/llvm/<wbr>Support/GenericDomTree.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/GenericDomTree.h?rev=306892&r1=306891&r2=306892&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/include/<wbr>llvm/Support/GenericDomTree.h?<wbr>rev=306892&r1=306891&r2=<wbr>306892&view=diff</a><br>
> ==============================<wbr>==============================<wbr>==================<br>
> --- llvm/trunk/include/llvm/<wbr>Support/GenericDomTree.h (original)<br>
> +++ llvm/trunk/include/llvm/<wbr>Support/GenericDomTree.h Fri Jun 30 14:51:40 2017<br>
> @@ -65,12 +65,14 @@ template <class NodeT> class DomTreeNode<br>
><br>
> NodeT *TheBB;<br>
> DomTreeNodeBase *IDom;<br>
> + unsigned Level;<br>
> std::vector<DomTreeNodeBase *> Children;<br>
> mutable unsigned DFSNumIn = ~0;<br>
> mutable unsigned DFSNumOut = ~0;<br>
><br>
> public:<br>
> - DomTreeNodeBase(NodeT *BB, DomTreeNodeBase *iDom) : TheBB(BB), IDom(iDom) {}<br>
> + DomTreeNodeBase(NodeT *BB, DomTreeNodeBase *iDom)<br>
> + : TheBB(BB), IDom(iDom), Level(IDom ? IDom->Level + 1 : 0) {}<br>
><br>
> using iterator = typename std::vector<DomTreeNodeBase *>::iterator;<br>
> using const_iterator =<br>
> @@ -83,6 +85,7 @@ template <class NodeT> class DomTreeNode<br>
><br>
> NodeT *getBlock() const { return TheBB; }<br>
> DomTreeNodeBase *getIDom() const { return IDom; }<br>
> + unsigned getLevel() const { return Level; }<br>
><br>
> const std::vector<DomTreeNodeBase *> &getChildren() const { return Children; }<br>
><br>
> @@ -100,6 +103,8 @@ template <class NodeT> class DomTreeNode<br>
> if (getNumChildren() != Other->getNumChildren())<br>
> return true;<br>
><br>
> + if (Level != Other->Level) return true;<br>
> +<br>
> SmallPtrSet<const NodeT *, 4> OtherChildren;<br>
> for (const DomTreeNodeBase *I : *Other) {<br>
> const NodeT *Nd = I->getBlock();<br>
> @@ -116,18 +121,19 @@ template <class NodeT> class DomTreeNode<br>
><br>
> void setIDom(DomTreeNodeBase *NewIDom) {<br>
> assert(IDom && "No immediate dominator?");<br>
> - if (IDom != NewIDom) {<br>
> - typename std::vector<DomTreeNodeBase *>::iterator I =<br>
> - find(IDom->Children, this);<br>
> - assert(I != IDom->Children.end() &&<br>
> - "Not in immediate dominator children set!");<br>
> - // I am no longer your child...<br>
> - IDom->Children.erase(I);<br>
> -<br>
> - // Switch to new dominator<br>
> - IDom = NewIDom;<br>
> - IDom->Children.push_back(this)<wbr>;<br>
> - }<br>
> + if (IDom == NewIDom) return;<br>
> +<br>
> + auto I = find(IDom->Children, this);<br>
> + assert(I != IDom->Children.end() &&<br>
> + "Not in immediate dominator children set!");<br>
> + // I am no longer your child...<br>
> + IDom->Children.erase(I);<br>
> +<br>
> + // Switch to new dominator<br>
> + IDom = NewIDom;<br>
> + IDom->Children.push_back(this)<wbr>;<br>
> +<br>
> + UpdateLevel();<br>
> }<br>
><br>
> /// getDFSNumIn/getDFSNumOut - These return the DFS visitation order for nodes<br>
> @@ -143,6 +149,23 @@ private:<br>
> return this->DFSNumIn >= other->DFSNumIn &&<br>
> this->DFSNumOut <= other->DFSNumOut;<br>
> }<br>
> +<br>
> + void UpdateLevel() {<br>
> + assert(IDom);<br>
> + if (Level == IDom->Level + 1) return;<br>
> +<br>
> + SmallVector<DomTreeNodeBase *, 64> WorkStack = {this};<br>
> +<br>
> + while (!WorkStack.empty()) {<br>
> + DomTreeNodeBase *Current = WorkStack.pop_back_val();<br>
> + Current->Level = Current->IDom->Level + 1;<br>
> +<br>
> + for (DomTreeNodeBase *C : *Current) {<br>
> + assert(C->IDom);<br>
> + if (C->Level != C->IDom->Level + 1) WorkStack.push_back(C);<br>
> + }<br>
> + }<br>
> + }<br>
> };<br>
><br>
> template <class NodeT><br>
> @@ -152,9 +175,10 @@ raw_ostream &operator<<(raw_ostream &O,<br>
> else<br>
> O << " <<exit node>>";<br>
><br>
> - O << " {" << Node->getDFSNumIn() << "," << Node->getDFSNumOut() << "}";<br>
> + O << " {" << Node->getDFSNumIn() << "," << Node->getDFSNumOut() << "} ["<br>
> + << Node->getLevel() << "]\n";<br>
><br>
> - return O << "\n";<br>
> + return O;<br>
> }<br>
><br>
> template <class NodeT><br>
> @@ -345,6 +369,13 @@ template <class NodeT> class DominatorTr<br>
> if (!isReachableFromEntry(A))<br>
> return false;<br>
><br>
> + if (B->getIDom() == A) return true;<br>
> +<br>
> + if (A->getIDom() == B) return false;<br>
> +<br>
> + // A can only dominate B if it is higher in the tree.<br>
> + if (A->getLevel() >= B->getLevel()) return false;<br>
> +<br>
> // Compare the result of the tree walk and the dfs numbers, if expensive<br>
> // checks are enabled.<br>
> #ifdef EXPENSIVE_CHECKS<br>
> @@ -388,51 +419,18 @@ template <class NodeT> class DominatorTr<br>
> return &Entry;<br>
> }<br>
><br>
> - // If B dominates A then B is nearest common dominator.<br>
> - if (dominates(B, A))<br>
> - return B;<br>
> -<br>
> - // If A dominates B then A is nearest common dominator.<br>
> - if (dominates(A, B))<br>
> - return A;<br>
> -<br>
> DomTreeNodeBase<NodeT> *NodeA = getNode(A);<br>
> DomTreeNodeBase<NodeT> *NodeB = getNode(B);<br>
><br>
> - // If we have DFS info, then we can avoid all allocations by just querying<br>
> - // it from each IDom. Note that because we call 'dominates' twice above, we<br>
> - // expect to call through this code at most 16 times in a row without<br>
> - // building valid DFS information. This is important as below is a *very*<br>
> - // slow tree walk.<br>
> - if (DFSInfoValid) {<br>
> - DomTreeNodeBase<NodeT> *IDomA = NodeA->getIDom();<br>
> - while (IDomA) {<br>
> - if (NodeB->DominatedBy(IDomA))<br>
> - return IDomA->getBlock();<br>
> - IDomA = IDomA->getIDom();<br>
> - }<br>
> - return nullptr;<br>
> - }<br>
> + // Use level information to go up the tree until the levels match. Then<br>
> + // continue going up til we arrive at the same node.<br>
> + while (NodeA && NodeA != NodeB) {<br>
> + if (NodeA->getLevel() < NodeB->getLevel()) std::swap(NodeA, NodeB);<br>
><br>
> - // Collect NodeA dominators set.<br>
> - SmallPtrSet<DomTreeNodeBase<<wbr>NodeT> *, 16> NodeADoms;<br>
> - NodeADoms.insert(NodeA);<br>
> - DomTreeNodeBase<NodeT> *IDomA = NodeA->getIDom();<br>
> - while (IDomA) {<br>
> - NodeADoms.insert(IDomA);<br>
> - IDomA = IDomA->getIDom();<br>
> + NodeA = NodeA->IDom;<br>
> }<br>
><br>
> - // Walk NodeB immediate dominators chain and find common dominator node.<br>
> - DomTreeNodeBase<NodeT> *IDomB = NodeB->getIDom();<br>
> - while (IDomB) {<br>
> - if (NodeADoms.count(IDomB) != 0)<br>
> - return IDomB->getBlock();<br>
> -<br>
> - IDomB = IDomB->getIDom();<br>
> - }<br>
> -<br>
> - return nullptr;<br>
> + return NodeA ? NodeA->getBlock() : nullptr;<br>
> }<br>
><br>
> const NodeT *findNearestCommonDominator(<wbr>const NodeT *A, const NodeT *B) {<br>
> @@ -481,8 +479,10 @@ template <class NodeT> class DominatorTr<br>
> } else {<br>
> assert(Roots.size() == 1);<br>
> NodeT *OldRoot = Roots.front();<br>
> - DomTreeNodes[OldRoot] =<br>
> - NewNode->addChild(std::move(<wbr>DomTreeNodes[OldRoot]));<br>
> + auto &OldNode = DomTreeNodes[OldRoot];<br>
> + OldNode = NewNode->addChild(std::move(<wbr>DomTreeNodes[OldRoot]));<br>
> + OldNode->IDom = NewNode;<br>
> + OldNode->UpdateLevel();<br>
> Roots[0] = BB;<br>
> }<br>
> return RootNode = NewNode;<br>
><br>
> Modified: llvm/trunk/include/llvm/<wbr>Support/<wbr>GenericDomTreeConstruction.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/GenericDomTreeConstruction.h?rev=306892&r1=306891&r2=306892&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/include/<wbr>llvm/Support/<wbr>GenericDomTreeConstruction.h?<wbr>rev=306892&r1=306891&r2=<wbr>306892&view=diff</a><br>
> ==============================<wbr>==============================<wbr>==================<br>
> --- llvm/trunk/include/llvm/<wbr>Support/<wbr>GenericDomTreeConstruction.h (original)<br>
> +++ llvm/trunk/include/llvm/<wbr>Support/<wbr>GenericDomTreeConstruction.h Fri Jun 30 14:51:40 2017<br>
> @@ -318,6 +318,39 @@ struct SemiNCAInfo {<br>
> return true;<br>
> }<br>
><br>
> + // Check if for every parent with a level L in the tree all of its children<br>
> + // have level L + 1.<br>
> + static bool VerifyLevels(const DomTreeT &DT) {<br>
> + for (auto &NodeToTN : DT.DomTreeNodes) {<br>
> + const TreeNodePtr TN = NodeToTN.second.get();<br>
> + const NodePtr BB = TN->getBlock();<br>
> + if (!BB) continue;<br>
> +<br>
> + const TreeNodePtr IDom = TN->getIDom();<br>
> + if (!IDom && TN->getLevel() != 0) {<br>
> + errs() << "Node without an IDom ";<br>
> + PrintBlockOrNullptr(errs(), BB);<br>
> + errs() << " has a nonzero level " << TN->getLevel() << "!\n";<br>
> + errs().flush();<br>
> +<br>
> + return false;<br>
> + }<br>
> +<br>
> + if (IDom && TN->getLevel() != IDom->getLevel() + 1) {<br>
> + errs() << "Node ";<br>
> + PrintBlockOrNullptr(errs(), BB);<br>
> + errs() << " has level " << TN->getLevel() << " while it's IDom ";<br>
> + PrintBlockOrNullptr(errs(), IDom->getBlock());<br>
> + errs() << " has level " << IDom->getLevel() << "!\n";<br>
> + errs().flush();<br>
> +<br>
> + return false;<br>
> + }<br>
> + }<br>
> +<br>
> + return true;<br>
> + }<br>
> +<br>
> // Checks if the tree has the parent property: if for all edges from V to W in<br>
> // the input graph, such that V is reachable, the parent of W in the tree is<br>
> // an ancestor of V in the tree.<br>
> @@ -405,9 +438,8 @@ bool Verify(const DominatorTreeBaseByGra<br>
> "NodePtr should be a pointer type");<br>
> SemiNCAInfo<typename std::remove_pointer<NodePtr>::<wbr>type> SNCA;<br>
><br>
> - return SNCA.verifyReachability(DT) && SNCA.verifyParentProperty(DT) &&<br>
> - SNCA.verifySiblingProperty(DT)<wbr>;<br>
> -<br>
> + return SNCA.verifyReachability(DT) && SNCA.VerifyLevels(DT) &&<br>
> + SNCA.verifyParentProperty(DT) && SNCA.verifySiblingProperty(DT)<wbr>;<br>
> }<br>
><br>
> } // namespace DomTreeBuilder<br>
><br>
> Modified: llvm/trunk/unittests/IR/<wbr>DominatorTreeTest.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/IR/DominatorTreeTest.cpp?rev=306892&r1=306891&r2=306892&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/unittests/<wbr>IR/DominatorTreeTest.cpp?rev=<wbr>306892&r1=306891&r2=306892&<wbr>view=diff</a><br>
> ==============================<wbr>==============================<wbr>==================<br>
> --- llvm/trunk/unittests/IR/<wbr>DominatorTreeTest.cpp (original)<br>
> +++ llvm/trunk/unittests/IR/<wbr>DominatorTreeTest.cpp Fri Jun 30 14:51:40 2017<br>
> @@ -230,6 +230,12 @@ TEST(DominatorTree, Unreachable) {<br>
> EXPECT_EQ(DT->getNode(BB4)-><wbr>getDFSNumIn(), 3UL);<br>
> EXPECT_EQ(DT->getNode(BB4)-><wbr>getDFSNumOut(), 4UL);<br>
><br>
> + // Check levels before<br>
> + EXPECT_EQ(DT->getNode(BB0)-><wbr>getLevel(), 0U);<br>
> + EXPECT_EQ(DT->getNode(BB1)-><wbr>getLevel(), 1U);<br>
> + EXPECT_EQ(DT->getNode(BB2)-><wbr>getLevel(), 1U);<br>
> + EXPECT_EQ(DT->getNode(BB4)-><wbr>getLevel(), 1U);<br>
> +<br>
> // Reattach block 3 to block 1 and recalculate<br>
> BB1->getTerminator()-><wbr>eraseFromParent();<br>
> BranchInst::Create(BB4, BB3, ConstantInt::getTrue(F.<wbr>getContext()), BB1);<br>
> @@ -248,6 +254,13 @@ TEST(DominatorTree, Unreachable) {<br>
> EXPECT_EQ(DT->getNode(BB4)-><wbr>getDFSNumIn(), 5UL);<br>
> EXPECT_EQ(DT->getNode(BB4)-><wbr>getDFSNumOut(), 6UL);<br>
><br>
> + // Check levels after<br>
> + EXPECT_EQ(DT->getNode(BB0)-><wbr>getLevel(), 0U);<br>
> + EXPECT_EQ(DT->getNode(BB1)-><wbr>getLevel(), 1U);<br>
> + EXPECT_EQ(DT->getNode(BB2)-><wbr>getLevel(), 1U);<br>
> + EXPECT_EQ(DT->getNode(BB3)-><wbr>getLevel(), 2U);<br>
> + EXPECT_EQ(DT->getNode(BB4)-><wbr>getLevel(), 1U);<br>
> +<br>
> // Change root node<br>
> DT->verifyDomTree();<br>
> BasicBlock *NewEntry =<br>
><br>
><br>
> ______________________________<wbr>_________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature" data-smartmail="gmail_signature"><div>Jakub Kuderski</div></div>
</div>