[llvm] r314801 - [Dominators] Add DFS number verification
Jakub (Kuba) Kuderski via llvm-commits
llvm-commits at lists.llvm.org
Tue Oct 3 13:46:25 PDT 2017
Well, I don't know why it's failing by just looking at it. I'll submit a
fix for the LoopRotate bug in a moment and will see it unbreaks this
buildbot.
On Tue, Oct 3, 2017 at 4:42 PM, Juergen Ributzka <juergen at ributzka.de>
wrote:
> I don't know if this is reproducible on Linux, but the file that is
> failing is common code in compiler-rt (sanitizer_libc.cc.o).
>
> On Tue, Oct 3, 2017 at 1:32 PM, Jakub (Kuba) Kuderski <
> kubakuderski at gmail.com> wrote:
>
>> Hi Juergen,
>>
>> It seems like the patch caught some invalid DominatorTrees :).
>>
>> I'm now investigating a failure in LoopRotation happening in
>> test/Transforms/ArgumentPromotion/pr3085.ll with expensive checks
>> enabled, but I don't know if that's related.
>> I looked and the buildbot's output and it seems to fail on OSX -- do you
>> know if it's reproducible on x86_64 linux?
>>
>> Best,
>> Jakub
>>
>> On Tue, Oct 3, 2017 at 4:26 PM, Juergen Ributzka <juergen at ributzka.de>
>> wrote:
>>
>>> Hi Jakub,
>>>
>>> I think this broke one of our jobs on Green Dragon:
>>> http://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-ex
>>> pensive/8568/consoleFull
>>>
>>> Could you please take a look?
>>>
>>> Thanks
>>>
>>> Cheers,
>>> Juergen
>>>
>>> On Tue, Oct 3, 2017 at 7:33 AM, Jakub Kuderski via llvm-commits <
>>> llvm-commits at lists.llvm.org> wrote:
>>>
>>>> Author: kuhar
>>>> Date: Tue Oct 3 07:33:41 2017
>>>> New Revision: 314801
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=314801&view=rev
>>>> Log:
>>>> [Dominators] Add DFS number verification
>>>>
>>>> Summary:
>>>> This patch teaches the DominatorTree verifier to check DFS In/Out
>>>> numbers which are used to answer dominance queries.
>>>> DFS number verification is done in O(nlogn), so it shouldn't add much
>>>> overhead on top of the O(n^3) sibling property verification.
>>>> This check should detect errors like the one spotted in PR34466 and
>>>> related bug reports.
>>>>
>>>> The patch also cleans up the DFS calculation a bit, as all constructed
>>>> trees should have a single root now.
>>>>
>>>> I see 2 new test failures when running check-all after this change:
>>>>
>>>> ```
>>>> Failing Tests (2):
>>>> Polly :: Isl/CodeGen/OpenMP/reference-a
>>>> rgument-from-non-affine-region.ll
>>>> Polly :: Isl/CodeGen/OpenMP/two-paralle
>>>> l-loops-reference-outer-indvar.ll
>>>>
>>>> ```
>>>> which seem to happen just after `Create LLVM-IR from SCoPs` -- I
>>>> XFAILed them in r314800.
>>>>
>>>> Reviewers: dberlin, grosser, davide, zhendongsu, bollu
>>>>
>>>> Reviewed By: dberlin
>>>>
>>>> Subscribers: nandini12396, bollu, Meinersbur, brzycki, llvm-commits
>>>>
>>>> Differential Revision: https://reviews.llvm.org/D38331
>>>>
>>>> Modified:
>>>> llvm/trunk/include/llvm/Support/GenericDomTree.h
>>>> llvm/trunk/include/llvm/Support/GenericDomTreeConstruction.h
>>>>
>>>> Modified: llvm/trunk/include/llvm/Support/GenericDomTree.h
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/
>>>> Support/GenericDomTree.h?rev=314801&r1=314800&r2=314801&view=diff
>>>> ============================================================
>>>> ==================
>>>> --- llvm/trunk/include/llvm/Support/GenericDomTree.h (original)
>>>> +++ llvm/trunk/include/llvm/Support/GenericDomTree.h Tue Oct 3
>>>> 07:33:41 2017
>>>> @@ -702,28 +702,25 @@ public:
>>>> return;
>>>> }
>>>>
>>>> - unsigned DFSNum = 0;
>>>> -
>>>> SmallVector<std::pair<const DomTreeNodeBase<NodeT> *,
>>>> typename DomTreeNodeBase<NodeT>::const_
>>>> iterator>,
>>>> 32> WorkStack;
>>>>
>>>> const DomTreeNodeBase<NodeT> *ThisRoot = getRootNode();
>>>> -
>>>> + assert((!Parent || ThisRoot) && "Empty constructed DomTree");
>>>> if (!ThisRoot)
>>>> return;
>>>>
>>>> - // Even in the case of multiple exits that form the post dominator
>>>> root
>>>> - // nodes, do not iterate over all exits, but start from the
>>>> virtual root
>>>> - // node. Otherwise bbs, that are not post dominated by any exit
>>>> but by the
>>>> - // virtual root node, will never be assigned a DFS number.
>>>> - WorkStack.push_back(std::make_pair(ThisRoot, ThisRoot->begin()));
>>>> + // Both dominators and postdominators have a single root node. In
>>>> the case
>>>> + // case of PostDominatorTree, this node is a virtual root.
>>>> + WorkStack.push_back({ThisRoot, ThisRoot->begin()});
>>>> +
>>>> + unsigned DFSNum = 0;
>>>> ThisRoot->DFSNumIn = DFSNum++;
>>>>
>>>> while (!WorkStack.empty()) {
>>>> const DomTreeNodeBase<NodeT> *Node = WorkStack.back().first;
>>>> - typename DomTreeNodeBase<NodeT>::const_iterator ChildIt =
>>>> - WorkStack.back().second;
>>>> + const auto ChildIt = WorkStack.back().second;
>>>>
>>>> // If we visited all of the children of this node, "recurse"
>>>> back up the
>>>> // stack setting the DFOutNum.
>>>> @@ -735,7 +732,7 @@ public:
>>>> const DomTreeNodeBase<NodeT> *Child = *ChildIt;
>>>> ++WorkStack.back().second;
>>>>
>>>> - WorkStack.push_back(std::make_pair(Child, Child->begin()));
>>>> + WorkStack.push_back({Child, Child->begin()});
>>>> Child->DFSNumIn = DFSNum++;
>>>> }
>>>> }
>>>>
>>>> Modified: llvm/trunk/include/llvm/Support/GenericDomTreeConstruction.h
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/
>>>> Support/GenericDomTreeConstruction.h?rev=314801&r1=314800&r2
>>>> =314801&view=diff
>>>> ============================================================
>>>> ==================
>>>> --- llvm/trunk/include/llvm/Support/GenericDomTreeConstruction.h
>>>> (original)
>>>> +++ llvm/trunk/include/llvm/Support/GenericDomTreeConstruction.h Tue
>>>> Oct 3 07:33:41 2017
>>>> @@ -1349,6 +1349,102 @@ struct SemiNCAInfo {
>>>> return true;
>>>> }
>>>>
>>>> + // Check if the computed DFS numbers are correct. Note that DFS info
>>>> may not
>>>> + // be valid, and when that is the case, we don't verify the numbers.
>>>> + static bool VerifyDFSNumbers(const DomTreeT &DT) {
>>>> + if (!DT.DFSInfoValid || !DT.Parent)
>>>> + return true;
>>>> +
>>>> + const NodePtr RootBB = IsPostDom ? nullptr : DT.getRoots()[0];
>>>> + const TreeNodePtr Root = DT.getNode(RootBB);
>>>> +
>>>> + auto PrintNodeAndDFSNums = [](const TreeNodePtr TN) {
>>>> + errs() << BlockNamePrinter(TN) << " {" << TN->getDFSNumIn() <<
>>>> ", "
>>>> + << TN->getDFSNumOut() << '}';
>>>> + };
>>>> +
>>>> + // Verify the root's DFS In number. Although DFS numbering would
>>>> also work
>>>> + // if we started from some other value, we assume 0-based
>>>> numbering.
>>>> + if (Root->getDFSNumIn() != 0) {
>>>> + errs() << "DFSIn number for the tree root is not:\n\t";
>>>> + PrintNodeAndDFSNums(Root);
>>>> + errs() << '\n';
>>>> + errs().flush();
>>>> + return false;
>>>> + }
>>>> +
>>>> + // For each tree node verify if children's DFS numbers cover their
>>>> parent's
>>>> + // DFS numbers with no gaps.
>>>> + for (const auto &NodeToTN : DT.DomTreeNodes) {
>>>> + const TreeNodePtr Node = NodeToTN.second.get();
>>>> +
>>>> + // Handle tree leaves.
>>>> + if (Node->getChildren().empty()) {
>>>> + if (Node->getDFSNumIn() + 1 != Node->getDFSNumOut()) {
>>>> + errs() << "Tree leaf should have DFSOut = DFSIn + 1:\n\t";
>>>> + PrintNodeAndDFSNums(Node);
>>>> + errs() << '\n';
>>>> + errs().flush();
>>>> + return false;
>>>> + }
>>>> +
>>>> + continue;
>>>> + }
>>>> +
>>>> + // Make a copy and sort it such that it is possible to check if
>>>> there are
>>>> + // no gaps between DFS numbers of adjacent children.
>>>> + SmallVector<TreeNodePtr, 8> Children(Node->begin(), Node->end());
>>>> + std::sort(Children.begin(), Children.end(),
>>>> + [](const TreeNodePtr Ch1, const TreeNodePtr Ch2) {
>>>> + return Ch1->getDFSNumIn() < Ch2->getDFSNumIn();
>>>> + });
>>>> +
>>>> + auto PrintChildrenError = [Node, &Children, PrintNodeAndDFSNums](
>>>> + const TreeNodePtr FirstCh, const TreeNodePtr SecondCh =
>>>> nullptr) {
>>>> + assert(FirstCh);
>>>> +
>>>> + errs() << "Incorrect DFS numbers for:\n\tParent ";
>>>> + PrintNodeAndDFSNums(Node);
>>>> +
>>>> + errs() << "\n\tChild ";
>>>> + PrintNodeAndDFSNums(FirstCh);
>>>> +
>>>> + if (SecondCh) {
>>>> + errs() << "\n\tSecond child ";
>>>> + PrintNodeAndDFSNums(SecondCh);
>>>> + }
>>>> +
>>>> + errs() << "\nAll children: ";
>>>> + for (const TreeNodePtr Ch : Children) {
>>>> + PrintNodeAndDFSNums(Ch);
>>>> + errs() << ", ";
>>>> + }
>>>> +
>>>> + errs() << '\n';
>>>> + errs().flush();
>>>> + };
>>>> +
>>>> + if (Children.front()->getDFSNumIn() != Node->getDFSNumIn() + 1)
>>>> {
>>>> + PrintChildrenError(Children.front());
>>>> + return false;
>>>> + }
>>>> +
>>>> + if (Children.back()->getDFSNumOut() + 1 !=
>>>> Node->getDFSNumOut()) {
>>>> + PrintChildrenError(Children.back());
>>>> + return false;
>>>> + }
>>>> +
>>>> + for (size_t i = 0, e = Children.size() - 1; i != e; ++i) {
>>>> + if (Children[i]->getDFSNumOut() + 1 != Children[i +
>>>> 1]->getDFSNumIn()) {
>>>> + PrintChildrenError(Children[i], Children[i + 1]);
>>>> + 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) {
>>>> @@ -1521,7 +1617,8 @@ bool Verify(const DomTreeT &DT) {
>>>> SemiNCAInfo<DomTreeT> SNCA(nullptr);
>>>> return SNCA.verifyRoots(DT) && SNCA.verifyReachability(DT) &&
>>>> SNCA.VerifyLevels(DT) && SNCA.verifyNCD(DT) &&
>>>> - SNCA.verifyParentProperty(DT) &&
>>>> SNCA.verifySiblingProperty(DT);
>>>> + SNCA.verifyParentProperty(DT) &&
>>>> SNCA.verifySiblingProperty(DT) &&
>>>> + SNCA.VerifyDFSNumbers(DT);
>>>> }
>>>>
>>>> } // namespace DomTreeBuilder
>>>>
>>>>
>>>> _______________________________________________
>>>> 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/20171003/25e3b540/attachment.html>
More information about the llvm-commits
mailing list