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

Mon Jan 18 15:05:18 PST 2021

nikic added a comment.

In D94827#2505431 <https://reviews.llvm.org/D94827#2505431>, @lebedev.ri wrote:

> 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.

I'm not sure why my comment was taken to be hostile. All I want to say is that there's a tradeoff here, and I haven't been convinced that it's a good one yet.

I think your point that SimplifyCFG is a utility and may be used in passes that want to preserve DT is a good one. I can see an argument for supporting DT-preservation in SimplifyCFG, while not requiring it in SimplifyCFGPass and not allowing use inside SimplifyCFG itself. Do I understand correctly that DwarfEHPrepare may currently use an outdated dominator tree because SimplifyCFG does not preserve it? If that is the case, then that should certainly be addressed in //some// way.

> 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.

LLVM does not prioritize compile-time over everything. Compile-time regressions are fine as long as they are justified.

I have already stated my concern above. Preserving the DT is one thing, and a 0.5% compile-time regression is not overly problematic. However, the intention behind this change is clearly to also make use of DT inside SimplifyCFG, and that's where things become very problematic.  I did a quick try of fetching the DT (and thus requiring a DTU flush) on every SimplifyCFG iteration (which is what would happen if we implemented your idea regarding unreachable blocks), and here's the result I got: https://llvm-compile-time-tracker.com/compare.php?from=22b68440e1647e16b5ee24b924986207173c02d1&to=879bffddcb45b02f3c819e542a327a5539999ea2&stat=instructions Now we go from 0.5% regression to 2.5%. consumer-typeset sees a 5% regression. The regressions are larger on individual files, e.g. libclamav_pe.c sees as >30% regression. This isn't peanuts. And these are still "average case" regressions on relatively benign benchmarks. You can bet that if you do this, you're going to see pathologically large regressions on generated code.

I feel like this is a valid concern to have, and I don't like the way you're just brushing this off completely. While it seems like common sense to me, I will look into updating our developer policy to make explicit mention of compile-time regressions, next to performance and correctness regressions.

>> - 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.

Oh, sorry, looks like I misunderstood what was going on there. It didn't cross my mind that you would intentionally write incorrect DTU code in order to fix it later.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94827