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

[nickdesaulniers]

Summary

Reverts commit 1b104388752f66191c867380efde7bbf1f13ca80.

After D130316, this is no longer necessary.

[arsenm]

LGTM. Feels wrong that inline asm would necessitate special label handling

[void]

In D130323#3670500, @arsenm wrote:

LGTM. Feels wrong that inline asm would necessitate special label handling

"Inline asm feels wrong."

Fixed that for you. :-P

[nickdesaulniers]

Turning on more build targets, either this or https://reviews.llvm.org/D130391 regresses (llvm/test/CodeGen/RISCV/inline-asm-S-constraint.ll).

[nickdesaulniers]

In D130323#3673261, @nickdesaulniers wrote:

Turning on more build targets, either this or https://reviews.llvm.org/D130391 regresses (llvm/test/CodeGen/RISCV/inline-asm-S-constraint.ll).

It's the child patch.

[nickdesaulniers]

• rebase

[nickdesaulniers]

It's the child patch.

i.e. this one regresses llvm/test/CodeGen/RISCV/inline-asm-S-constraint.ll. I'm going to rebase D130391 to go in first, since that shouldn't be controversial. Let's see.

[nickdesaulniers]

I think I've found the issue: AddrLabelMap::getAddrLabelSymbolToEmit uses createNamedTempSymbol() when a BasicBlock has its address taken (i.e. blockaddress), but it doesn't update the symbol table (MCContext's Symbols member), so AsmParser can't find the existing symbol.

It looks like in the failing test case llvm/test/CodeGen/RISCV/inline-asm-S-constraint.ll that

createSymbol is called with .Ltmp to create the initial BB label, but then later getOrCreateSymbol calls createSymbol for .Ltmp0.

It seems like AddrLabelMap::getAddrLabelSymbolToEmit should have just added .Ltmp0 to MCContext's Symbols then and there.

llvm/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/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp

case '$': {
 
           // FIXME: Shouldn't arch-independent output template handling go into
           // PrintAsmOperand?
           // 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 if (InlineAsm::isMemKind(OpFlags)) {
             Error = AP->PrintAsmMemoryOperand(
                 MI, OpNo, Modifier[0] ? Modifier : nullptr, OS);
           } else {
             Error = AP->PrintAsmOperand(MI, OpNo,

llvm/lib/MC/MCContext.cpp

 
 MCContext::MCContext(const Triple &TheTriple, const MCAsmInfo *mai,
                      const MCRegisterInfo *mri, const MCSubtargetInfo *msti,
                      const SourceMgr *mgr, MCTargetOptions const *TargetOpts,
                      bool DoAutoReset, StringRef Swift5ReflSegmentName)
     : Swift5ReflectionSegmentName(Swift5ReflSegmentName), TT(TheTriple),
       SrcMgr(mgr), InlineSrcMgr(nullptr), DiagHandler(defaultDiagHandler),
       MAI(mai), MRI(mri), MSTI(msti), 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 = std::string(SrcMgr->getMemoryBuffer(SrcMgr->getMainFileID())
                                    ->getBufferIdentifier());
 
void MCContext::reset() {
   GOFFAllocator.DestroyAll();
   MachOAllocator.DestroyAll();
   WasmAllocator.DestroyAll();
   XCOFFAllocator.DestroyAll();
   MCInstAllocator.DestroyAll();
   SPIRVAllocator.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;
-}
-
 MCSymbolXCOFF *
 MCContext::createXCOFFSymbolImpl(const StringMapEntry<bool> *Name,
                                  bool IsTemporary) {
   if (!Name)
     return new (nullptr, *this) MCSymbolXCOFF(nullptr, IsTemporary);
 
   StringRef OriginalName = Name->first();
   if (OriginalName.startswith("._Renamed..") ||

llvm/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().getInlineAsmLabel(SymbolName);
-    if (!Sym)
-      Sym = getContext().getOrCreateSymbol(
+    MCSymbol *Sym = getContext().getOrCreateSymbol(
           MAI.shouldEmitLabelsInUpperCase() ? SymbolName.upper() : 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))

llvm/lib/MC/MCParser/MasmParser.cpp

if (Split.second.empty()) {
         lookUpField(Base, Member, Info);
       } else if (Structs.count(SymbolName.lower())) {
         // This is actually a reference to a field offset.
         Res = MCConstantExpr::create(Info.Offset, getContext());
         return false;
       }
     }
 
-    MCSymbol *Sym = getContext().getInlineAsmLabel(SymbolName);
+    MCSymbol *Sym = getContext().lookupSymbol(SymbolName);
     if (!Sym) {
       // If this is a built-in numeric value, treat it as a constant.
       auto BuiltinIt = BuiltinSymbolMap.find(SymbolName.lower());
       const BuiltinSymbol Symbol = (BuiltinIt == BuiltinSymbolMap.end())
                                        ? BI_NO_SYMBOL
                                        : BuiltinIt->getValue();
       if (Symbol != BI_NO_SYMBOL) {
         const MCExpr *Value = evaluateBuiltinValue(Symbol, FirstTokenLoc);