[MC] Don't recreate a label if it's already used

[void]

Summary

This patch keeps track of MCSymbols created for blocks that were
referenced in inline asm. It prevents creating a new symbol which
doesn't refer to the block.

Inline asm may have a reference to a label. The asm parser however
doesn't recognize it as a label and tries to create a new symbol. The
result being that instead of the original symbol (e.g. ".Ltmp0") the
parser replaces it in the inline asm with the new one (e.g. ".Ltmp00")
without updating it in the symbol table. So the machine basic block
retains the "old" symbol (".Ltmp0"), but the inline asm uses the new one
(".Ltmp00").

[void]

Friendly ping. :-)

[nickdesaulniers]

lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp

435 ↗

(On Diff #211845)

Seeing repeated references to a the same variable in a parameter list (Sym) is a code smell. So registerInlineAsmLabel could just take a MCSymbol* and pull the name out of it via getName().

test/CodeGen/AArch64/callbr-asm-label.ll

1 ↗

(On Diff #211845)

the triple for AArch64 should be aarch64-linux-gnu (armv4t is 32b ARM).

test/CodeGen/X86/callbr-asm-label-addr.ll

8 ↗

(On Diff #211845)

Maybe not necessary to solve in this patch, but it's curious to me why we emit BOTH .Ltmp1 AND .LBB0_1 for %bar, but only .Ltmp0 for %baz (with %bb.2 commented out). Obviously related to %bar being the callbr jump target (while %baz is simply the address of a label), but something seems wrong to me to emit so many labels to the same statement.

From this case specifically, I feel like .LBB0_1: should not be emitted, and instead there should be a (commented out) # %bb.1: # %bar.

[nickdesaulniers]

lib/MC/MCContext.cpp

64 ↗

(On Diff #211845)

I think we may want to be invoking the StringMap constructor that additionally takes an InitialSize, which we'd set to 0U, as it's highly unlikely that the %l modifier is used at all for any given piece of code.

IIUC, StringMap's constructor that simply accepts an allocator will allocate room for 1 instance. See the definition of StringMapImpl::StringMapImpl(unsigned InitSize, unsigned itemSize).

I think that micro optimization would save many small allocations for the general case.

[void]

Remove smelly code and use correct triple in test.

[void]

lib/MC/MCContext.cpp

64 ↗

(On Diff #211845)

The c'tor that's called when we supply only an allocator is this one:

explicit StringMapImpl(unsigned itemSize)
      : ItemSize(itemSize) {}

If we supply an InitSize, it executes the init() function during creation, but only if it's non-zero. So the effect is the same whether we supply it or not.

test/CodeGen/X86/callbr-asm-label-addr.ll

8 ↗

(On Diff #211845)

Yeah, that's beyond the scope of this patch. It should *probably* emit a label only when needed, but that's probably infeasible with the current structure of MC...

[void]

Update testcase.

[void]

Move method out-of-line to keep MCContext.h simpler.

llvm/trunk/include/llvm/MC/MCContext.h

private:
     DenseMap<std::pair<unsigned, unsigned>, MCSymbol *> LocalSymbols;
 
     /// Keeps tracks of names that were used both for used declared and
     /// artificial symbols. The value is "true" if the name has been used for a
     /// non-section symbol (there can be at most one of those, plus an unlimited
     /// number of section symbols with the same name).
     StringMap<bool, BumpPtrAllocator &> UsedNames;
 
+    /// Keeps track of labels that are used in inline assembly.
+    SymbolTable InlineAsmUsedLabelNames;
+
     /// The next ID to dole out to an unnamed assembler temporary symbol with
     /// a given prefix.
     StringMap<unsigned> NextID;
 
     /// Instances of directional local labels.
     DenseMap<unsigned, MCLabel *> Instances;
     /// NextInstance() creates the next instance of the directional local label
     /// for the LocalLabelVal and adds it to the map if needed.
public:
     void setSymbolValue(MCStreamer &Streamer, StringRef Sym, uint64_t Val);
 
     /// getSymbols - Get a reference for the symbol table for clients that
     /// want to, for example, iterate over all symbols. 'const' because we
     /// still want any modifications to the table itself to use the MCContext
     /// APIs.
     const SymbolTable &getSymbols() const { return Symbols; }
 
+    /// isInlineAsmLabel - Return true if the name is a label referenced in
+    /// inline assembly.
+    MCSymbol *getInlineAsmLabel(StringRef Name) const {
+      return InlineAsmUsedLabelNames.lookup(Name);
+    }
+
+    /// registerInlineAsmLabel - Records that the name is a label referenced in
+    /// inline assembly.
+    void registerInlineAsmLabel(MCSymbol *Sym);
+
     /// @}
 
     /// \name Section Management
     /// @{
 
     enum : unsigned {
       /// Pass this value as the UniqueID during section creation to get the
       /// generic section with the given name and characteristics. The usual

llvm/trunk/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp

case '$': {
 
           // 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)) {

llvm/trunk/lib/MC/MCContext.cpp

AsSecureLogFileName("as-secure-log-file-name",
                  "AS_SECURE_LOG_FILE env variable)"),
         cl::init(getenv("AS_SECURE_LOG_FILE")), cl::Hidden);
 
 MCContext::MCContext(const MCAsmInfo *mai, const MCRegisterInfo *mri,
                      const MCObjectFileInfo *mofi, const SourceMgr *mgr,
                      MCTargetOptions const *TargetOpts, bool DoAutoReset)
     : SrcMgr(mgr), InlineSrcMgr(nullptr), MAI(mai), MRI(mri), MOFI(mofi),
       Symbols(Allocator), UsedNames(Allocator),
+      InlineAsmUsedLabelNames(Allocator),
       CurrentDwarfLoc(0, 0, 0, DWARF2_FLAG_IS_STMT, 0, 0),
       AutoReset(DoAutoReset), TargetOptions(TargetOpts) {
   SecureLogFile = AsSecureLogFileName;
 
   if (SrcMgr && SrcMgr->getNumBuffers())
     MainFileName =
         SrcMgr->getMemoryBuffer(SrcMgr->getMainFileID())->getBufferIdentifier();
 }

 void MCContext::reset() {
   // Call the destructors so the fragments are freed
   COFFAllocator.DestroyAll();
   ELFAllocator.DestroyAll();
   MachOAllocator.DestroyAll();
   XCOFFAllocator.DestroyAll();
 
   MCSubtargetAllocator.DestroyAll();
+  InlineAsmUsedLabelNames.clear();
   UsedNames.clear();
   Symbols.clear();
   Allocator.Reset();
   Instances.clear();
   CompilationDir.clear();
   MainFileName.clear();
   MCDwarfLineTablesCUMap.clear();
   SectionsForRanges.clear();

 
 void MCContext::setSymbolValue(MCStreamer &Streamer,
                               StringRef Sym,
                               uint64_t Val) {
   auto Symbol = getOrCreateSymbol(Sym);
   Streamer.EmitAssignment(Symbol, MCConstantExpr::create(Val, *this));
 }
 
+void MCContext::registerInlineAsmLabel(MCSymbol *Sym) {
+  InlineAsmUsedLabelNames[Sym->getName()] = Sym;
+}
+
 //===----------------------------------------------------------------------===//
 // Section Management
 //===----------------------------------------------------------------------===//
 
 MCSectionMachO *MCContext::getMachOSection(StringRef Segment, StringRef Section,
                                            unsigned TypeAndAttributes,
                                            unsigned Reserved2, SectionKind Kind,
                                            const char *BeginSymName) {

llvm/trunk/lib/MC/MCParser/AsmParser.cpp

if (!Split.second.empty()) {
       } else if (MAI.doesAllowAtInName() && !MAI.useParensForSymbolVariant()) {
         Variant = MCSymbolRefExpr::VK_None;
       } else {
         return Error(SMLoc::getFromPointer(Split.second.begin()),
                      "invalid variant '" + Split.second + "'");
       }
     }
 
-    MCSymbol *Sym = getContext().getOrCreateSymbol(SymbolName);
+    MCSymbol *Sym = getContext().getInlineAsmLabel(SymbolName);
+    if (!Sym)
+      Sym = getContext().getOrCreateSymbol(SymbolName);
 
     // If this is an absolute variable reference, substitute it now to preserve
     // semantics in the face of reassignment.
     if (Sym->isVariable()) {
       auto V = Sym->getVariableValue(/*SetUsed*/ false);
       bool DoInline = isa<MCConstantExpr>(V) && !Variant;
       if (auto TV = dyn_cast<MCTargetExpr>(V))
         DoInline = TV->inlineAssignedExpr();

llvm/trunk/test/CodeGen/AArch64/callbr-asm-label.ll

+; RUN: llc < %s -mtriple=aarch64-linux-gnu | FileCheck %s
+
+@X = common local_unnamed_addr global i32 0, align 4
+
+define i32 @test1() {
+; CHECK-LABEL: test1:
+; CHECK:         .word b
+; CHECK-NEXT:    .word .Ltmp0
+; CHECK-LABEL: .Ltmp0:
+; CHECK-LABEL: .LBB0_1: // %l_yes
+; CHECK-LABEL: .LBB0_2: // %cleanup
+entry:
+  callbr void asm sideeffect "1:\0A\09.word b, ${0:l}\0A\09", "X"(i8* blockaddress(@test1, %l_yes))
+          to label %cleanup [label %l_yes]
+
+l_yes:
+  br label %cleanup
+
+cleanup:
+  %retval.0 = phi i32 [ 1, %l_yes ], [ 0, %entry ]
+  ret i32 %retval.0
+}
+
+define void @test2() {
+; CHECK-LABEL: test2:
+entry:
+  %0 = load i32, i32* @X, align 4
+  %and = and i32 %0, 1
+  %tobool = icmp eq i32 %and, 0
+  br i1 %tobool, label %if.end10, label %if.then
+
+if.then:
+; CHECK:       .word b
+; CHECK-NEXT:  .word .Ltmp2
+; CHECK-LABEL: .Ltmp2:
+; CHECK-NEXT:  .LBB1_3: // %if.end6
+  callbr void asm sideeffect "1:\0A\09.word b, ${0:l}\0A\09", "X"(i8* blockaddress(@test2, %if.end6))
+          to label %if.then4 [label %if.end6]
+
+if.then4:
+  %call5 = tail call i32 bitcast (i32 (...)* @g to i32 ()*)()
+  br label %if.end6
+
+if.end6:
+  %.pre = load i32, i32* @X, align 4
+  %.pre13 = and i32 %.pre, 1
+  %phitmp = icmp eq i32 %.pre13, 0
+  br i1 %phitmp, label %if.end10, label %if.then9
+
+if.then9:
+; CHECK-LABEL: .Ltmp4:
+; CHECK-NEXT:  .LBB1_5: // %l_yes
+  callbr void asm sideeffect "", "X"(i8* blockaddress(@test2, %l_yes))
+          to label %if.end10 [label %l_yes]
+
+if.end10:
+  br label %l_yes
+
+l_yes:
+  ret void
+}
+
+declare i32 @g(...)

llvm/trunk/test/CodeGen/X86/callbr-asm-label-addr.ll

+; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu | FileCheck %s
+
+define i32 @test1(i32 %x) {
+; CHECK-LABEL: test1:
+; CHECK:         .quad .Ltmp0
+; CHECK-NEXT:    .quad .Ltmp1
+; CHECK-LABEL: .Ltmp1:
+; CHECK-LABEL: .LBB0_1: # %bar
+; CHECK-NEXT:    callq foo
+; CHECK-LABEL: .Ltmp0:
+; CHECK-NEXT:  # %bb.2: # %baz
+entry:
+  callbr void asm sideeffect ".quad ${0:l}\0A\09.quad ${1:l}", "i,X,~{dirflag},~{fpsr},~{flags}"(i8* blockaddress(@test1, %baz), i8* blockaddress(@test1, %bar))
+          to label %asm.fallthrough [label %bar]
+
+asm.fallthrough:
+  br label %bar
+
+bar:
+  %call = tail call i32 @foo(i32 %x)
+  br label %baz
+
+baz:
+  %call1 = tail call i32 @mux(i32 %call)
+  ret i32 %call1
+}
+
+declare i32 @foo(i32)
+
+declare i32 @mux(i32)

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

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc < %s -mtriple=i686-- -O3 | FileCheck %s
 
 ; Tests for using callbr as an asm-goto wrapper
 
 ; Test 1 - fallthrough label gets removed, but the fallthrough code that is
 ; unreachable due to asm ending on a jmp is still left in.
 define i32 @test1(i32 %a) {
 ; CHECK-LABEL: test1:
 ; CHECK:       # %bb.0: # %entry
 ; CHECK-NEXT:    movl {{[0-9]+}}(%esp), %eax
 ; CHECK-NEXT:    addl $4, %eax
 ; CHECK-NEXT:    #APP
 ; CHECK-NEXT:    xorl %eax, %eax
-; CHECK-NEXT:    jmp .Ltmp00
+; CHECK-NEXT:    jmp .Ltmp0
 ; CHECK-NEXT:    #NO_APP
 ; CHECK-NEXT:  .LBB0_1: # %normal
 ; CHECK-NEXT:    xorl %eax, %eax
 ; CHECK-NEXT:    retl
 ; CHECK-NEXT:  .Ltmp0: # Block address taken
 ; CHECK-NEXT:  .LBB0_2: # %fail
 ; CHECK-NEXT:    movl $1, %eax
 ; CHECK-NEXT:    retl

 ; CHECK-NEXT:    .p2align 4, 0x90
 ; CHECK-NEXT:  .Ltmp4: # Block address taken
 ; CHECK-NEXT:  .LBB2_4: # %label04
 ; CHECK-NEXT:    # Parent Loop BB2_1 Depth=1
 ; CHECK-NEXT:    # Parent Loop BB2_2 Depth=2
 ; CHECK-NEXT:    # Parent Loop BB2_3 Depth=3
 ; CHECK-NEXT:    # => This Inner Loop Header: Depth=4
 ; CHECK-NEXT:    #APP
-; CHECK-NEXT:    jmp .Ltmp10
-; CHECK-NEXT:    jmp .Ltmp20
-; CHECK-NEXT:    jmp .Ltmp30
+; CHECK-NEXT:    jmp .Ltmp1
+; CHECK-NEXT:    jmp .Ltmp2
+; CHECK-NEXT:    jmp .Ltmp3
 ; CHECK-NEXT:    #NO_APP
 ; CHECK-NEXT:  .LBB2_5: # %normal0
 ; CHECK-NEXT:    # in Loop: Header=BB2_4 Depth=4
 ; CHECK-NEXT:    #APP
-; CHECK-NEXT:    jmp .Ltmp10
-; CHECK-NEXT:    jmp .Ltmp20
-; CHECK-NEXT:    jmp .Ltmp30
-; CHECK-NEXT:    jmp .Ltmp40
+; CHECK-NEXT:    jmp .Ltmp1
+; CHECK-NEXT:    jmp .Ltmp2
+; CHECK-NEXT:    jmp .Ltmp3
+; CHECK-NEXT:    jmp .Ltmp4
 ; CHECK-NEXT:    #NO_APP
 ; CHECK-NEXT:  .LBB2_6: # %normal1
 ; CHECK-NEXT:    movl {{[0-9]+}}(%esp), %eax
 ; CHECK-NEXT:    retl
 entry:
   %a.addr = alloca i32, align 4
   store i32 %a, i32* %a.addr, align 4
   br label %label01