[PATCH] D28767: Do not verify Dominator tree if it has no roots

[Serge Pavlov via llvm-commits]

The fact is that sometimes domtree is not available. If a function is
`available_externally`  all codegen passes are skipped, although they are
marked as required. MachineDominatorTree is also skipped and there is no
domtree at all. Attempt to call `getRoot` for such domtree will obviously
cause a crash. There is no way to get 'right' domtree in this case, so
prior to using it we must either:
1. Check if corresponding pass was indeed run, or
2. Check the domtree state trying to reveal if it is valid.
The first approach was taken in https://reviews.llvm.org/D27190 but it was
not accepted. This fix tries to use variant 2.

Just to emphasize a key point: there are cases when domtree is absent
because the pass that it creates was not run and this behavior is by design.


Thanks,
--Serge

2017-01-25 21:08 GMT+07:00 Daniel Berlin via Phabricator <
reviews at reviews.llvm.org>:

> dberlin added a comment.
>
> This looks, IMHO, very wrong.
> We require domtrees to have roots at all times.
> See getRoot, for example, which says:
>
>   NodeT *getRoot() const {
>     assert(this->Roots.size() == 1 && "Should always have entry node!");
>     return this->Roots[0];
>   }
>
> So if we've created a DominatorTree to call verifyDomTree on,and it
> doesn't have a root, we have in fact, created a broken dom tree and
> verification *should* fail.
>
> I'm not sure why we are creating such a tree, but it's *definitely
> invalid* according to how we work right now.
> All this patch does is paper over that by calling getRoots instead of
> getRoot, because getRoots happens to not assert (we should add a
> non-emptyness assert there too).
>
> I think you need to fix the underlying problem of creating DominatorTrees
> without roots, or start a discussion on llvmdev about whether the empty dom
> tree should be valid.
> (Preferably, the former).
>
> Because so far, we've never considered "the completely empty" domtree to
> be valid, and we haven't audited our functions to make sure they all work
> properly in such a world.
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D28767
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170125/b051b28a/attachment.html>