[PATCH] D54730: [DomTree] Fix order of domtree updates in MergeBlockIntoPredecessor.

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added a comment.

In https://reviews.llvm.org/D54730#1304696, @efriedma wrote:

> Fundamentally, does this have to be an expensive operation?  It seems like there should be some way to keep a "local" operation like this from propagating across the entire tree.


Historically, this and other cases were handled by manual tree modifications based on local tree properties. The problem was that there were dozens small and very difficult to detect bugs in that manual update code that I replaced with automatic incremental updates over the summer 2017. Even though most of them seemed simple and reasonably documented, there are non-trivial edge cases (e.g., inside irreducible control flow) that people missed in virtually all the code that performed manual updates. The consensus was that we'd rather take small performance hit and use a single correct updating algorithm only falling back to manual updates when running time degraded significantly.

So far we haven't had many people complaining about performance of the incremental updater; it was actually quite surprising that the first optimized version performed so well in practice. There were a handful performance problems in various pieces of the optimizer that we either fixed or mitigated, but we were only able to do so with good repros. It's easy to create big and weird graphs (like ladder graphs), but we hardly ever have to deal with IR like this coming from a real frontend. I'd be great if you could produce some real repro out of your code. It should be enough to take a snapshot of the IR just before you start scheduling updates in this pass, and recreate it with branching only -- we don't need any non-branching instructions or real value names.


Repository:
  rL LLVM

https://reviews.llvm.org/D54730