[llvm] r306919 - [Dominators] Reapply r306892, r306893, r306893.

Davide Italiano via llvm-commits llvm-commits at lists.llvm.org
Thu Jul 6 10:35:12 PDT 2017


Yes, that sounds like an issue in LoopRotate.

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



-- 
Davide

"There are no solved problems; there are only problems that are more
or less solved" -- Henri Poincare


More information about the llvm-commits mailing list