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