[llvm] r306919 - [Dominators] Reapply r306892, r306893, r306893.
Jakub (Kuba) Kuderski via llvm-commits
llvm-commits at lists.llvm.org
Thu Jul 6 10:19:00 PDT 2017
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170706/fb83ef19/attachment.html>
More information about the llvm-commits
mailing list