[llvm] r306919 - [Dominators] Reapply r306892, r306893, r306893.
Jakub (Kuba) Kuderski via llvm-commits
llvm-commits at lists.llvm.org
Thu Jul 6 10:33:25 PDT 2017
I filed a bug for that here: https://bugs.llvm.org/show_bug.cgi?id=33701
On Thu, Jul 6, 2017 at 10:19 AM, Jakub (Kuba) Kuderski <
kubakuderski at gmail.com> wrote:
> Mikael,
>
> I played with your reduced IR and here's what happens.
> LoopRotate:rotateLoop iterates over predecessors of bb6, which are
> bb6.loopexit, bb5, and bb3. bb3 is unreachable and it doesn't appear in the
> DomTree:
>
> [1] %bb0
> [2] %bb2
> [3] %bb4
> [4] %bb1
> [5] %bb6
> [5] %bb5
> [3] %bb6.loopexit
>
> The loop tries to find NCD of all predecessors but bb3 doesn't have any,
> so findNearestCommonDominator returns nullptr. rotateLoop doesn't handle
> this case.
>
> I don't think that it's a problem with DomTree, but rather an edge case
> that rotateLoop doesn't handle.
>
> Thanks,
> ~Kuba
>
>
> On Tue, Jul 4, 2017 at 5:51 AM, Jakub (Kuba) Kuderski <
> kubakuderski at gmail.com> wrote:
>
>> Hi Mikael,
>>
>> I'm on vacation till Wednesday and will investigate on Thursday. If
>> that's serious enough, feel free to revert.
>>
>>
>> On Jul 4, 2017 00:37, "Mikael Holmén" <mikael.holmen at ericsson.com> wrote:
>>
>> Hi Jakub,
>>
>> I get a crash when running -loop-rotate on the attached program with your
>> patch:
>>
>> opt -S -loop-rotate -o - reduced9.ll
>>
>> gives
>>
>> opt: ../include/llvm/Support/GenericDomTree.h:499: void
>> llvm::DominatorTreeBase<llvm::BasicBlock>::changeImmediateDominator(DomTreeNodeBase<NodeT>
>> *, DomTreeNodeBase<NodeT> *) [N = llvm::BasicBlock]: Assertion `N &&
>> NewIDom && "Cannot change null node pointers!"' failed.
>>
>>
>> -debug printouts:
>>
>> LoopSimplify: Resolving "br i1 undef" to exit in bb2
>> LoopSimplify: Creating dedicated exit block bb6.loopexit
>> LoopRotation: rotating Loop at depth 1 containing:
>> %bb1<header><exiting>,%bb2<exiting>,%bb4<latch>
>>
>> Regards,
>> Mikael
>>
>>
>> On 07/01/2017 02:23 AM, Jakub Kuderski via llvm-commits wrote:
>>
>>> Author: kuhar
>>> Date: Fri Jun 30 17:23:01 2017
>>> New Revision: 306919
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=306919&view=rev
>>> Log:
>>> [Dominators] Reapply r306892, r306893, r306893.
>>>
>>> This reverts commit r306907 and reapplies the patches in the title.
>>> The patches used to make one of the
>>> CodeGen/ARM/2011-02-07-AntidepClobber.ll test to fail because of a
>>> missing null check.
>>>
>>> Modified:
>>> llvm/trunk/include/llvm/Analysis/IteratedDominanceFrontier.h
>>> llvm/trunk/include/llvm/Support/GenericDomTree.h
>>> llvm/trunk/include/llvm/Support/GenericDomTreeConstruction.h
>>> llvm/trunk/lib/Analysis/IteratedDominanceFrontier.cpp
>>> llvm/trunk/unittests/IR/DominatorTreeTest.cpp
>>>
>>> Modified: llvm/trunk/include/llvm/Analysis/IteratedDominanceFrontier.h
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/
>>> Analysis/IteratedDominanceFrontier.h?rev=306919&r1=306918&r2
>>> =306919&view=diff
>>> ============================================================
>>> ==================
>>> --- llvm/trunk/include/llvm/Analysis/IteratedDominanceFrontier.h
>>> (original)
>>> +++ llvm/trunk/include/llvm/Analysis/IteratedDominanceFrontier.h Fri
>>> Jun 30 17:23:01 2017
>>> @@ -86,7 +86,6 @@ public:
>>> private:
>>> DominatorTreeBase<BasicBlock> &DT;
>>> bool useLiveIn;
>>> - DenseMap<DomTreeNode *, unsigned> DomLevels;
>>> const SmallPtrSetImpl<BasicBlock *> *LiveInBlocks;
>>> const SmallPtrSetImpl<BasicBlock *> *DefBlocks;
>>> };
>>>
>>> Modified: llvm/trunk/include/llvm/Support/GenericDomTree.h
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/
>>> Support/GenericDomTree.h?rev=306919&r1=306918&r2=306919&view=diff
>>> ============================================================
>>> ==================
>>> --- llvm/trunk/include/llvm/Support/GenericDomTree.h (original)
>>> +++ llvm/trunk/include/llvm/Support/GenericDomTree.h Fri Jun 30
>>> 17:23:01 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
>>> @@ -376,7 +407,7 @@ template <class NodeT> class DominatorTr
>>> /// findNearestCommonDominator - Find nearest common dominator
>>> basic block
>>> /// for basic block A and B. If there is no such block then return
>>> NULL.
>>> - NodeT *findNearestCommonDominator(NodeT *A, NodeT *B) {
>>> + NodeT *findNearestCommonDominator(NodeT *A, NodeT *B) const {
>>> assert(A->getParent() == B->getParent() &&
>>> "Two blocks are not in same function");
>>> @@ -388,54 +419,24 @@ 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;
>>> - }
>>> -
>>> - // 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();
>>> - }
>>> + if (!NodeA || !NodeB) return nullptr;
>>> - // 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();
>>> + // 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);
>>> - IDomB = IDomB->getIDom();
>>> + NodeA = NodeA->IDom;
>>> }
>>> - return nullptr;
>>> + return NodeA ? NodeA->getBlock() : nullptr;
>>> }
>>> - const NodeT *findNearestCommonDominator(const NodeT *A, const
>>> NodeT *B) {
>>> + const NodeT *findNearestCommonDominator(const NodeT *A,
>>> + const NodeT *B) const {
>>> // Cast away the const qualifiers here. This is ok since
>>> // const is re-introduced on the return type.
>>> return findNearestCommonDominator(const_cast<NodeT *>(A),
>>> @@ -481,8 +482,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=306919&r1=306918&r2
>>> =306919&view=diff
>>> ============================================================
>>> ==================
>>> --- llvm/trunk/include/llvm/Support/GenericDomTreeConstruction.h
>>> (original)
>>> +++ llvm/trunk/include/llvm/Support/GenericDomTreeConstruction.h Fri
>>> Jun 30 17:23:01 2017
>>> @@ -280,6 +280,7 @@ struct SemiNCAInfo {
>>> }
>>> void doFullDFSWalk(const DomTreeT &DT) {
>>> + NumToNode.push_back(nullptr);
>>> unsigned Num = 0;
>>> for (auto *Root : DT.Roots)
>>> if (!DT.isPostDominator())
>>> @@ -317,6 +318,78 @@ 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 for every edge From -> To in the graph
>>> + // NCD(From, To) == IDom(To) or To.
>>> + bool verifyNCD(const DomTreeT &DT) {
>>> + clear();
>>> + doFullDFSWalk(DT);
>>> +
>>> + for (auto &BlockToInfo : NodeToInfo) {
>>> + auto &Info = BlockToInfo.second;
>>> +
>>> + const NodePtr From = NumToNode[Info.Parent];
>>> + if (!From) continue;
>>> +
>>> + const NodePtr To = BlockToInfo.first;
>>> + const TreeNodePtr ToTN = DT.getNode(To);
>>> + assert(ToTN);
>>> +
>>> + const NodePtr NCD = DT.findNearestCommonDominator(From, To);
>>> + const TreeNodePtr NCDTN = NCD ? DT.getNode(NCD) : nullptr;
>>> + const TreeNodePtr ToIDom = ToTN->getIDom();
>>> + if (NCDTN != ToTN && NCDTN != ToIDom) {
>>> + errs() << "NearestCommonDominator verification
>>> failed:\n\tNCD(From:";
>>> + PrintBlockOrNullptr(errs(), From);
>>> + errs() << ", To:";
>>> + PrintBlockOrNullptr(errs(), To);
>>> + errs() << ") = ";
>>> + PrintBlockOrNullptr(errs(), NCD);
>>> + errs() << ",\t (should be To or IDom[To]: ";
>>> + PrintBlockOrNullptr(errs(), ToIDom ? ToIDom->getBlock() :
>>> nullptr);
>>> + errs() << ")\n";
>>> + errs().flush();
>>> +
>>> + return false;
>>> + }
>>> + }
>>> +
>>> + return true;
>>> + }
>>> +
>>> // The below routines verify the correctness of the dominator tree
>>> relative to
>>> // the CFG it's coming from. A tree is a dominator tree iff it has
>>> two
>>> // properties, called the parent property and the sibling property.
>>> Tarjan
>>> @@ -441,9 +514,9 @@ 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)
>>> &&
>>> + return SNCA.verifyReachability(DT) && SNCA.VerifyLevels(DT) &&
>>> + SNCA.verifyNCD(DT) && SNCA.verifyParentProperty(DT) &&
>>> SNCA.verifySiblingProperty(DT);
>>> -
>>> }
>>> } // namespace DomTreeBuilder
>>>
>>> Modified: llvm/trunk/lib/Analysis/IteratedDominanceFrontier.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/
>>> IteratedDominanceFrontier.cpp?rev=306919&r1=306918&r2=306919&view=diff
>>> ============================================================
>>> ==================
>>> --- llvm/trunk/lib/Analysis/IteratedDominanceFrontier.cpp (original)
>>> +++ llvm/trunk/lib/Analysis/IteratedDominanceFrontier.cpp Fri Jun 30
>>> 17:23:01 2017
>>> @@ -20,14 +20,6 @@ namespace llvm {
>>> template <class NodeTy>
>>> void IDFCalculator<NodeTy>::calculate(
>>> SmallVectorImpl<BasicBlock *> &PHIBlocks) {
>>> - // If we haven't computed dominator tree levels, do so now.
>>> - if (DomLevels.empty()) {
>>> - for (auto DFI = df_begin(DT.getRootNode()), DFE =
>>> df_end(DT.getRootNode());
>>> - DFI != DFE; ++DFI) {
>>> - DomLevels[*DFI] = DFI.getPathLength() - 1;
>>> - }
>>> - }
>>> -
>>> // Use a priority queue keyed on dominator tree level so that
>>> inserted nodes
>>> // are handled from the bottom of the dominator tree upwards.
>>> typedef std::pair<DomTreeNode *, unsigned> DomTreeNodePair;
>>> @@ -37,7 +29,7 @@ void IDFCalculator<NodeTy>::calculate(
>>> for (BasicBlock *BB : *DefBlocks) {
>>> if (DomTreeNode *Node = DT.getNode(BB))
>>> - PQ.push(std::make_pair(Node, DomLevels.lookup(Node)));
>>> + PQ.push({Node, Node->getLevel()});
>>> }
>>> SmallVector<DomTreeNode *, 32> Worklist;
>>> @@ -72,7 +64,7 @@ void IDFCalculator<NodeTy>::calculate(
>>> if (SuccNode->getIDom() == Node)
>>> continue;
>>> - unsigned SuccLevel = DomLevels.lookup(SuccNode);
>>> + const unsigned SuccLevel = SuccNode->getLevel();
>>> if (SuccLevel > RootLevel)
>>> continue;
>>>
>>> Modified: llvm/trunk/unittests/IR/DominatorTreeTest.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/IR/
>>> DominatorTreeTest.cpp?rev=306919&r1=306918&r2=306919&view=diff
>>> ============================================================
>>> ==================
>>> --- llvm/trunk/unittests/IR/DominatorTreeTest.cpp (original)
>>> +++ llvm/trunk/unittests/IR/DominatorTreeTest.cpp Fri Jun 30 17:23:01
>>> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170706/e46d51c1/attachment.html>
More information about the llvm-commits
mailing list