[PATCH] D61055: [LoopSimplifyCFG] Suppress expensive DomTree verification in LoopSimplifyCFG

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added a comment.

In D61055#1481740 <https://reviews.llvm.org/D61055#1481740>, @yrouban wrote:

> In D61055#1481627 <https://reviews.llvm.org/D61055#1481627>, @dmgreen wrote:
>
> > Hello. I think it's fine to always use DominatorTree::VerificationLevel::Fast, the others are more about checking that the DomTree construction code was correct. Fast will compare against a new tree, so for updates will check the the tree is correct.
>
>
> may be we have to set //Fast// by default instead of //Full//?
>
>   bool DominatorTreeBase<>::verify(VerificationLevel VL = VerificationLevel::Full) const {




In D61055#1481754 <https://reviews.llvm.org/D61055#1481754>, @dmgreen wrote:

> Yeah, sounds like a good idea to me. I almost sugested the same thing. There are a load of tests that should still be using Full (i.e. the domtree construction still needs to be checked with basic/full).
>
> @kuhar what do you think?


It's not entirely true that the Basic/Full level is only ensuring DT construction/updates are correct. The big downside of the Fast mode is that it assumes not only that construction is correct, but also uses custom logic to traverse CFG. We hit an issue with these assumptions recently with a change to Clang CFG that caused it to return nullptrs from `children(node)`, which broke domtree construction.

That being said, I don't think that people should asserting correctness of domtree within transforms, unless there's a strong need. This is generally not cheap, even at the Fast level, and because of that I think that if someone really wants to have a fast domtree verification in an assertion, they should make it explicit. Historically, Fast had been the default, and the running time penalty started to pile up, as many places assumed it's very cheap to perform. It's much easier to realize these assertions are fishy if they are not usually-cheap-but-not-quite by default.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61055/new/

https://reviews.llvm.org/D61055