[PATCH] D81494: [GVN] Do not try to split critical edges with indbr preds in loop.

Wed Jun 10 09:20:47 PDT 2020

fhahn added a comment.

In D81494#2083191 <https://reviews.llvm.org/D81494#2083191>, @efriedma wrote:

> There's a separate bailout "COULD NOT PRE LOAD BECAUSE OF INDBR CRITICAL EDGE" earlier, so any block in the CriticalEdgePred has a non-indirectbr terminator.  However, if LoadBB has any predecessor which is an indirectbr, even if it's not the edge we want to split, the call to llvm::SplitCriticalEdge fails.  This is because it's trying to do something more than just split the critical edge.  I guess here, it's trying to preserve LoopSimplify form: %exit is an exit block, so it should only have predecessors inside the loop.

Exactly! Sorry for not making it clear in the description.

> It should be possible to preserve the invariant here.  For the testcase, the resulting function looks something like the following.  We create a new basic block `exit.latch1` to split the edge.  Then we split `exit` at the beginning of the block, before any non-PHI instructions.  Then `exit.latch1` branches to the split block.  (See also llvm::SplitIndirectBrCriticalEdges.)

Right, that should work. I tried, but unfortunately it means moving instructions to a different block & potentially introducing new PHIs and some users of SplitCriticalEdge do not seem to like it (especially the MemoryDependenceAnalysis caching....). I've put up the patch there D81584 <https://reviews.llvm.org/D81584>, but some of the issues are not resolved yet (and I am not sure if it is the best way forward at the moment, given that users seem not prepared for instructions being moved).

> I'm a little skeptical it's a good idea to bail out of GVN, as opposed to fixing SplitCriticalEdge.  If SplitCriticalEdge was private to GVN, maybe it would be okay.  But as it is, I think it's a ticking time bomb if SplitCriticalEdge "randomly" crashes on certain edges due to undocumented constraints.

yes it seems a bit odd to have the check here in GVN. I've put up D81582 <https://reviews.llvm.org/D81582>, which updates SplitCriticalEdge to return nullptr before making any changes, if it would have to split a block with a `indirectbr` terminator. That should ensure we do not run into the crash for other users. GVN itself seems to bail out on blocks it *things* SplitCriticalEdge cannot handle, but that seems a bit fragile in general.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81494