[PATCH] D129288: [IR] Don't use blockaddresses as callbr arguments

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a comment.

Also, if we eliminate `blockaddress` operands from `callbr`, I wonder if we can revert the bitcode reader+writer changes added recently in:

- 23ec5782c3cc0d77b5e44285e5124cd4a3ffeeef <https://reviews.llvm.org/rG23ec5782c3cc0d77b5e44285e5124cd4a3ffeeef>
- 6baaad740a5abb4bfcff022a8114abb4eea66a2d <https://reviews.llvm.org/rG6baaad740a5abb4bfcff022a8114abb4eea66a2d>

Perhaps while retaining their tests.

> I think this approach will have a nice side effect wrt. inlining:

NVM, that case has `blockaddress`, but not `callbr`. So we should strike

> Allow inlining of callbr (https://github.com/llvm/llvm-project/issues/38908 / https://github.com/ClangBuiltLinux/linux/issues/263)

from the commit description.

Also, I left a few comments like: ``s/!X/!i/`. In 79ebc3b0dd130d759bf637c71c2a6aa039f0bd8f <https://reviews.llvm.org/rG79ebc3b0dd130d759bf637c71c2a6aa039f0bd8f> I meant to clean all of these up. Maybe a few slipped through the cracks, but while you're touching these lines anyways I would appreciate it if we could eradicate uses of X for the labels in our test cases.

Finally, I'm very happy with this cleanup. It seems like a great simplification that fixes many things that made working with `callbr` exceptional; now `callbr` seems like it fits in much better with the IR as we have much less special casing.  We will need some time to test our kernel builds with a change of this nature.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8758
+      auto *CB = cast<CallBrInst>(&Call);
+      OpInfo.CallOperandVal = CB->getBlockAddressForIndirectDest(LabelNo);
+      OpInfo.CallOperand = getValue(OpInfo.CallOperandVal);
----------------
jyknight wrote:
> I'm not sure we actually need to create a BlockAddress in SDAG either. But, fine to leave it for now, to keep the patch size down.
I assume we still need blockaddress in the MIR representation? This change is just creating them much later in the compilation pipeline. Or did I misunderstand your point?


================
Comment at: llvm/test/Analysis/BasicAA/pr52735.ll:14
 
-; CHECK: Both ModRef:  Ptr: i32* %v	<->  callbr void asm "movl $$1, $0", "=*m,X,~{dirflag},~{fpsr},~{flags}"(i32* nonnull elementtype(i32) %v, i8* blockaddress(@foo, %out))
+; CHECK: Both ModRef:  Ptr: i32* %v	<->  callbr void asm "movl $$1, $0", "=*m,!X,~{dirflag},~{fpsr},~{flags}"(i32* nonnull elementtype(i32) %v)
 
----------------
`s/!X/!i/`


================
Comment at: llvm/test/Analysis/BasicAA/pr52735.ll:21
   %0 = bitcast i32* %v to i8*
-  callbr void asm "movl $$1, $0", "=*m,X,~{dirflag},~{fpsr},~{flags}"(i32* elementtype(i32) nonnull %v, i8* blockaddress(@foo, %out))
+  callbr void asm "movl $$1, $0", "=*m,!X,~{dirflag},~{fpsr},~{flags}"(i32* elementtype(i32) nonnull %v)
           to label %asm.fallthrough [label %out]
----------------
`s/!X/!i/`


================
Comment at: llvm/test/CodeGen/X86/callbr-asm-outputs.ll:221
   %1 = call i32 @llvm.read_register.i32(metadata !3)
-  %2 = callbr i32 asm "", "={esp},X,{esp},~{dirflag},~{fpsr},~{flags}"(ptr blockaddress(@test5, %4), i32 %1)
+  %2 = callbr i32 asm "", "={esp},!X,{esp},~{dirflag},~{fpsr},~{flags}"(i32 %1)
           to label %3 [label %4]
----------------
`s/!X/!i/`


================
Comment at: llvm/test/CodeGen/X86/speculation-hardening-sls.ll:67
 entry:
-  callbr void asm sideeffect "jmp $0", "X"(ptr blockaddress(@asmgoto, %d))
+  callbr void asm sideeffect "jmp $0", "!X"()
             to label %asm.fallthrough [label %d]
----------------
`s/!X/!i/`


================
Comment at: llvm/test/Transforms/CodeExtractor/PartialInlinePGOMultiRegion.ll:104
 for.end:                                          ; preds = %for.cond2
-  callbr void asm sideeffect "1: nop\0A\09.quad b, ${0:l}, $$5\0A\09", "X,~{dirflag},~{fpsr},~{flags}"(i8* blockaddress(@bar, %l_yes))
+  callbr void asm sideeffect "1: nop\0A\09.quad b, ${0:l}, $$5\0A\09", "!X,~{dirflag},~{fpsr},~{flags}"()
           to label %asm.fallthrough [label %l_yes]
----------------
`s/!X/!i/`


================
Comment at: llvm/test/Transforms/IROutliner/illegal-callbr.ll:19
 ; CHECK:       fail1:
-; CHECK-NEXT:    [[TMP1:%.*]] = add i32 [[B]], 1
-; CHECK-NEXT:    [[TMP2:%.*]] = add i32 [[B]], 1
+; CHECK-NEXT:    call void @outlined_ir_func_0(i32 [[B]])
 ; CHECK-NEXT:    ret i32 0
----------------
Nice, so `callbr` will no longer inhibit outlining?


================
Comment at: llvm/test/Transforms/LoopStrengthReduce/callbr-critical-edge-splitting2.ll:18-19
 if.then:                                          ; preds = %entry
 ; It's ok to modify this test in the future should be able to split critical
 ; edges here, just noting that this is the critical edge that we care about.
+; CHECK: callbr void asm sideeffect "", "!i"()
----------------
I wonder if this comment is stale from some of your recent changes @nikic ?


================
Comment at: llvm/unittests/IR/InstructionsTest.cpp:1501
 entry:
-  callbr void asm sideeffect "// XXX: ${0:l}", "X"(i8* blockaddress(@foo, %branch_test.exit))
+  callbr void asm sideeffect "// XXX: ${0:l}", "X"(i8* poison)
           to label %land.rhs.i [label %branch_test.exit]
----------------
I think this test doesn't care about having any inputs at all. Consider dropping the arguments, rather than using `i8* poison`.


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

https://reviews.llvm.org/D129288