[PATCH] D81607: BreakCriticalEdges for callbr indirect dests

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers created this revision.
nickdesaulniers added reviewers: craig.topper, jyknight, void.
Herald added subscribers: llvm-commits, hiraditya.
Herald added a project: LLVM.
nickdesaulniers added inline comments.


================
Comment at: llvm/test/Transforms/CallSiteSplitting/callsite-split-callbr.ll:13
+; CHECK:       Top.split:
+; CHECK-NEXT:    call void @callee(i1 false)
+; CHECK-NEXT:    br label [[CALLSITEBB:%.*]]
----------------
noob question: this totally fails for me, even though the test was autogenerated.  FileCheck is complaining that:
```
/android0/llvm-project/llvm/test/Transforms/CallSiteSplitting/callsite-split-callbr.ll:13:15: error: CHECK-NEXT: expected string not found in input
; CHECK-NEXT: call void @callee(i1 false)
              ^
...
           9: Top.split: ; preds = %Top
next:13'0                X~~~~~~~~~~~~~ error: no match found
          10:  call void @callee(i1 false)
next:13'0     ~~~~~~~~~~~~~~~~~~~
next:13'1      ?                           possible intended match
...
```
which makes no sense to me.  Am I holding it wrong?


llvm::SplitEdge was failing an assertion that the BasicBlock only had
one successor (for BasicBlocks terminated by CallBrInst, we typically
have multiple successors).  It was surprising that the earlier call to
SplitCriticalEdge did not handle the critical edge (there was an early
return).  Removing that triggered another assertion relating to creating
a BlockAddress for a BasicBlock that did not (yet) have a parent, which
is a simple order of operations issue in llvm::SplitCriticalEdge (a
freshly constructed BasicBlock must be inserted into a Function's basic
block list to have a parent).

Thanks to @nathanchance for the report.
Fixes: https://github.com/ClangBuiltLinux/linux/issues/1018


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D81607

Files:
  llvm/lib/Transforms/Utils/BreakCriticalEdges.cpp
  llvm/test/Transforms/CallSiteSplitting/callsite-split-callbr.ll


Index: llvm/test/Transforms/CallSiteSplitting/callsite-split-callbr.ll
===================================================================
--- /dev/null
+++ llvm/test/Transforms/CallSiteSplitting/callsite-split-callbr.ll
@@ -0,0 +1,53 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -callsite-splitting -S -o - < %s | FileCheck %s
+
+; Check that we can split the critical edge between Top and CallSiteBB, and
+; rewrite the first callbr's indirect destination correctly.
+
+define void @caller() {
+; CHECK-LABEL: @caller(
+; CHECK-NEXT:  Top:
+; CHECK-NEXT:    callbr void asm sideeffect "", "X,~{dirflag},~{fpsr},~{flags}"(i8* blockaddress(@caller, [[TOP_SPLIT:%.*]]))
+; CHECK-NEXT:    to label [[NEXTCOND:%.*]] [label %Top.split]
+; CHECK:       Top.split:
+; CHECK-NEXT:    call void @callee(i1 false)
+; CHECK-NEXT:    br label [[CALLSITEBB:%.*]]
+; CHECK:       NextCond:
+; CHECK-NEXT:    br label [[NEXTCOND_SPLIT:%.*]]
+; CHECK:       NextCond.split:
+; CHECK-NEXT:    call void @callee(i1 true)
+; CHECK-NEXT:    br label [[CALLSITEBB]]
+; CHECK:       CallSiteBB:
+; CHECK-NEXT:    [[PHI:%.*]] = phi i1 [ false, [[TOP_SPLIT]] ], [ true, [[NEXTCOND_SPLIT]] ]
+; CHECK-NEXT:    callbr void asm sideeffect "", "r,X,~{dirflag},~{fpsr},~{flags}"(i1 [[PHI]], i8* blockaddress(@caller, [[END2:%.*]]))
+; CHECK-NEXT:    to label [[END:%.*]] [label %End2]
+; CHECK:       End:
+; CHECK-NEXT:    ret void
+; CHECK:       End2:
+; CHECK-NEXT:    ret void
+;
+Top:
+  callbr void asm sideeffect "", "X,~{dirflag},~{fpsr},~{flags}"(i8* blockaddress(@caller, %CallSiteBB))
+  to label %NextCond [label %CallSiteBB]
+
+NextCond:
+  br label %CallSiteBB
+
+CallSiteBB:
+  %phi = phi i1 [0, %Top],[1, %NextCond]
+  call void @callee(i1 %phi)
+  callbr void asm sideeffect "", "r,X,~{dirflag},~{fpsr},~{flags}"(i1 %phi, i8* blockaddress(@caller, %End2))
+  to label %End [label %End2]
+
+End:
+  ret void
+End2:
+  ret void
+}
+
+define void @callee(i1 %b) {
+; CHECK-LABEL: @callee(
+; CHECK-NEXT:    ret void
+;
+  ret void
+}
Index: llvm/lib/Transforms/Utils/BreakCriticalEdges.cpp
===================================================================
--- llvm/lib/Transforms/Utils/BreakCriticalEdges.cpp
+++ llvm/lib/Transforms/Utils/BreakCriticalEdges.cpp
@@ -150,10 +150,6 @@
   // it in this generic function.
   if (DestBB->isEHPad()) return nullptr;
 
-  // Don't split the non-fallthrough edge from a callbr.
-  if (isa<CallBrInst>(TI) && SuccNum > 0)
-    return nullptr;
-
   if (Options.IgnoreUnreachableDests &&
       isa<UnreachableInst>(DestBB->getFirstNonPHIOrDbgOrLifetime()))
     return nullptr;
@@ -165,14 +161,14 @@
   BranchInst *NewBI = BranchInst::Create(DestBB, NewBB);
   NewBI->setDebugLoc(TI->getDebugLoc());
 
-  // Branch to the new block, breaking the edge.
-  TI->setSuccessor(SuccNum, NewBB);
-
   // Insert the block into the function... right after the block TI lives in.
   Function &F = *TIBB->getParent();
   Function::iterator FBBI = TIBB->getIterator();
   F.getBasicBlockList().insert(++FBBI, NewBB);
 
+  // Branch to the new block, breaking the edge.
+  TI->setSuccessor(SuccNum, NewBB);
+
   // If there are any PHI nodes in DestBB, we need to update them so that they
   // merge incoming values from NewBB instead of from TIBB.
   {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D81607.269923.patch
Type: text/x-patch
Size: 3316 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200610/803eab22/attachment.bin>