[PATCH] D81089: Mark InstCombine as not preserving CFG

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added a comment.

In D81089#2086290 <https://reviews.llvm.org/D81089#2086290>, @efriedma wrote:

> Even if we agree the way the PostDominatorTree computes roots in cases involving infinite loops shouldn't depend on the ordering of successors, I'm not sure it's correct to say that instcombine "preserves the CFG".  Even for the regular DominatorTree, I think reordering the successors invalidates DFS numbering (in the sense that it changes the result of getDFSNumIn()).


You can always preserve domtrees by marking dfsnumbers as invalid.

> In terms of how the PostDominatorTree should decide what a "root" is, I'm not sure there's any good way to make that decision. Given an SCC which doesn't have any natural parent, you can pick any node in that SCC as the "fake root", and there isn't any natural reason to favor one node over another.
> 
> Really, the natural choice is to just follow the standard dominator algorithm and say the nodes are unreachable.  Then for passes which want it, we can add a transform to mutate the CFG to introduce fake roots.

Daniel B., Tobias G., and myself had a very long discussion about discovering reverse-unreachable nodes or not in postdominators a few years ago. Infinite loops are fairly rare, but can cause very long path that lead to them to be reverse-unreachable as well. This is rather surprising and introduces corner cases in analyzes/optimizations, which can be avoided by shifting this complexity to postdominator construction, while not breaking algorithm that assume the textbook definition of postdominators. As a consequence, you have to chose some arbitrary way of selecting a fake-entry node from nodes in an scc. Right now this order is whatever children or inverse_children return, but could be based on the order of blocks in a function instead.

The issue I see with introducing fake exits is that postdominators is an analysis used by other analyses, and it would be probably complicated to schedule a transformation in front of any analysis that uses postdominators. Even if an analysis doesn't need postdominance info for reverse-unreachable code, it would see a postdominators analysis results for reverse-reachable nodes if a pass running before it introduced them. Note that this would affect all IR-like things that use postdominators, e.g., llvm ir, clang cfg, machine ir, callgraphs, and third-party ir, including irs outside of the monorepo.


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

https://reviews.llvm.org/D81089