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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

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.

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

  bb:
    br label %loop.header
  
  loop.header:
    store i32 0, i32* %p3, align 4
    indirectbr i8* %p1, [label %loop.latch1, label %loop.latch2]
  
  loop.latch1:
    %c = call i1 @cond()
    br i1 %c, label %loop.header, label %exit.latch1
  
  loop.latch2:
    indirectbr i8* %p2, [label %loop.header, label %exit]
  
  exit.latch1:
    br label %exit.split
  
  exit:
    br label %exit.split
  
  exit.split:
    %x = load i32, i32* %p3, align 4
    %y = add i32 %x, 0
    ret i32 %y

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81494