[SelectionDAG] treat X constrained labels as i for asm

[nickdesaulniers]

Summary

Completely rework how we handle X constrained labels for inline asm.

X should really be treated as i. Then existing tests can be moved to use
i D115410 and clang can just emit i D115311. (D115410 and D115311 are
callbr, but this can be done for label inputs, too).

Coincidentally, this simplification solves an ICE uncovered by D87279
based on assumptions made during D69868.

This is the third approach considered. See also discussions v1 (D114895)
and v2 (D115409).

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

[nickdesaulniers]

• add more tests, rework parts of TargetLowering::LowerAsmOperandForConstraint

[nickdesaulniers]

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

4605–4629

Note to reviewers: I got a little carried away refactoring the existing code here. If it's too distracting from this review, I'm happy to pre-commit this hunk.

llvm/test/CodeGen/AArch64/inlineasm-X-constraint.ll

124

Note to reviewers, the below code modified looks like it was originally a typo, based on this comment.

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

20

Note to reviewers: I'm happy to pre-commit removing this regex so that we can better see here what changed. Please let me know if that would be worthwhile.

[jyknight]

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

1686

How does BasicBlock get here, vs BlockAddress?

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

5094–5095

I'm still confused as to why this deals in both BlockAddress and BasicBlock types -- why aren't the values all BlockAddresses?

llvm/test/CodeGen/AArch64/inlineasm-X-constraint.ll

130

Can now "CHECK: bl bb"?

llvm/test/CodeGen/PowerPC/ppc64-inlineasm-clobber.ll

84 ↗

(On Diff #394402)

The asm fallthrough grew a label it didn't need before? And...that affected the need to save/restore lr or something?

Hmm...wait...it sure looks like the expected code in this test was demonstrating a bug...! It wasn't saving/restoring lr (mflr/mtlr) around the callbr, but the callbr said it clobbers lr! That doesn't seem right -- despite the test asserting it...

OK...but why did this change fix that?

[nickdesaulniers]

• update aarch64 test to check branch target

[nickdesaulniers]

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

1686

Note that this method also handles BlockAddresses above in the conditional for isa<Constant> (BlockAddress isa<Constant>; BasicBlock !isa<Constant> on line 1622.

Without this check, llvm/test/CodeGen/X86/asm-block-labels.ll asserts:

Can't get register for value!
UNREACHABLE executed at /android0/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1685!
...
2.	Running pass 'X86 DAG->DAG Instruction Selection' on function '@bar'
...
 #7 0x00000000030f2e89 llvm::SelectionDAGBuilder::getValueImpl(llvm::Value const*) (/android0/llvm-project/llvm/build/bin/llc+0x30f2e89)
 #8 0x00000000030f1bbf llvm::SelectionDAGBuilder::getValue(llvm::Value const*) (/android0/llvm-project/llvm/build/bin/llc+0x30f1bbf)
 #9 0x00000000030fcd7b llvm::SelectionDAGBuilder::visitInlineAsm(llvm::CallBase const&, llvm::BasicBlock const*) (/android0/llvm-project/llvm/build/bin/llc+0x30fcd7b)

Note that @bar is only composed of call (no callbr); one statement has a label operand.

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

5094–5095

llvm/test/CodeGen/X86/asm-block-labels.ll with assert without the check for BasicBlock.

Value type is non-standard value, Other.
UNREACHABLE executed at /android0/llvm-project/llvm/include/llvm/Support/MachineValueType.h:865!
...
2.	Running pass 'X86 DAG->DAG Instruction Selection' on function '@bar'
...
 #8 0x00000000030d9abc getCopyToParts(llvm::SelectionDAG&, llvm::SDLoc const&, llvm::SDValue, llvm::SDValue*, unsigned int, llvm::MVT, llvm::Value const*, llvm::Optional<unsigned int>, llvm::ISD::NodeType) SelectionDAGBuilder.cpp:0:0
 #9 0x00000000030d9207 llvm::RegsForValue::getCopyToRegs(llvm::SDValue, llvm::SelectionDAG&, llvm::SDLoc const&, llvm::SDValue&, llvm::SDValue*, llvm::Value const*, llvm::ISD::NodeType) const (/android0/llvm-project/llvm/build/bin/llc+0x30d9207)
#10 0x00000000030feb26 llvm::SelectionDAGBuilder::visitInlineAsm(llvm::CallBase const&, llvm::BasicBlock const*) (/android0/llvm-project/llvm/build/bin/llc+0x30feb26)
#11 0x00000000030dca47 llvm::SelectionDAGBuilder::visit(llvm::Instruction const&) (/android0/llvm-project/llvm/build/bin/llc+0x30dca47)

ie. call (and technically callbr) can accept a label operand; not just a blockaddress. Why are label operands distinct from blockaddresses? /me shrugs.

llvm/test/CodeGen/PowerPC/ppc64-inlineasm-clobber.ll

84 ↗

(On Diff #394402)

+ @stefanp @nemanjai

[void]

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

8570–8571

The old code uses DAG.getTargetBlockAddress(...). This new code uses DAG.getBlockAddress(...). What's the difference?

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

4605–4629

This is fine, but in the future it might be best to commit refactoring in a separate NFC commit.

[nickdesaulniers]

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

8570–8571

BlockAddressSDNode::classof is interesting; there's one node with two possible opcodes; ISD::BlockAddress and ISD::TargetBlockAddress.

SelectionDAG:: getTargetBlockAddress is also interesting, as it is implemented in terms of a call to SelectionDAG::getBlockAddress with the optional isTarget parameter set to true (defaulting to false otherwise).

So this change as is (Diff 395500) is not setting the isTarget flag, which just changes the opcode of the SDNode. The only places in the codebase where I observe ISD::TargetBlockAddress being handled differently from ISD::BlockAddress are:

1. TargetLowering::ComputeConstraintToUse which I actually unify in this diff, see below.
2. SelectionDAGISel::SelectCodeCommon. I'm not quite sure yet what this code is doing, will read through it a couple times now. Perhaps that's what's causing the difference in llvm/test/CodeGen/PowerPC/ppc64-inlineasm-clobber.ll and llvm/test/CodeGen/X86/callbr-asm-bb-exports.ll? That said PPCTargetLowering::PPCTargetLowering is doing something "Custom" with ISD::BlockAddress but not ISD::TargetBlockAddress. I wonder if it should be? Rephrased: should PPCTargetLowering::LowerOperation have an additional case ISD::TargetBlockAddress that also dispatches to PPCTargetLowering::LowerBlockAddress AND PPCTargetLowering::PPCTargetLowering mark ISD::TargetBlockAddress as having custom handling? Lots (all?) backends seem to follow this pattern though? Messing with that creates a pretty spectacular failure though.
3. SDNode::getOperationName which converts an SDNode to a std::string based on the opcode. Not interesting/relevant.

---

Just another thought. If we're ok matching GCC with D115471, I could actually rebase this on that, then restore the ArgNo based checking here back to what it was when asm goto support was first introduced. Then we'd still be using TargetBlockAddresses rather than BlockAddresses.

[void]

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

8570–8571

There is a use of it in the .td files:

$ grep tblockaddress
llvm/include/llvm/Target/TargetSelectionDAG.td:def tblockaddress: SDNode<"ISD::TargetBlockAddress",  SDTPtrLeaf, [],
  ...

My concern is that changing this will affect code gen in peculiar ways. Is it possible to retain the previous use of the TargetBlockAddress? Or could we transmography the node from a ISD::BlockAddress to ISD::TargetBlockAddress?

[craig.topper]

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

8570–8571

For more information. The "Target" version of nodes are ignored by the isel step of SelectionDAG. They are considered already selected and ignored. This is what the code in SelectNodeCommon is doing.

BlockAddress needs to be replaced before isel finishes, TargetBlockAddress is the only version that can go through InstrEmitter to make MachineIR.

I think usually TargetBlockAddress should be created from BlockAddress as part of lowering. Maybe we were doing something different for asm goto because we weren't really using blockaddress the way it was originally intended? I don't recall if I wrote that part of the code or if I inherited it when I took over the asm goto patch.

[nickdesaulniers]

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

8570–8571

That said PPCTargetLowering::PPCTargetLowering is doing something "Custom" with ISD::BlockAddress but not ISD::TargetBlockAddress. I wonder if it should be?

Ok, so IIUC, it looks like the backend specific TargetLowering methods "raise" or "promote" ISD::BlockAddresses to ISD::TargetBlockAddresses. I suspect it's more correct for this part of ISEL (SelectionDAGBuilder::visitInlineAsm) to lower to ISD::BlockAddress and then let the target specific backends raise those to ISD::TargetBlockAddresses correctly. But I will admit I'm pretty unfamiliar with all this, so I probably don't know what I'm talking about.

[void]

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

8570–8571

I don't recall if I wrote that part of the code or if I inherited it when I took over the asm goto patch.

I think you're on the git blame output for that one. ;-)

Okay, I'll back off on this. It sounds like it was more an attempt to say "No, we're sure we have the correct thing here."

[nickdesaulniers]

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

8570–8571

Is it possible to retain the previous use of the TargetBlockAddress?

The issue is in disambiguating between BlockAddress input operands that are just inputs vs indirect destinations of the asm goto. See my previous approaches https://reviews.llvm.org/D114895, https://reviews.llvm.org/D115409 to this. I think https://reviews.llvm.org/D115471 further complicates this since it changes the ordering of the arguments to the asm.

If I rebased this diff on D115471, then I could retain the use of TargetBlockAddress here, simply dropping the NumMatchingOps logic.

[nickdesaulniers]

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

5094–5095

So it looks like a label is a Type that a Value can have, while blockaddress is a Constant Value, IIUC. Still, this holds:

call (and technically callbr) can accept a label operand; not just a blockaddress.

See also the above relationship between ISD::TargetBlockAddress and ISD::BlockAddress; they're the same SDNode (BlockAddressSDNode). This hunk of this change is unifying the operands rather than relying one of the possible opcodes for this type of SDNode.

[nickdesaulniers]

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

8570–8571

For more information. The "Target" version of nodes are ignored by the isel step of SelectionDAG. They are considered already selected and ignored. This is what the code in SelectNodeCommon is doing.

BlockAddress needs to be replaced before isel finishes, TargetBlockAddress is the only version that can go through InstrEmitter to make MachineIR.

I think usually TargetBlockAddress should be created from BlockAddress as part of lowering. Maybe we were doing something different for asm goto because we weren't really using blockaddress the way it was originally intended? I don't recall if I wrote that part of the code or if I inherited it when I took over the asm goto patch.

That seems to match up with what @sunfish mentions on discord:

the Target* constants are for things which are legalized, such as immediates which are known to be directly lowerable to machine code. The non-Target form is the pre-legalized version which may need to be legalized, into TargetBlockAddress or something else.

So while yes, we know that the indirect destinations are immediates, this change here of mine is forcing the blockaddresses to now be legalized whereas before my change they were not. Perhaps that's slightly slower, but it resolves the ambiguity between indirect destination blockaddresses, and non-indirect destination blockaddresses.

[nickdesaulniers]

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

5094–5095

call (and technically callbr) can accept a label operand; not just a blockaddress.

Scratch and technically callbr; Verifier::visitCallBrInst validates that we do not permit naked label Values in the arg list for callbr IIUC.

To re-summarize the answer to the question

why aren't the values all BlockAddresses?

Because call instructions can have arguments that are labels; not just blockaddresses, as observed in llvm/test/CodeGen/X86/asm-block-labels.ll.

llvm/test/CodeGen/PowerPC/ppc64-inlineasm-clobber.ll

84 ↗

(On Diff #394402)

So I thought this may be because of the changes to SelectionDAGBuilder::visitInlineAsm here (in D115688 Diff 395500).

As a test, I rebased locally D115471 to appear first, rather than last in the patch set, rebasing this on that. That allowed me to remove the NumMatching logic, but keep basically the same version as when callbr support was initially added; if the indirect dests are always the final inputs, the ambiguity alluded to in this patch no longer exists.

I know you weren't enthused by D115471, but I think landing that first actually simplifies this patch (the diff to llvm/test/CodeGen/X86/callbr-asm-bb-exports.ll here goes away, and we return SelectionDAGBuilder::visitInlineAsm back to how it was prior to output support). For that reason, I'd like you to reconsider D115471.

Hmm...wait...it sure looks like the expected code in this test was demonstrating a bug...! It wasn't saving/restoring lr (mflr/mtlr) around the callbr, but the callbr said it clobbers lr! That doesn't seem right -- despite the test asserting it...

I left an additional comment on https://reviews.llvm.org/D101657.

OK...but why did this change fix that?

I'm still not precisely sure why this change tickled that, but perhaps let's fix that first? https://reviews.llvm.org/D116424

[jyknight]

LG, with a few minor comments.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

1686

Taking the label directly as a value seems like something we should not be supporting at all. Yet, since I guess we already did, OK, fine.

llvm/test/CodeGen/AArch64/inlineasm-X-constraint.ll

130

Still outstanding: would be nice to include the target in the check line, verifying that it's an immediate not a register.

llvm/test/CodeGen/PowerPC/ppc64-inlineasm-clobber.ll

84 ↗

(On Diff #394402)

I assume the diff in this file has gone away now?

[nickdesaulniers]

• rebase on D116424 and a few other recent changes (2aed08131df56, e4d1779990672, 0312fe2901122)

[nickdesaulniers]

• rebase on pre-committed TargetLowering refactor

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

RegsForValue RFV(*DAG.getContext(), TLI, DAG.getDataLayout(), InReg,
                      Inst->getType(), None);
     SDValue Chain = DAG.getEntryNode();
     return RFV.getCopyFromRegs(DAG, FuncInfo, getCurSDLoc(), Chain, nullptr, V);
   }
 
   if (const MetadataAsValue *MD = dyn_cast<MetadataAsValue>(V)) {
     return DAG.getMDNode(cast<MDNode>(MD->getMetadata()));
   }
+  if (const auto *BB = dyn_cast<BasicBlock>(V))
+    return DAG.getBasicBlock(FuncInfo.MBBMap[BB]);
   llvm_unreachable("Can't get register for value!");
 }
 
 void SelectionDAGBuilder::visitCatchPad(const CatchPadInst &I) {
   auto Pers = classifyEHPersonality(FuncInfo.Fn->getPersonalityFn());
   bool IsMSVCCXX = Pers == EHPersonality::MSVC_CXX;
   bool IsCoreCLR = Pers == EHPersonality::CoreCLR;
   bool IsSEH = isAsynchronousEHPersonality(Pers);
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.hasArg()) {
       OpInfo.CallOperandVal = Call.getArgOperand(ArgNo);
-
-      // Process the call argument. BasicBlocks are labels, currently appearing
-      // only in asm's.
-      if (isa<CallBrInst>(Call) &&
-          ArgNo >= (cast<CallBrInst>(&Call)->arg_size() -
-                        cast<CallBrInst>(&Call)->getNumIndirectDests() -
-                        NumMatchingOps) &&
-          (NumMatchingOps == 0 ||
-           ArgNo < (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)) {
-        OpInfo.CallOperand = DAG.getBasicBlock(FuncInfo.MBBMap[BB]);
-      } else {
-        OpInfo.CallOperand = getValue(OpInfo.CallOperandVal);
-      }
-
       Type *ParamElemTy = Call.getAttributes().getParamElementType(ArgNo);
       EVT VT = OpInfo.getCallOperandValEVT(*DAG.getContext(), TLI,
                                            DAG.getDataLayout(), ParamElemTy);
       OpInfo.ConstraintVT = VT.isSimple() ? VT.getSimpleVT() : MVT::Other;
       ArgNo++;
     } else if (OpInfo.Type == InlineAsm::isOutput && !OpInfo.isIndirect) {
       // The return value of the call is this value.  As such, there is no
       // corresponding argument.
       assert(!Call.getType()->isVoidTy() && "Bad inline asm!");
       if (StructType *STy = dyn_cast<StructType>(Call.getType())) {
         OpInfo.ConstraintVT = TLI.getSimpleValueType(
             DAG.getDataLayout(), STy->getElementType(ResNo));
       } else {
         assert(ResNo == 0 && "Asm only has one result!");
         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/lib/CodeGen/SelectionDAG/TargetLowering.cpp

void TargetLowering::LowerAsmOperandForConstraint(SDValue Op,
                                                   std::vector<SDValue> &Ops,
                                                   SelectionDAG &DAG) const {
 
   if (Constraint.length() > 1) return;
 
   char ConstraintLetter = Constraint[0];
   switch (ConstraintLetter) {
   default: break;
-  case 'X':     // Allows any operand; labels (basic block) use this.
-    if (Op.getOpcode() == ISD::BasicBlock ||
-        Op.getOpcode() == ISD::TargetBlockAddress) {
-      Ops.push_back(Op);
-      return;
-    }
-    LLVM_FALLTHROUGH;
+  case 'X':    // Allows any operand
   case 'i':    // Simple Integer or Relocatable Constant
   case 'n':    // Simple Integer
   case 's': {  // Relocatable Constant
 
     ConstantSDNode *C;
     uint64_t Offset = 0;
 
     // Match (GA) or (C) or (GA+C) or (GA-C) or ((GA+C)+C) or (((GA+C)+C)+C),
     // etc., since getelementpointer is variadic. We can't use
     // SelectionDAG::FoldSymbolOffset because it expects the GA to be accessible
     // while in this case the GA may be furthest from the root node which is
     // likely an ISD::ADD.
     while (true) {
       if ((C = dyn_cast<ConstantSDNode>(Op)) && ConstraintLetter != 's') {
         // gcc prints these as sign extended.  Sign extend value to 64 bits
         // now; without this it would get ZExt'd later in
         // ScheduleDAGSDNodes::EmitNode, which is very generic.
         bool IsBool = C->getConstantIntValue()->getBitWidth() == 1;
         BooleanContent BCont = getBooleanContents(MVT::i64);
         ISD::NodeType ExtOpc =
             IsBool ? getExtendForContent(BCont) : ISD::SIGN_EXTEND;
         int64_t ExtVal =
             ExtOpc == ISD::ZERO_EXTEND ? C->getZExtValue() : C->getSExtValue();
         Ops.push_back(
             DAG.getTargetConstant(Offset + ExtVal, SDLoc(C), MVT::i64));
         return;
       }
       if (ConstraintLetter != 'n') {
         if (const auto *GA = dyn_cast<GlobalAddressSDNode>(Op)) {
           Ops.push_back(DAG.getTargetGlobalAddress(GA->getGlobal(), SDLoc(Op),
                                                    GA->getValueType(0),
                                                    Offset + GA->getOffset()));
           return;
         }
         if (const auto *BA = dyn_cast<BlockAddressSDNode>(Op)) {
           Ops.push_back(DAG.getTargetBlockAddress(
               BA->getBlockAddress(), BA->getValueType(0),
               Offset + BA->getOffset(), BA->getTargetFlags()));
           return;
         }
+        if (isa<BasicBlockSDNode>(Op)) {
+          Ops.push_back(Op);
+          return;
+        }
       }
       const unsigned OpCode = Op.getOpcode();
       if (OpCode == ISD::ADD || OpCode == ISD::SUB) {
         if ((C = dyn_cast<ConstantSDNode>(Op.getOperand(0))))
           Op = Op.getOperand(1);
         // Subtraction is not commutative.
         else if (OpCode == ISD::ADD &&
                  (C = dyn_cast<ConstantSDNode>(Op.getOperand(1))))
if (OpInfo.Codes.size() == 1) {
     OpInfo.ConstraintCode = OpInfo.Codes[0];
     OpInfo.ConstraintType = getConstraintType(OpInfo.ConstraintCode);
   } else {
     ChooseConstraint(OpInfo, *this, Op, DAG);
   }
 
   // 'X' matches anything.
   if (OpInfo.ConstraintCode == "X" && OpInfo.CallOperandVal) {
-    // Labels and constants are handled elsewhere ('X' is the only thing
-    // that matches labels).  For Functions, the type here is the type of
-    // the result, which is not what we want to look at; leave them alone.
+    // Constants are handled elsewhere.  For Functions, the type here is the
+    // type of the result, which is not what we want to look at; leave them
+    // alone.
     Value *v = OpInfo.CallOperandVal;
-    if (isa<BasicBlock>(v) || isa<ConstantInt>(v) || isa<Function>(v)) {
-      OpInfo.CallOperandVal = v;
+    if (isa<ConstantInt>(v) || isa<Function>(v)) {
       return;
     }
 
-    if (Op.getNode() && Op.getOpcode() == ISD::TargetBlockAddress)
+    if (isa<BasicBlock>(v) || isa<BlockAddress>(v)) {
+      OpInfo.ConstraintCode = "i";
       return;
+    }
 
     // Otherwise, try to resolve it to something we know about by looking at
     // the actual operand type.
     if (const char *Repl = LowerXConstraint(OpInfo.ConstraintVT)) {
       OpInfo.ConstraintCode = Repl;
       OpInfo.ConstraintType = getConstraintType(OpInfo.ConstraintCode);
     }
   }

llvm/test/CodeGen/AArch64/inlineasm-X-constraint.ll

entry:
   tail call void asm sideeffect "bl $0", "X"(void (...)* @foo) nounwind
   tail call void asm sideeffect "br $0", "X"(void (...)* bitcast (void ()* @f4 to void (...)*)) nounwind
   ret void
 }
 
 ; The following IR can be generated from C code with a function like:
 ; void a() {
 ;   void* a = &&A;
 ;   asm volatile ("bl %0" : : "X" (a));
 ;  A:
 ;   return;
 ; }
-;
-; Ideally this would give the block address of bb, but it requires us to see
-; through blockaddress, which we can't do at the moment. This might break some
-; existing use cases where a user would expect to get a block label and instead
-; gets the block address in a register. However, note that according to the
-; "no constraints" definition this behaviour is correct (although not very nice).
 
 ; CHECK-LABEL: f7
-; CHECK: bl
+; CHECK: bl .Ltmp3
 define void @f7() {
-  call void asm sideeffect "br $0", "X"( i8* blockaddress(@f7, %bb) )
+  call void asm sideeffect "bl $0", "X"( i8* blockaddress(@f7, %bb) )
   br label %bb
 bb:
   ret void
 }
 
 ; If we use a constraint "=*X", we should get a store back to *%x (in x0).
 ; CHECK-LABEL: f8
 ; CHECK: add   [[Dest:x[0-9]+]], x0, x0
 ; CHECK: str  [[Dest]], [x0]
 define void @f8(i64 *%x) {
 entry:
   tail call void asm sideeffect "add $0, x0, x0", "=*X"(i64* elementtype(i64) %x)
   ret void
 }

llvm/test/CodeGen/X86/asm-block-labels2.ll

+; RUN: not llc -mtriple=x86_64-linux-gnu -o - %s 2>&1 | FileCheck %s
+
+; Test that the blockaddress with X, i, or s constraint is printed as an
+; immediate (.Ltmp0).
+; Test that blockaddress with n constraint is an error.
+define void @test1() {
+; CHECK: error: constraint 'n' expects an integer constant expression
+; CHECK-LABEL: test1:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:  .Ltmp0: # Block address taken
+; CHECK-NEXT:  # %bb.1: # %b
+; CHECK-NEXT:    #APP
+; CHECK-NEXT:    # .Ltmp0 .Ltmp0 .Ltmp0
+; CHECK-NEXT:    #NO_APP
+; CHECK-NEXT:    retq
+entry:
+  br label %b
+b:
+  call void asm "# $0 $1 $2", "X,i,s"(i8* blockaddress(@test1, %b), i8* blockaddress(@test1, %b), i8* blockaddress(@test1, %b))
+  call void asm "# $0", "n"(i8* blockaddress(@test1, %b))
+  ret void
+}

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-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, TargetExternalSymbol:i64'xorl $0, $0; jmp ${1:l}', MDNode:ch<null>, TargetConstant:i64<8>, TargetConstant:i32<2293769>, Register:i32 %5, TargetConstant:i64<13>, TargetBlockAddress:i64<@test, %fail> 0, TargetConstant:i32<12>, Register:i32 $df, TargetConstant:i32<12>, Register:i16 $fpsw, TargetConstant:i32<12>, Register:i32 $eflags, t22:1
 
 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,X,~{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},X,{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"}