Don't rely on '%l' modifiers to indicate a label reference

[void]

Summary

It's not necessary to use an '%l' modifier when referencing a label.
Treat block addresses and MBB references as if the modifier is used
anyway. This prevents us from generating references to ficticious
labels.

[nickdesaulniers]

Thanks for the fix. Two minor fixes requested, then should be good to land.

llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp

470

Would you mind hoisting the expression MI->getOperand(OpNo) into a local variable defined above near the definition of OpNo? It would DRY up this code.

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

135

do we require dso_local now? (opt -verify <file> says we don't). Would you mind dropping it?

[void]

Remove dso_local which isn't necessary

[void]

llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp

470

The compiler should do this already, because MI is const and getOperand has no side-effects. I think that's who no one has bothered to do it before.

[nickdesaulniers]

llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp

470

Technically, here !MI->getOperand(OpNo).isBlockAddress() && !MI->getOperand(OpNo).isMBB() is tautalogically false (due to the earlier checks), so you'd only need to check for l. Maybe a comment like // %l should have been handled by the above conditions already., too would be nice.

470

No doubt, but it's annoying to continually type out the full expression, hence the request to DRY up the code. Please make that change.

[void]

Make temp variable for the machine instruction operand.

[nickdesaulniers]

llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp

437

I feel like the check that OpNo >= MI->getNumOperands() should probably guard any calls to MI->getOperand(OpNo) (or that we should check the output of MI->getOperand(OpNo) for validity. Otherwise, getOperand will assert. This would allow you to hoist the two OpNo >= MI->getNumOperands() guards below.

[void]

Change back.

[void]

llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp

437

This is one of the reasons why I didn't want to hoist the change out. It's more trouble than it's worth and in the end isn't that useful. I'm changing it back.

llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp

case '$': {
       if (Val >= NumOperands-1)
         report_fatal_error("Invalid $ operand number in inline asm string: '" +
                            Twine(AsmStr) + "'");
 
       // Okay, we finally have a value number.  Ask the target to print this
       // operand!
       if (CurVariant == -1 || CurVariant == AsmPrinterVariant) {
         unsigned OpNo = InlineAsm::MIOp_FirstOperand;
 
         bool Error = false;
 
         // Scan to find the machine operand number for the operand.
         for (; Val; --Val) {
           if (OpNo >= MI->getNumOperands()) break;
           unsigned OpFlags = MI->getOperand(OpNo).getImm();
           OpNo += InlineAsm::getNumOperandRegisters(OpFlags) + 1;
         }
 
         // We may have a location metadata attached to the end of the
         // instruction, and at no point should see metadata at any
         // other point while processing. It's an error if so.
         if (OpNo >= MI->getNumOperands() ||
             MI->getOperand(OpNo).isMetadata()) {
           Error = true;
         } else {
           unsigned OpFlags = MI->getOperand(OpNo).getImm();
           ++OpNo;  // Skip over the ID number.
 
           // FIXME: Shouldn't arch-independent output template handling go into
           // PrintAsmOperand?
-          if (Modifier[0] == 'l') { // Labels are target independent.
-            if (MI->getOperand(OpNo).isBlockAddress()) {
-              const BlockAddress *BA = MI->getOperand(OpNo).getBlockAddress();
-              MCSymbol *Sym = AP->GetBlockAddressSymbol(BA);
-              Sym->print(OS, AP->MAI);
-              MMI->getContext().registerInlineAsmLabel(Sym);
-            } else if (MI->getOperand(OpNo).isMBB()) {
-              const MCSymbol *Sym = MI->getOperand(OpNo).getMBB()->getSymbol();
-              Sym->print(OS, AP->MAI);
-            } else {
-              Error = true;
-            }
-          } else {
-            if (InlineAsm::isMemKind(OpFlags)) {
-              Error = AP->PrintAsmMemoryOperand(
-                  MI, OpNo, Modifier[0] ? Modifier : nullptr, OS);
-            } else {
-              Error = AP->PrintAsmOperand(MI, OpNo,
-                                          Modifier[0] ? Modifier : nullptr, OS);
-            }
-          }
-        }
+          // Labels are target independent.
+          } else if (Modifier[0] == 'l' &&
+                     !MI->getOperand(OpNo).isBlockAddress() &&
+                     !MI->getOperand(OpNo).isMBB()) {
+          } else if (InlineAsm::isMemKind(OpFlags)) {
         if (Error) {
           std::string msg;
           raw_string_ostream Msg(msg);
           Msg << "invalid operand in inline asm: '" << AsmStr << "'";
           MMI->getModule()->getContext().emitError(LocCookie, Msg.str());
         }
       }
       break;

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

 
 normal:
   ret i32 0
 
 fail:
   ret i32 1
 }
 
-
 ; Test 3 - asm-goto implements a loop. The loop gets recognized, but many loop
 ; transforms fail due to canonicalization having callbr exceptions. Trivial
 ; blocks at labels 1 and 3 also don't get simplified due to callbr.
 define dso_local i32 @test3(i32 %a) {
 ; CHECK-LABEL: test3:
 ; CHECK:       # %bb.0: # %entry
 ; CHECK-NEXT:  .Ltmp1: # Block address taken
 ; CHECK-NEXT:  .LBB2_1: # %label01

 normal0:                                          ; preds = %label04
   callbr void asm sideeffect "jmp ${0:l}; jmp ${1:l}; jmp ${2:l}; jmp ${3:l}", "X,X,X,X,~{dirflag},~{fpsr},~{flags}"(i8* blockaddress(@test3, %label01), i8* blockaddress(@test3, %label02), i8* blockaddress(@test3, %label03), i8* blockaddress(@test3, %label04))
           to label %normal1 [label %label01, label %label02, label %label03, label %label04]
 
 normal1:                                          ; preds = %normal0
   %1 = load i32, i32* %a.addr, align 4
   ret i32 %1
 }
+
+; Test 4 - asm-goto referenced with the 'l' (ell) modifier and not.
+define dso_local void @test4() {
+; CHECK-LABEL: test4:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    #APP
+; CHECK-NOT:     ja .Ltmp50
+; CHECK-NEXT:    ja .Ltmp5
+; CHECK-NEXT:    #NO_APP
+; CHECK-NEXT:  .LBB3_1:
+; CHECK-NEXT:    #APP
+; CHECK-NOT:     ja .Ltmp50
+; CHECK-NEXT:    ja .Ltmp5
+; CHECK-NEXT:    #NO_APP
+; CHECK-NEXT:    jmp .LBB3_3
+entry:
+  callbr void asm sideeffect "ja $0", "X,~{dirflag},~{fpsr},~{flags}"(i8* blockaddress(@test4, %quux))
+          to label %asm.fallthrough [label %quux]
+
+asm.fallthrough:                                  ; preds = %entry
+  callbr void asm sideeffect "ja ${0:l}", "X,~{dirflag},~{fpsr},~{flags}"(i8* blockaddress(@test4, %quux))
+          to label %cleanup [label %quux]
+
+quux:                                             ; preds = %asm.fallthrough, %entry
+  br label %cleanup
+
+cleanup:                                          ; preds = %asm.fallthrough, %quux
+  ret void
+}