[SelectionDAGBuilder] drop special handling for CallBr

[nickdesaulniers]

Summary

Now that indirect destinations are using "i" constraints rather than "X"
we no longer need this special handling.

Add a test case that was previously ICE'ing.

Reported-by: kernel test robot <[e-mail]>
Fixes: https://github.com/ClangBuiltLinux/linux/issues/1512

[nickdesaulniers]

• I'm just wrestling with arc at this point

[nickdesaulniers]

• phab sux

[nickdesaulniers]

llvm/test/CodeGen/X86/callbr-asm-bb-exports.ll

23

@craig.topper can you triple check this change to this whole file carefully, please?

I'm not sure TBH why my change to SelectionDAGBuilder changed this. I'm also not sure of the original intent of the test. It's running -debug-only=isel which prints A LOT of different phases; I'm not sure which it was originally testing.

I'm not sure why we get an BlockAddress now; I don't really understand the output from selectionDAG here. (t16 looks unused to me, IIUC).

[jyknight]

OK, I do think that special case definitely needs to be deleted. It's assuming that the block args are in a particular place in the argument list of the callbr -- the place Clang put them -- and then assigned special behavior based on that location. But I think it shouldn't do so -- they should be able to placed anywhere in the arglist.

The question then, is why this code needs to exist? It's accomplishing two things:

1. If we get rid of this code but continue to use the "X" constraint, llvm won't emit an immediate symbol reference (today), rather, it will emit a register reference instead. Even though X means "accept anything", and therefore a register is thus supposedly acceptable, it seems like it should probably prefer emitting an immediate if it can. (GCC does emit it as an immediate here)
2. Emits a TargetBlockAddress instead of BlockAddress. (I'm not sure what the use of TargetBlockAddress accomplishes? I _think_ it should be okay to use BlockAddress instead, but I'm not 100% sure...)

Assuming the point 2 is OK, then I think ideally, we'd flip the order of things, and do:
a. Cause "X" constraint to emit an immediate symbol reference if it can, rather than copying to a register first -- and remove this special case. (Fixes bug.)
b. Updates tests and change clang to emit "i" instead of "X" constraints. (Clarifies intent that an immediate is actually required).

[craig.topper]

llvm/test/CodeGen/X86/callbr-asm-bb-exports.ll

23

I think the important thing was that the t22 CopyFromReg chain output is used by the inlineasm_br.

I'm guessing the BlockAddress is being created by the getValue call in the else that is now reachable?

Maybe my regex to ignore most of the inlineasm_br line is hiding too much.

[nickdesaulniers]

In D115409#3190589, @jyknight wrote:

OK, I do think that special case definitely needs to be deleted. It's assuming that the block args are in a particular place in the argument list of the callbr -- the place Clang put them -- and then assigned special behavior based on that location. But I think it shouldn't do so -- they should be able to placed anywhere in the arglist.

The question then, is why this code needs to exist? It's accomplishing two things:

1. If we get rid of this code but continue to use the "X" constraint, llvm won't emit an immediate symbol reference (today), rather, it will emit a register reference instead. Even though X means "accept anything", and therefore a register is thus supposedly acceptable, it seems like it should probably prefer emitting an immediate if it can. (GCC does emit it as an immediate here)

Done in https://reviews.llvm.org/D115688. Closing this in favor of that. PTAL.

2. Emits a TargetBlockAddress instead of BlockAddress. (I'm not sure what the use of TargetBlockAddress accomplishes? I _think_ it should be okay to use BlockAddress instead, but I'm not 100% sure...)

Not sure about this either way. Perhaps something for reviewers to consider.

Assuming the point 2 is OK, then I think ideally, we'd flip the order of things, and do:
a. Cause "X" constraint to emit an immediate symbol reference if it can, rather than copying to a register first -- and remove this special case. (Fixes bug.)
b. Updates tests and change clang to emit "i" instead of "X" constraints. (Clarifies intent that an immediate is actually required).

Now for me to rebase D115410, D115311, and D115471 onto D115688.

llvm/test/CodeGen/X86/callbr-asm-bb-exports.ll

23

Sounds like I should pre-commit (before D115688) a change that unrolls the regex?

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

void SelectionDAGBuilder::visitInlineAsm(const CallBase &Call,
 
   // First Pass: Calculate HasSideEffects and ExtraFlags (AlignStack,
   // AsmDialect, MayLoad, MayStore).
   bool HasSideEffect = IA->hasSideEffects();
   ExtraFlags ExtraInfo(Call);
 
   unsigned ArgNo = 0;   // ArgNo - The argument of the CallInst.
   unsigned ResNo = 0;   // ResNo - The result number of the next output.
-  unsigned NumMatchingOps = 0;
   for (auto &T : TargetConstraints) {
     ConstraintOperands.push_back(SDISelAsmOperandInfo(T));
     SDISelAsmOperandInfo &OpInfo = ConstraintOperands.back();
 
     // Compute the value type for each operand.
     if (OpInfo.Type == InlineAsm::isInput ||
         (OpInfo.Type == InlineAsm::isOutput && OpInfo.isIndirect)) {
       OpInfo.CallOperandVal = Call.getArgOperand(ArgNo++);
 
-      // Process the call argument. BasicBlocks are labels, currently appearing
-      // only in asm's.
-      if (isa<CallBrInst>(Call) &&
-          ArgNo - 1 >= (cast<CallBrInst>(&Call)->arg_size() -
-                        cast<CallBrInst>(&Call)->getNumIndirectDests() -
-                        NumMatchingOps) &&
-          (NumMatchingOps == 0 ||
-           ArgNo - 1 <
-               (cast<CallBrInst>(&Call)->arg_size() - NumMatchingOps))) {
-        const auto *BA = cast<BlockAddress>(OpInfo.CallOperandVal);
-        EVT VT = TLI.getValueType(DAG.getDataLayout(), BA->getType(), true);
-        OpInfo.CallOperand = DAG.getTargetBlockAddress(BA, VT);
-      } else if (const auto *BB = dyn_cast<BasicBlock>(OpInfo.CallOperandVal)) {
+      if (const auto *BB = dyn_cast<BasicBlock>(OpInfo.CallOperandVal)) {
         OpInfo.CallOperand = DAG.getBasicBlock(FuncInfo.MBBMap[BB]);
       } else {
         OpInfo.CallOperand = getValue(OpInfo.CallOperandVal);
       }
 
       EVT VT = OpInfo.getCallOperandValEVT(*DAG.getContext(), TLI,
                                            DAG.getDataLayout());
       OpInfo.ConstraintVT = VT.isSimple() ? VT.getSimpleVT() : MVT::Other;
if (OpInfo.Type == InlineAsm::isInput ||
         OpInfo.ConstraintVT = TLI.getAsmOperandValueType(
             DAG.getDataLayout(), Call.getType()).getSimpleVT();
       }
       ++ResNo;
     } else {
       OpInfo.ConstraintVT = MVT::Other;
     }
 
-    if (OpInfo.hasMatchingInput())
-      ++NumMatchingOps;
-
     if (!HasSideEffect)
       HasSideEffect = OpInfo.hasMemory(TLI);
 
     // Determine if this InlineAsm MayLoad or MayStore based on the constraints.
     // FIXME: Could we compute this on OpInfo rather than T?
 
     // Compute the constraint code and ConstraintType to use.
     TLI.ComputeConstraintToUse(T, SDValue());

llvm/test/CodeGen/X86/callbr-asm-bb-exports.ll

 ; REQUIRES: asserts
 ; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -o /dev/null -debug-only=isel 2>&1 | FileCheck %s
 
 ; Make sure we emit the basic block exports and the TokenFactor before the
 ; inlineasm_br. Not sure how to get a MachineIR change so this reads the debug
 ; output from SelectionDAG.
 
-; CHECK: t0: ch = EntryToken
+; CHECK: Initial selection DAG: %bb.0 'test:entry'
+; CHECK-NEXT: SelectionDAG has 33 nodes:
+; CHECK-NEXT: t0: ch = EntryToken
+; CHECK-NEXT: t16: i64 = BlockAddress<@test, %fail> 0
 ; CHECK-NEXT: t4: i32,ch = CopyFromReg t0, Register:i32 %3
 ; CHECK-NEXT: t10: i32 = add t4, Constant:i32<1>
 ; CHECK-NEXT: t12: ch = CopyToReg t0, Register:i32 %0, t10
 ; CHECK-NEXT: t6: i32,ch = CopyFromReg t0, Register:i32 %4
 ; CHECK-NEXT: t13: i32 = add t6, Constant:i32<1>
 ; CHECK-NEXT: t15: ch = CopyToReg t0, Register:i32 %1, t13
 ; CHECK-NEXT: t17: ch = TokenFactor t12, t15
 ; CHECK-NEXT: t2: i32,ch = CopyFromReg t0, Register:i32 %2
 ; CHECK-NEXT: t8: i32 = add t2, Constant:i32<4>
 ; CHECK-NEXT: t22: ch,glue = CopyToReg t17, Register:i32 %5, t8
-; CHECK-NEXT: t29: ch,glue = inlineasm_br t22, {{.*}}, t22:1
+; CHECK-NEXT: t30: ch,glue = inlineasm_br t22, {{.*}}, t22:1
+; CHECK-NEXT: t32: ch = br t30, BasicBlock:ch
 
 define i32 @test(i32 %a, i32 %b, i32 %c) {
 entry:
   %0 = add i32 %a, 4
   %1 = add i32 %b, 1
   %2 = add i32 %c, 1
   callbr void asm "xorl $0, $0; jmp ${1:l}", "r,i,~{dirflag},~{fpsr},~{flags}"(i32 %0, i8* blockaddress(@test, %fail)) to label %normal [label %fail]
 
 normal:
   ret i32 %1
 
 fail:
   ret i32 %2
 }

llvm/test/CodeGen/X86/callbr-asm-outputs.ll

 
 label_true:                                       ; preds = %asm.fallthrough, %entry
   br label %return
 
 return:                                           ; preds = %entry, %asm.fallthrough, %label_true, %asm.fallthrough2
   %retval.0 = phi i32 [ %add, %asm.fallthrough2 ], [ -2, %label_true ], [ -1, %asm.fallthrough ], [ -1, %entry ]
   ret i32 %retval.0
 }
+
+; Test5 - test that we don't rely on a positional constraint. ie. +r in
+; GCCAsmStmt being turned into "={esp},0" since after D87279 they're turned
+; into "={esp},{esp}". This previously caused an ICE; this test is more so
+; about avoiding another ICE than what the actual output is.
+define dso_local void @test5() {
+; CHECK-LABEL: test5:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    #APP
+; CHECK-NEXT:    #NO_APP
+; CHECK-NEXT:  .Ltmp6: # Block address taken
+; CHECK-NEXT:  # %bb.1:
+; CHECK-NEXT:    retl
+  %1 = call i32 @llvm.read_register.i32(metadata !3)
+  %2 = callbr i32 asm "", "={esp},i,{esp},~{dirflag},~{fpsr},~{flags}"(i8* blockaddress(@test5, %4), i32 %1)
+          to label %3 [label %4]
+
+3:
+  call void @llvm.write_register.i32(metadata !3, i32 %2)
+  br label %4
+
+4:
+  ret void
+}
+
+declare i32 @llvm.read_register.i32(metadata)
+declare void @llvm.write_register.i32(metadata, i32)
+!3 = !{!"esp"}