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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:5512
 
+      // Upgrade explicit blockaddress arguments to label constraints.
+      unsigned LabelNo = 0;
----------------
jyknight wrote:
> I'd put `if (isa<InlineAsm>(Callee))` around this whole upgrade section. While we only currently support inlineasm calee, the reader code is otherwise agnostic to the type of the callee, and it seems better to preserve that.
> 
> Also, this upgrade code seems potentially problematic -- a user could write the C code:
> `int foo() { label: asm goto("" : : "r"(&&label) : : label2); return 1; label2: return 2; }`
> which turns into
> ```
>   callbr void asm sideeffect "", "r,i,~{dirflag},~{fpsr},~{flags}"(i8* blockaddress(@foo, %2), i8* blockaddress(@foo, %4)) #1
>           to label %3 [label %4]
> ```
> and this would error out on it.
> 
> (I'm not sure _why_ a user would write that, but we shouldn't fail in the bitcode reader if they do!)
> 
> I think it needs to:
> 1. Only do anything if the constraint string doesn't contain label constraints (so we don't run this code on new bitcode).
> 2. Only validate and remove the last IndirectDests.size() arguments -- not other blockaddress values that may appear earlier in the arglist.
> 
> (And of course, a test-case. Are there any test-cases for this code? I didn't see one.)
I've changed the code to only upgrade the trailing arguments. There's test coverage for this in llvm/test/Bitcode/callbr.ll.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8730
     if (OpInfo.hasArg()) {
-      OpInfo.CallOperandVal = Call.getArgOperand(ArgNo);
       OpInfo.CallOperand = getValue(OpInfo.CallOperandVal);
----------------
jyknight wrote:
> Why is this removed?
This was a leftover from a previous version.

It's rather suspicious that this doesn't break anything -- apparently, most of the code here is redundant. I've prepared a patch to clean it up (https://gist.github.com/nikic/219eec68f61b02fa4b22024307a50f84) and rely on `TargetLowering::ParseConstraints()` instead. Once I land that the label handling code added here will also move into that function.


================
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
----------------
nickdesaulniers wrote:
> Nice, so `callbr` will no longer inhibit outlining?
Yes, in the sense that the indirect dest blocks are no longer address taken, so can be outlined. The callbr itself can't be outlined yet, because the outliner generally doesn't support outlining terminators.


================
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"()
----------------
nickdesaulniers wrote:
> I wonder if this comment is stale from some of your recent changes @nikic ?
>From what I can tell, we don't perform any critical edge splitting here (we do perform it in the function below though).


================
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]
----------------
nickdesaulniers wrote:
> I think this test doesn't care about having any inputs at all. Consider dropping the arguments, rather than using `i8* poison`.
This was a leftover from a previous patch.


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

https://reviews.llvm.org/D129288