[llvm] r306919 - [Dominators] Reapply r306892, r306893, r306893.
Jakub (Kuba) Kuderski via llvm-commits
llvm-commits at lists.llvm.org
Tue Jul 4 05:51:56 PDT 2017
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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170704/0c419fbd/attachment-0001.html>
More information about the llvm-commits
mailing list