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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

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

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94827