Address feedback

Author: lenary

llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

@@ -39,7 +39,7 @@ static cl::opt<bool> ULEB128Reloc(
 RISCVAsmBackend::RISCVAsmBackend(const MCSubtargetInfo &STI, uint8_t OSABI,
                                  bool Is64Bit, const MCTargetOptions &Options)
     : MCAsmBackend(llvm::endianness::little), STI(STI), OSABI(OSABI),
-      Is64Bit(Is64Bit), TargetOptions(Options), VendorSymbols() {
+      Is64Bit(Is64Bit), TargetOptions(Options) {
   RISCVFeatures::validate(STI.getTargetTriple(), STI.getFeatureBits());
 }
 
@@ -612,38 +612,41 @@ bool RISCVAsmBackend::evaluateTargetFixup(const MCFixup &Fixup,
          isPCRelFixupResolved(AUIPCTarget.getAddSym(), *AUIPCDF);
 }
 
-std::optional<StringRef>
-RISCVAsmBackend::getVendorIdentifierForFixup(unsigned FixupKind) const {
-  switch (FixupKind) {
+void RISCVAsmBackend::maybeAddVendorReloc(const MCFragment &F, const MCFixup &Fixup) {
+  MCContext &Ctx = Asm->getContext();
+
+  StringRef VendorIdentifier;
+
+  switch (Fixup.getTargetKind()) {
+  default:
+    // No Vendor Relocation Required.
+    return;
   case RISCV::fixup_riscv_qc_e_branch:
   case RISCV::fixup_riscv_qc_abs20_u:
   case RISCV::fixup_riscv_qc_e_32:
   case RISCV::fixup_riscv_qc_e_jump_plt:
-    return "QUALCOMM";
+    VendorIdentifier = "QUALCOMM";
+    break;
   }
 
-  return std::nullopt;
-}
-
-void RISCVAsmBackend::addVendorReloc(const MCFragment &F, const MCFixup &Fixup,
-                                     StringRef VendorIdentifier) {
-  MCContext &Ctx = Asm->getContext();
+  // Create a local symbol for the vendor relocation to reference. It's fine if the symbol has the same name as an existing symbol.
+  MCSymbol *VendorSymbol = Ctx.createLocalSymbol(VendorIdentifier);
+  auto [It, Inserted] = VendorSymbols.try_emplace(VendorIdentifier, VendorSymbol);
 
-  auto It = VendorSymbols.find(VendorIdentifier);
-  if (It == VendorSymbols.end()) {
-    MCSymbol *VendorSymbol = Ctx.createLocalSymbol(VendorIdentifier);
+  if (Inserted) {
+    // Setup the just-created symbol
     VendorSymbol->setVariableValue(MCConstantExpr::create(0, Ctx));
     Asm->registerSymbol(*VendorSymbol);
-
-    It = VendorSymbols.try_emplace(VendorIdentifier, VendorSymbol).first;
+  } else {
+    // Fetch the existing symbol
+    VendorSymbol = It->getValue();
   }
 
-  MCSymbol *VendorSymbol = It->getValue();
   const MCExpr *VendorExpr = MCSymbolRefExpr::create(VendorSymbol, Ctx);
   MCFixup VendorFixup =
       MCFixup::create(Fixup.getOffset(), VendorExpr, ELF::R_RISCV_VENDOR);
-  // Explicitly create MCValue so that the absolute symbol is not evaluated to
-  // a constant.
+  // Explicitly create MCValue rather than using `VendorExpr->evaluateAsRelocatable`
+  // so that the absolute symbol is not evaluated to a constant.
   MCValue VendorTarget = MCValue::get(VendorSymbol);
   uint64_t VendorValue;
   Asm->getWriter().recordRelocation(F, VendorFixup, VendorTarget, VendorValue);
@@ -702,9 +705,7 @@ bool RISCVAsmBackend::addReloc(const MCFragment &F, const MCFixup &Fixup,
   if (!IsResolved) {
     // Some Fixups require a vendor relocation, record it (directly) before we
     // add the relocation.
-    if (std::optional<StringRef> VendorIdentifier =
-            getVendorIdentifierForFixup(Fixup.getTargetKind()))
-      addVendorReloc(F, Fixup, *VendorIdentifier);
+    maybeAddVendorReloc(F, Fixup);
 
     Asm->getWriter().recordRelocation(F, Fixup, Target, FixedValue);
   }

llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.h

@@ -50,14 +50,10 @@ class RISCVAsmBackend : public MCAsmBackend {
   bool evaluateTargetFixup(const MCFixup &Fixup, const MCValue &Target,
                            uint64_t &Value) override;
 
-  std::optional<StringRef>
-  getVendorIdentifierForFixup(unsigned FixupKind) const;
-
   bool addReloc(const MCFragment &, const MCFixup &, const MCValue &,
                 uint64_t &FixedValue, bool IsResolved) override;
 
-  void addVendorReloc(const MCFragment &, const MCFixup &,
-                      StringRef VendorSymbol);
+  void maybeAddVendorReloc(const MCFragment &, const MCFixup &);
 
   void applyFixup(const MCFragment &, const MCFixup &, const MCValue &Target,
                   MutableArrayRef<char> Data, uint64_t Value,

llvm/test/MC/RISCV/vendor-symbol.s

@@ -0,0 +1,32 @@
+# RUN: llvm-mc -triple riscv32 -mattr=+experimental-xqcibi %s \
+# RUN:     -filetype=obj -o - \
+# RUN:     | llvm-readelf -sr - \
+# RUN:     | FileCheck %s
+
+
+## This checks that the vendor identifier symbols required for vendor
+## relocations do not interfere with symbols with identical names that
+## are written in assembly.
+
+.option exact
+
+  qc.e.bgeui s0, 20, QUALCOMM
+
+  .global QUALCOMM
+QUALCOMM:
+  nop
+
+  qc.e.bgeui s0, 20, QUALCOMM
+
+
+# CHECK-LABEL: Relocation section '.rela.text'
+## Note the different values for the "Sym. Value" Field
+# CHECK: R_RISCV_VENDOR    00000000 QUALCOMM + 0
+# CHECK: R_RISCV_CUSTOM193 00000006 QUALCOMM + 0
+# CHECK: R_RISCV_VENDOR    00000000 QUALCOMM + 0
+# CHECK: R_RISCV_CUSTOM193 00000006 QUALCOMM + 0
+
+
+# CHECK-LABEL: Symbol table '.symtab'
+# CHECK: 00000000 0 NOTYPE  LOCAL  DEFAULT ABS QUALCOMM
+# CHECK: 00000006 0 NOTYPE  GLOBAL DEFAULT   2 QUALCOMM