[PATCH] D94827: [SimplifyCFG] Require and preserve dominator tree

Mon Jan 18 13:19:58 PST 2021

lebedev.ri added a comment.

Wow. So much polarization here.

In D94827#2502435 <https://reviews.llvm.org/D94827#2502435>, @kuhar wrote:

> Wow, this is fantastic. When I first started working on the domtree updater back in 2017, SimplifyGFG seemed like one of the most difficult passes to handle, and I wasn't sure if we ever get there. Very impressive work, @lebedev.ri!

In D94827#2504239 <https://reviews.llvm.org/D94827#2504239>, @mkazantsev wrote:

> This is awesome. And scary. This is scarily awesome. :)

Why thank you.
It ended up being rather boringly trivial after all to me.
The hard part was coming up with the gradual roll-out part (the flag)

As for the rest of comments, let me split this up into parts.

1. Should SimplifyCFG know how to preserve {,Post}DominatorTree
2. Should SimplifyCFG make use of DominatorTree
3. Should SimplifyCFGPass require DominatorTree

As for 1., for the life of me i can not take that question seriously.
Let me point out that SimplifyCFG is an *Utility*, it can be called from other passes,
and as is painfully visible from the diff, both the `DwarfEHPrepare`
and `AMDGPUUnifyDivergentExitNodes` do that already.

By keeping SimplifyCFG illiterate about DomTrees, we force it's users
to also avoid DomTrees to some extent, be it either to just not preserve them,
or recompute them each time after calling SimplifyCFG.

Is that really what we want to be doing?
So if such a question is seriously being asked,
i'd personally suggest to take a step back and **really** think about it.

3 - ignoring the cost of domtree preservation, this is basically free.
As far as i can tell from the tests, it doesn't result in any new domtree creations,
we'd always already calculate domtree after the simplifycfg invocation,
now we'll just do that earlier.

2 - well, yeah, that is a question of the patch, isn't it? :)
Not sure why it's being met with such surprisingly-hostile comments.

In D94827#2505300 <https://reviews.llvm.org/D94827#2505300>, @nikic wrote:

> I'm somewhat skeptical about this change. The motivation seems a bit weak given the costs involved. The costs are:
>
> - Compile-time cost. Your best case estimate puts this at a 0.5% compile-time regression. This is for the case where SimplifyCFG simplify preserves DT, without using it. Once the DT is used, the DTU may be flushed many times while SimplifyCFG iterates. This will drive the average-case cost up, and likely introduce pathological cases as well.

Gentle reminder that i have already previously suggested to codify that
LLVM now prioritizes compile-time over everything.
Or something to that regard, about compile time.
I don't think that has happened yet, so i'm going to largely blatantly ignore this point.

> - Code complexity: At least watching from afar, your path to preserving DT in SimplifyCFG involved quite a few subtle issues, where the first implementation of DT preservation for a given transformation later turned out not to be entirely correct. Given the large number of very different transforms that SimplifyCFG performs, this adds a code complexity and maintenance cost.

1. See above.
2. This is true for practically every change, to some extent, so i'm not sure what the point is here.
3. "Never attribute to malice that which is adequately explained by stupidity", or in this case, it's not that "later turned out not to be entirely correct", this was a *very* intentional roll-out approach, to weed out the cases that lacked test coverage.

> The (listed) benefits are:
>
> - Not processing unreachable blocks. I am doubtful that we'll want to use the DT for this purpose, because that would require flushing the DTU on each SimplifyCFG iteration. You'd probably be better off running removeUnreachableBlocks each time. Though it's not clear that this is even a problem that needs solving.

> - Using DT in FoldBranchToCommonDest. This seems to be the real motivation here. I will have to dive deeper into the code to understand what the problem here is and whether it justifies the costs.

In the mean time, now that i'm done with the groundwork, i'll fix that the currently-preferred way.

================
Comment at: llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp:263
                                 const SimplifyCFGOptions &Options) {
-  assert((!RequireAndPreserveDomTree ||
-          (DT && DT->verify(DominatorTree::VerificationLevel::Full))) &&
----------------
mkazantsev wrote:
> Why remove that? It looks useful to have (maybe over-paranoid) checks that DT is correct at least for the first time. I'd rather keep them for a while unless there is a reason to drop them now.
Note that this checks that the DomTree *provided by the pass manager* is valid.
Do we actually want to check that here?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94827