[RISCV] Vendor Relocations for Xqci extension #135400

svs-quic · 2025-04-11T16:40:11Z

This patch implements vendor relocation support for RISC-V, starting with the Xqci extensions.

Vendor Relocations need to emit the vendor symbol, and add an R_RISCV_VENDOR relocation against that symbol before the specific vendor relocation itself.

This patch adds a maybeAddVendorReloc function which is called from addReloc, to implement this functionality. Vendor identifier symbols are cached, so that at most one is emitted per vendor identifier.

Vendor identifiers symbols do not interfere with identically named symbols used by assembly.

llvmbot · 2025-04-11T16:40:45Z

@llvm/pr-subscribers-backend-risc-v

@llvm/pr-subscribers-mc

Author: Sudharsan Veeravalli (svs-quic)

Changes

This patch adds support for vendor relocations on Qualcomm uC Xqci instructions.

It adds a new fixupNeedsMarkerELFRelocation function that targets can override to add any marker relocations in the ELF object.

The psabi doc describes the vendor relocations here https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-elf.adoc#nonstandard-relocations-aka-vendor-specific-relocations

> These vendor-specific relocations must be preceded by a R_RISCV_VENDOR relocation against a vendor identifier symbol. The preceding R_RISCV_VENDOR relocation is used by the linker to choose the correct implementation for the associated nonstandard relocation.
>
> The vendor identifier symbol should be a defined symbol and should set the type to STT_NOTYPE, binding to STB_LOCAL, and the size of symbol to zero.

Full diff: https://github.com/llvm/llvm-project/pull/135400.diff

7 Files Affected:

(modified) llvm/include/llvm/MC/MCAsmBackend.h (+12)
(modified) llvm/lib/MC/ELFObjectWriter.cpp (+15)
(modified) llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp (+27)
(modified) llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.h (+6)
(modified) llvm/test/MC/RISCV/xqcibi-relocations.s (+14)
(modified) llvm/test/MC/RISCV/xqcilb-relocations.s (+11)
(modified) llvm/test/MC/RISCV/xqcili-relocations.s (+9)

diff --git a/llvm/include/llvm/MC/MCAsmBackend.h b/llvm/include/llvm/MC/MCAsmBackend.h
index 5953de30c2eb2..fcc67afdc6fc5 100644
--- a/llvm/include/llvm/MC/MCAsmBackend.h
+++ b/llvm/include/llvm/MC/MCAsmBackend.h
@@ -125,6 +125,18 @@ class MCAsmBackend {
     return false;
   }
 
+  /// Hook to add any marker relocations that the target may need in addition
+  /// to the normal relocations on instructions. Currently used only by RISCV
+  /// to add vendor relocations.
+  virtual bool fixupNeedsMarkerELFRelocation(MCAssembler &Asm,
+                                             const MCFragment &F,
+                                             const MCFixup &Fixup,
+                                             unsigned &PreRelocType,
+                                             MCSymbol *&PreRelocSymbol,
+                                             uint64_t &PreRelocAddend) const {
+    return false;
+  }
+
   /// Apply the \p Value for given \p Fixup into the provided data fragment, at
   /// the offset specified by the fixup and following the fixup kind as
   /// appropriate. Errors (such as an out of range fixup value) should be
diff --git a/llvm/lib/MC/ELFObjectWriter.cpp b/llvm/lib/MC/ELFObjectWriter.cpp
index 23d5517920e7b..426937ee415bd 100644
--- a/llvm/lib/MC/ELFObjectWriter.cpp
+++ b/llvm/lib/MC/ELFObjectWriter.cpp
@@ -1435,6 +1435,21 @@ void ELFObjectWriter::recordRelocation(MCAssembler &Asm,
                         : C;
   FixedValue = usesRela(TO, FixupSection) ? 0 : Addend;
 
+  // Some RISC-V relocations need a marker relocation that appears before the
+  // relocation in question.
+  unsigned PreRelocType;
+  MCSymbol *PreRelocSymbol;
+  uint64_t PreRelocAddend;
+  if (Backend.fixupNeedsMarkerELFRelocation(Asm, *Fragment, Fixup, PreRelocType,
+                                            PreRelocSymbol, PreRelocAddend)) {
+    auto *SymbolELF = cast<MCSymbolELF>(PreRelocSymbol);
+    if (SymbolELF)
+      SymbolELF->setUsedInReloc();
+    ELFRelocationEntry Rec(FixupOffset, SymbolELF, PreRelocType,
+                           PreRelocAddend);
+    Relocations[&FixupSection].push_back(Rec);
+  }
+
   if (!RelocateWithSymbol) {
     const auto *SectionSymbol =
         SecA ? cast<MCSymbolELF>(SecA->getBeginSymbol()) : nullptr;
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
index 49c8c6957aa34..bbcb84ea78566 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
@@ -702,6 +702,33 @@ bool RISCVAsmBackend::handleAddSubRelocations(const MCAssembler &Asm,
   return true;
 }
 
+bool RISCVAsmBackend::fixupNeedsMarkerELFRelocation(
+    MCAssembler &Asm, const MCFragment &F, const MCFixup &Fixup,
+    unsigned &PreRelocType, MCSymbol *&PreRelocSymbol,
+    uint64_t &PreRelocAddend) const {
+  switch (Fixup.getTargetKind()) {
+  default:
+    return false;
+  case RISCV::fixup_riscv_qc_e_branch:
+  case RISCV::fixup_riscv_qc_e_32:
+  case RISCV::fixup_riscv_qc_abs20_u:
+  case RISCV::fixup_riscv_qc_e_jump_plt: {
+    MCContext &Ctx = Asm.getContext();
+    MCSymbol *VendorSymbol = Ctx.getOrCreateSymbol("QUALCOMM");
+    MCFragment &DF = F.getParent()->getDummyFragment();
+    VendorSymbol->setFragment(&DF);
+    VendorSymbol->setOffset(0);
+
+    Asm.registerSymbol(*VendorSymbol);
+
+    PreRelocType = ELF::R_RISCV_VENDOR;
+    PreRelocSymbol = VendorSymbol;
+    PreRelocAddend = 0;
+    return true;
+  }
+  }
+}
+
 void RISCVAsmBackend::applyFixup(const MCAssembler &Asm, const MCFixup &Fixup,
                                  const MCValue &Target,
                                  MutableArrayRef<char> Data, uint64_t Value,
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.h b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.h
index f5e8d340d9bce..9076074987a37 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.h
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.h
@@ -56,6 +56,12 @@ class RISCVAsmBackend : public MCAsmBackend {
                                const MCFixup &Fixup, const MCValue &Target,
                                uint64_t &FixedValue) const override;
 
+  bool fixupNeedsMarkerELFRelocation(MCAssembler &Asm, const MCFragment &F,
+                                     const MCFixup &Fixup,
+                                     unsigned &PreRelocType,
+                                     MCSymbol *&PreRelocSymbol,
+                                     uint64_t &PreRelocAddend) const override;
+
   void applyFixup(const MCAssembler &Asm, const MCFixup &Fixup,
                   const MCValue &Target, MutableArrayRef<char> Data,
                   uint64_t Value, bool IsResolved,
diff --git a/llvm/test/MC/RISCV/xqcibi-relocations.s b/llvm/test/MC/RISCV/xqcibi-relocations.s
index 7028e8a737c86..a0ab7a450349d 100644
--- a/llvm/test/MC/RISCV/xqcibi-relocations.s
+++ b/llvm/test/MC/RISCV/xqcibi-relocations.s
@@ -2,11 +2,13 @@
 # RUN:     | FileCheck -check-prefix=INSTR -check-prefix=FIXUP %s
 # RUN: llvm-mc -filetype=obj -triple riscv32 -mattr=+experimental-xqcibi %s -o %t.o
 # RUN: llvm-readobj -r %t.o | FileCheck -check-prefix=RELOC %s
+# RUN: llvm-readelf -s %t.o | FileCheck -check-prefix=VENDORSYM %s
 
 # Check prefixes:
 # RELOC - Check the relocation in the object.
 # FIXUP - Check the fixup on the instruction.
 # INSTR - Check the instruction is handled properly by the ASMPrinter.
+# VENDORSYM - Check the vendor symbol.
 
 .text
 
@@ -22,6 +24,15 @@ qc.e.bgeui x8, 12, foo
 # INSTR: qc.e.bgeui s0, 12, foo
 # FIXUP: fixup A - offset: 0, value: foo, kind: fixup_riscv_qc_e_branch
 
+# Check that we print the correct relocations in exact mode
+.option exact
+qc.e.bgeui x8, 12, foo
+# RELOC: R_RISCV_VENDOR QUALCOMM 0x0
+# RELOC: R_RISCV_CUSTOM193 foo 0x0
+# INSTR: qc.e.bgeui s0, 12, foo
+# FIXUP: fixup A - offset: 0, value: foo, kind: fixup_riscv_qc_e_branch
+.option noexact
+
 # Check that a label in a different section is handled similar to an undefined
 # symbol and gets relaxed to (qc.e.bgeui + jal)
 qc.e.bltui x4, 9, .bar
@@ -34,6 +45,9 @@ qc.e.beqi x7, 8, .L1
 # INSTR: qc.e.beqi t2, 8, .L1
 # FIXUP: fixup A - offset: 0, value: .L1, kind: fixup_riscv_qc_e_branch
 
+# Check that there is only one vendor symbol created and that it is local and NOTYPE
+# VENDORSYM-COUNT-1: 00000000     0 NOTYPE  LOCAL  DEFAULT     2 QUALCOMM
+
 .L1:
   ret
 
diff --git a/llvm/test/MC/RISCV/xqcilb-relocations.s b/llvm/test/MC/RISCV/xqcilb-relocations.s
index a475cde3f6bfd..f9787df352f36 100644
--- a/llvm/test/MC/RISCV/xqcilb-relocations.s
+++ b/llvm/test/MC/RISCV/xqcilb-relocations.s
@@ -2,41 +2,49 @@
 # RUN:     | FileCheck -check-prefix=INSTR -check-prefix=FIXUP %s
 # RUN: llvm-mc -filetype=obj -triple riscv32 -mattr=+experimental-xqcilb %s -o %t.o
 # RUN: llvm-readobj -r %t.o | FileCheck -check-prefix=RELOC %s
+# RUN: llvm-readelf -s %t.o | FileCheck -check-prefix=VENDORSYM %s
 
 # Check prefixes:
 # RELOC - Check the relocation in the object.
 # FIXUP - Check the fixup on the instruction.
 # INSTR - Check the instruction is handled properly by the ASMPrinter.
+# VENDORSYM - Check the vendor symbol.
 
 .text
 
 qc.e.j foo
+# RELOC: R_RISCV_VENDOR QUALCOMM 0x0
 # RELOC: R_RISCV_CUSTOM195 foo 0x0
 # INSTR: qc.e.j foo
 # FIXUP: fixup A - offset: 0, value: foo, kind: fixup_riscv_qc_e_jump_plt
 
 qc.e.j foo@plt
+# RELOC: R_RISCV_VENDOR QUALCOMM 0x0
 # RELOC: R_RISCV_CUSTOM195 foo 0x0
 # INSTR: qc.e.j foo
 # FIXUP: fixup A - offset: 0, value: foo, kind: fixup_riscv_qc_e_jump_plt
 
 qc.e.jal foo@plt
+# RELOC: R_RISCV_VENDOR QUALCOMM 0x0
 # RELOC: R_RISCV_CUSTOM195 foo 0x0
 # INSTR: qc.e.jal foo
 # FIXUP: fixup A - offset: 0, value: foo, kind: fixup_riscv_qc_e_jump_plt
 
 qc.e.jal foo
+# RELOC: R_RISCV_VENDOR QUALCOMM 0x0
 # RELOC: R_RISCV_CUSTOM195 foo 0x0
 # INSTR: qc.e.jal foo
 # FIXUP: fixup A - offset: 0, value: foo, kind: fixup_riscv_qc_e_jump_plt
 
 # Check that a label in a different section is handled similar to an undefined symbol
 qc.e.j .bar
+# RELOC: R_RISCV_VENDOR QUALCOMM 0x0
 # RELOC: R_RISCV_CUSTOM195 .bar 0x0
 # INSTR: qc.e.j .bar
 # FIXUP: fixup A - offset: 0, value: .bar, kind: fixup_riscv_qc_e_jump_plt
 
 qc.e.jal .bar
+# RELOC: R_RISCV_VENDOR QUALCOMM 0x0
 # RELOC: R_RISCV_CUSTOM195 .bar 0x0
 # INSTR: qc.e.jal .bar
 # FIXUP: fixup A - offset: 0, value: .bar, kind: fixup_riscv_qc_e_jump_plt
@@ -50,6 +58,9 @@ qc.e.jal .L1
 # INSTR:qc.e.jal .L1
 # FIXUP: fixup A - offset: 0, value: .L1, kind: fixup_riscv_qc_e_jump_plt
 
+# Check that there is only one vendor symbol created and that it is local and NOTYPE
+# VENDORSYM-COUNT-1: 00000000     0 NOTYPE  LOCAL  DEFAULT     2 QUALCOMM
+
 .L1:
   ret
 
diff --git a/llvm/test/MC/RISCV/xqcili-relocations.s b/llvm/test/MC/RISCV/xqcili-relocations.s
index b0a3f3bae11d5..b7ed8fedb35f8 100644
--- a/llvm/test/MC/RISCV/xqcili-relocations.s
+++ b/llvm/test/MC/RISCV/xqcili-relocations.s
@@ -2,31 +2,37 @@
 # RUN:     | FileCheck -check-prefix=INSTR -check-prefix=FIXUP %s
 # RUN: llvm-mc -filetype=obj -triple riscv32 -mattr=+experimental-xqcili %s -o %t.o
 # RUN: llvm-readobj -r %t.o | FileCheck -check-prefix=RELOC %s
+# RUN: llvm-readelf -s %t.o | FileCheck -check-prefix=VENDORSYM %s
 
 # Check prefixes:
 # RELOC - Check the relocation in the object.
 # FIXUP - Check the fixup on the instruction.
 # INSTR - Check the instruction is handled properly by the ASMPrinter.
+# VENDORSYM - Check the vendor symbol.
 
 .text
 
 qc.li x4, %qc.abs20(foo)
+# RELOC: R_RISCV_VENDOR QUALCOMM 0x0
 # RELOC: R_RISCV_CUSTOM192 foo 0x0
 # INSTR: qc.li tp, %qc.abs20(foo)
 # FIXUP: fixup A - offset: 0, value: %qc.abs20(foo), kind: fixup_riscv_qc_abs20_u
 
 qc.e.li x5, foo
+# RELOC: R_RISCV_VENDOR QUALCOMM 0x0
 # RELOC: R_RISCV_CUSTOM194 foo 0x0
 # INSTR: qc.e.li t0, foo
 # FIXUP: fixup A - offset: 0, value: foo, kind: fixup_riscv_qc_e_32
 
 # Check that a label in a different section is handled similar to an undefined symbol
 qc.li x9, %qc.abs20(.bar)
+# RELOC: R_RISCV_VENDOR QUALCOMM 0x0
 # RELOC: R_RISCV_CUSTOM192 .bar 0x0
 # INSTR: qc.li s1, %qc.abs20(.bar)
 # FIXUP: fixup A - offset: 0, value: %qc.abs20(.bar), kind: fixup_riscv_qc_abs20_u
 
 qc.e.li x8, .bar
+# RELOC: R_RISCV_VENDOR QUALCOMM 0x0
 # RELOC: R_RISCV_CUSTOM194 .bar 0x0
 # INSTR: qc.e.li s0, .bar
 # FIXUP: fixup A - offset: 0, value: .bar, kind: fixup_riscv_qc_e_32
@@ -40,6 +46,9 @@ qc.e.li x6, .L1
 # INSTR: qc.e.li t1, .L1
 # FIXUP: fixup A - offset: 0, value: .L1, kind: fixup_riscv_qc_e_32
 
+# Check that there is only one vendor symbol created and that it is local and NOTYPE
+# VENDORSYM-COUNT-1: 00000000     0 NOTYPE  LOCAL  DEFAULT     2 QUALCOMM
+
 .L1:
   ret

lenary · 2025-04-11T18:42:53Z

@MaskRay I've tagged you in the review because this hits target-independent MC code. We weren't sure of a better way to accomplish what is needed for the RISC-V R_RISCV_VENDOR relocations. We didn't want a separate fixup (as for Relaxation) because we didn't want to emit the R_RISCV_VENDOR relocation if the fixup could be evaluated/applied by the compiler, and that kind of "evaluating one fixup deletes another fixup" seemed complex and error-prone. Hopefully you find our approach reasonable.

There is linker support for these relocations, in https://github.com/qualcomm/eld - which requires the R_RISCV_VENDOR relocations - but it does look like the tests are missing from this repo.

lenary · 2025-04-11T21:26:22Z

Tests for the eld implementation are here: qualcomm/eld#16

llvm/include/llvm/MC/MCAsmBackend.h

MaskRay · 2025-04-13T16:48:02Z

llvm/lib/MC/ELFObjectWriter.cpp

+  unsigned PreRelocType;
+  MCSymbol *PreRelocSymbol;
+  uint64_t PreRelocAddend;
+  if (Backend.fixupNeedsMarkerELFRelocation(Asm, *Fragment, Fixup, PreRelocType,


(MCAssembler.cpp and ELFObjectWriter.cpp have significant tech debt. I try to clean up them from time to time. Just noticed that my latest change 19730e3 might need you to rebase.)

I think we should refrain from adding new hooks. (I added Backend.handleAddSubRelocations in the past, and on reflection we might need some cleanup so that it could be combined with getRelocType).

Can you add an extra fixup when fixup_riscv_qc_e_branch is appended? Or the hook is here because in some scenarios the fixup can be resolved and you don't want to add a redundant marker?

I think we should refrain from adding new hooks.

I do too, but "refrain from" doesn't mean "never". We could not find another way to accomplish this.

Or the hook is here because in some scenarios the fixup can be resolved and you don't want to add a redundant marker?

This. For instance, branches can be evaluated when not relaxing.

From my comment, above:

We didn't want a separate fixup (as for Relaxation) because we didn't want to emit the R_RISCV_VENDOR relocation if the fixup could be evaluated/applied by the compiler, and that kind of "evaluating one fixup deletes another fixup" seemed complex and error-prone.

You could customize evaluateTargetFixup. If the fixup is not resolved, call recordRelocation to append the marker relocation, and then rely on the default recordRelocation in evaluateFixup to append the actual relocation.

I looked into this, and evaluateTargetFixup doesn't get any info about the fixup being resolved (the info comes from this function when the IsTarget fixup flag is given) - to work this way we'd need to pass more information into this function (the RecordReloc bool), and replicate lots of the logic in evaluateFixup which is bypassed if you have the IsTarget fixup flag.

With all this in mind, I continue to think that the new hook is the best way to achieve vendor relocations for RISC-V.

I realise you've been away recently (me too), I'm happy to discuss this further when you're back.

@MaskRay Ping?

I have made the change, and commented about the duplicated code, which I don't like having to copy-paste. There is also complex logic working out when IsResolved might not be enough of a signal that a relocation is required, especially as RISC-V forces relocations during relaxation (which is also hard to test).

Cc @jrtc27 who introduced evaluateTargetFixup

Currently, the MCFixupKindInfo::FKF_IsTarget code path in MCAssembler.cpp skips the common code paths (getAddSym/getSubSym), because PCREL_LO12[IS] don't need them.

Now evaluateTargetFixup is reused by QUIC relocations, leading to some duplication. I think we can make evaluateTargetFixup return more information so that QUIC relocations keep reusing the common code path.

// MCAssembler.cpp if (FixupFlags & MCFixupKindInfo::FKF_IsTarget) { IsResolved = getBackend().evaluateTargetFixup(*this, Fixup, DF, Target, STI, Value); } else { // QUIC relocations should reuse this code path while PCREL_LO12[IS] skip it. const MCSymbol *Add = Target.getAddSym(); 7 refs const MCSymbol *Sub = Target.getSubSym();

The latter half of the duplication that adds a R_RISCV_VENDOR relocation can probably be refactored to reuse Type = TargetObjectWriter->getRelocType(Ctx, Target, Fixup, IsPCRel);

evaluteTargetFixup did exactly what was needed at the time and nothing more. If there are cases now where more of the surrounding code needs reusing then coming up with a way to communicate that seems perfectly reasonable to me.

I've refactored the relocation generation further. There is now an addReloc hook and #140494 demonstrates how to add two relocations from one fixup. For VENDOR, we just need to call .recordRelocation for a specific fixup kind.

Now done. Thanks for the cleanup, it is indeed a much easier structure to work with for vendor relocations.

llvm/test/MC/RISCV/xqcibi-relocations.s

llvm/test/MC/RISCV/xqcili-relocations.s

lenary · 2025-04-29T05:53:07Z

@tclin914 I've tagged you because you'll likely want this functionality for the andes extension with relocations, and it would be good to avoid duplicating this work.

MaskRay · 2025-05-07T06:54:58Z

llvm/test/MC/RISCV/xqcibi-relocations.s

+# Check that we print the correct relocations in exact mode
+.option exact
+qc.e.bgeui x8, 12, foo
+# RELOC: R_RISCV_VENDOR QUALCOMM 0x0


For relocation generation testing we should use llvm-objdump -dr to test r_offset and test that we don't incorrectly emit redundant relocations. See Sparc/Relocations/relocation-specifier.s for a modern example using llvm-objdump -dr

Looks like R_RISCV_VENDOR is only generated with exact. We should test that without exact there is no QUALCOMM. This is unclear from the PR description.

FIXUP from -show-encoding output is not really useful for relocations. Just remove it.

QUALCOMM should be emitted even without exact, we have started to use exact to avoid instruction compression happening and make testing more specific.

I will move to objdump -dr, I agree it's often clearer. I will revisit these tests.

resistor · 2025-05-15T12:11:29Z

Just wanted to note that we (CHERI, and specifically CHERIoT) are very interested in seeing R_RISCV_VENDOR support landed. Is this still moving ahead?

lenary · 2025-05-16T19:00:55Z

@resistor I'm going to try to make some more progress on this today.

This patch implements vendor relocation support for RISC-V, starting with the Xqci extensions. This is done by re-organising `RISCVAsmBackend::evaluateTargetFixup`, to also handle emitting the vendor relocation when the fixup is unresolved. An extra parameter `bool RecordReloc` is added to `evaluateTargetFixup`, so that the relocation is only emitted when it should be, as `evaluateTargetFixup` works the same way. This adds tests showing resolveable and unresolvable symbols for the xqci fixups (some of which are absolute and some of which are pc-relative), including tests when relaxation is enabled which forces relocations that might otherwise not be needed. Co-authored-by: Sudharsan Veeravalli <quic_svs@quicinc.com>

github-actions · 2025-05-16T23:32:40Z

✅ With the latest revision this PR passed the C/C++ code formatter.

MaskRay · 2025-05-17T16:15:35Z

llvm/test/MC/RISCV/xqcilb-relocations.s

-
-.text
+## This test checks that we emit the right relocations for Xqcilb
+## relative jumps. These can be resolved within the same section


These can be resolved within the same section and the destination symbol is local?

Otherwise I think there will be a relocation as well. (We also emit a relocation when the destination is a gnu ifunc symbol, which is an edge case that we don't necessarily describe).

Yeah, I think these always need a relocation. I forgot these are to the PLT, was just focussed on them being pc-relative.

I don't think I found the LLVM code to always emit the relocation (that I expect for call_plt), i will look over it all again.

I added tests to show that for non-local symbols, we still get the relocation, but for local symbols we don't (which matches when call is resolved directly to the symbol in the assembler)

You might want to edit one of the tests to define QUALCOMM: and use llvm-objdump -t to check the symbol table. There will be two QUALCOMM local symbols, which is fine.

I agree a test for this is needed. I'll add it as a separate test, for clarity.

MaskRay · 2025-05-17T16:20:15Z

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

+          cast<MCSymbolELF>(Ctx.createLocalSymbol(VendorIdentifier));
+
+      // Vendor Symbols are Absolute, Local, NOTYPE.
+      VendorSymbol->setType(ELF::STT_NOTYPE);


The symbol is by default NOTYPE and LOCAL. In the rare scenario that the users changes the type/binding, we should respect that.

So setType/setBinding should be omitted.

lenary · 2025-05-28T05:00:34Z

Apologies if this is hard to review vs the previous push because the most recent commit is a merge, but the "all commits" view should be small enough to review.

MaskRay · 2025-05-28T15:47:06Z

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

+
+  // Some Fixups require a vendor relocation, record it (directly) before we add
+  // the relocation.
+  if (!IsResolved || shouldForceRelocation(Fixup, Target)) {


If we are going to call shouldForceRelocation, just avoid the generic MCAsmBackend::addReloc and call the underlying recordRelocation directly.

MaskRay · 2025-05-28T15:57:11Z

llvm/test/MC/RISCV/xqcilb-relocations.s

+qc.e.j same_section
+
+# ASM: qc.e.jal same_section
+# OBJ: qc.e.jal 0x30


If we use OBJ-NEXT:, OBJ-NOT: R_RISCV will not be needed

I thought the obj-not way would be more maintainable for future additions to the tests, but i've changed to the obj-next way.

There are also changes to xqcilb to take in that jump targets are now printed properly.

MaskRay · 2025-05-28T15:58:34Z

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

+  const MCExpr *VendorExpr = MCSymbolRefExpr::create(VendorSymbol, Ctx);
+  MCFixup VendorFixup =
+      MCFixup::create(Fixup.getOffset(), VendorExpr,
+                      FirstLiteralRelocationKind + ELF::R_RISCV_VENDOR);


s/FirstLiteralRelocationKind + ELF::R_RISCV_VENDOR/ELF::R_RISCV_VENDOR/`

lenary · 2025-05-28T21:24:00Z

Failure is unrelated.

MaskRay · 2025-05-29T04:04:57Z

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

@@ -660,7 +698,19 @@ bool RISCVAsmBackend::addReloc(const MCFragment &F, const MCFixup &Fixup,
  if (IsResolved &&
      (getFixupKindInfo(Fixup.getKind()).Flags & MCFixupKindInfo::FKF_IsPCRel))
    IsResolved = isPCRelFixupResolved(Target.getAddSym(), F);
-  IsResolved = MCAsmBackend::addReloc(F, Fixup, Target, FixedValue, IsResolved);
+
+  if (IsResolved && shouldForceRelocation(Fixup, Target))


The default impl shouldForceRelocation just returns false. We should just delete this if and its body

MaskRay · 2025-05-29T04:05:38Z

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

@@ -20,6 +21,7 @@ namespace llvm {
 class MCAssembler;
 class MCObjectTargetWriter;
 class raw_ostream;
+class MCSymbolELF;


The header file doesn't use MCSymbolELF

MaskRay · 2025-05-29T04:08:40Z

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

@@ -47,9 +51,15 @@ class RISCVAsmBackend : public MCAsmBackend {
  bool evaluateTargetFixup(const MCFixup &Fixup, const MCValue &Target,
                           uint64_t &Value) override;

+  std::optional<StringRef>
+  getVendorIdentifierForFixup(unsigned TargetFixupKind) const;


rename this to FixupKind to be consistent with the impl.

(Beside FK_Data_ fixup kinds are almost always target-specific. "Target" probably does not convey extra meaning. )

lenary · 2025-05-31T00:23:34Z

The linux build failure is unrelated, it's an issue with precompiled headers in Flang. I will file a separate ticket about it if there isn't one.

svs-quic · 2025-06-03T10:05:35Z

Thanks for taking this up and working on it, Sam! The changes look good to me.

MaskRay · 2025-06-03T16:05:28Z

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

@@ -38,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) {
+      Is64Bit(Is64Bit), TargetOptions(Options), VendorSymbols() {


delete , VendorSymbols(). just rely the implicit default initialization

MaskRay · 2025-06-03T16:05:59Z

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

+  if (!IsResolved) {
+    // Some Fixups require a vendor relocation, record it (directly) before we
+    // add the relocation.
+    if (std::optional<StringRef> VendorIdentifier =


Combine the two functions and just call sth like maybeAddVendorReloc(F, Fixup)

MaskRay · 2025-06-03T16:10:15Z

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

+  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


This comment is not correct. recordRelocation always append a new relocation, unrelated to how you create a MCValue.

The point in this comment is to say why we didn't use VendorExpr->evaluateAsRelocatable to create VendorTarget - because it would return an MCValue of Constant Zero. I have clarified the comment in this direction.

MaskRay · 2025-06-03T16:12:24Z

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

+                                     StringRef VendorIdentifier) {
+  MCContext &Ctx = Asm->getContext();
+
+  auto It = VendorSymbols.find(VendorIdentifier);


find/try_emplace performs the hash table lookup twice. Just call try_emplace and use the second element of the return value.

Add a comment:

Create a local symbol for the vendor relocation to reference. It's fine if the symbol has the same name as an existing symbol.

MaskRay · 2025-06-03T16:14:50Z

llvm/test/MC/RISCV/xqcilb-relocations.s

-
-.text
+## This test checks that we emit the right relocations for Xqcilb
+## relative jumps. These can be resolved within the same section


You might want to edit one of the tests to define QUALCOMM: and use llvm-objdump -t to check the symbol table. There will be two QUALCOMM local symbols, which is fine.

MaskRay · 2025-06-03T16:15:24Z

llvm/test/MC/RISCV/xqcilb-relocations.s

+# OBJ: qc.e.j 0x0 <this_section>
+# OBJ-NEXT: R_RISCV_VENDOR QUALCOMM{{$}}
+# OBJ-NEXT: R_RISCV_CUSTOM195 undef{{$}}
+qc.e.j undef


In the absence of .option exact, qc.e.j undef will be converted to a shorter-range jal, is that intended?

No, I'll upload a separate patch to fix this.

Addressed in #142702

MaskRay · 2025-06-03T16:17:35Z

Can you edit the description to describe some properties? Like we create a local symbol and a R_RISCV_VENDOR relocation before another one

MaskRay · 2025-06-04T03:44:04Z

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

+void RISCVAsmBackend::maybeAddVendorReloc(const MCFragment &F,
+                                          const MCFixup &Fixup) {
+  MCContext &Ctx = Asm->getContext();
+


I'd delete the first two blank lines.

Done, with a little reorder to not grab the context until as late as possible

MaskRay · 2025-06-04T03:48:24Z

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

+
+  const MCExpr *VendorExpr = MCSymbolRefExpr::create(VendorSymbol, Ctx);
+  MCFixup VendorFixup =
+      MCFixup::create(Fixup.getOffset(), VendorExpr, ELF::R_RISCV_VENDOR);


MCFixup::create(Fixup.getOffset(), nullptr, ELF::R_RISCV_VENDOR) Then delete MCSymbolRefExpr::create . MCFixup::Value isn't used

MaskRay · 2025-06-04T03:49:59Z

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

+    Asm->registerSymbol(*VendorSymbol);
+  } else {
+    // Fetch the existing symbol
+    VendorSymbol = It->getValue();


the objdump -t CHECK lines need some -NOT pattern to check that we don't generate redundat local symbols.

lenary

This has been a long time in review, so I'm probably going to merge it fairly early tomorrow (pacific time) if there are no more comments.

lenary · 2025-06-04T04:13:28Z

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

+    Asm->registerSymbol(*VendorSymbol);
+  } else {
+    // Fetch the existing symbol
+    VendorSymbol = It->getValue();


lenary · 2025-06-04T04:13:44Z

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

+
+  const MCExpr *VendorExpr = MCSymbolRefExpr::create(VendorSymbol, Ctx);
+  MCFixup VendorFixup =
+      MCFixup::create(Fixup.getOffset(), VendorExpr, ELF::R_RISCV_VENDOR);


lenary · 2025-06-04T04:13:59Z

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

+void RISCVAsmBackend::maybeAddVendorReloc(const MCFragment &F,
+                                          const MCFixup &Fixup) {
+  MCContext &Ctx = Asm->getContext();
+


Done, with a little reorder to not grab the context until as late as possible

This patch implements vendor relocation support for RISC-V, starting with the Xqci extensions. Vendor Relocations need to emit the vendor symbol, and add an `R_RISCV_VENDOR` relocation against that symbol before the specific vendor relocation itself. This patch adds a `maybeAddVendorReloc` function which is called from `addReloc`, to implement this functionality. Vendor identifier symbols are cached, so that at most one is emitted per vendor identifier. Vendor identifiers symbols do not interfere with identically named symbols used by assembly. Co-authored-by: Sam Elliott <quic_aelliott@quicinc.com>

llvmbot added backend:RISC-V mc Machine (object) code labels Apr 11, 2025

svs-quic requested review from topperc, pgodeq and hchandel April 11, 2025 16:40

svs-quic requested a review from lenary April 11, 2025 16:40

lenary requested a review from MaskRay April 11, 2025 18:30

topperc reviewed Apr 11, 2025

View reviewed changes

llvm/include/llvm/MC/MCAsmBackend.h Outdated Show resolved Hide resolved

MaskRay reviewed Apr 13, 2025

View reviewed changes

llvm/test/MC/RISCV/xqcibi-relocations.s Outdated Show resolved Hide resolved

MaskRay reviewed Apr 13, 2025

View reviewed changes

llvm/test/MC/RISCV/xqcili-relocations.s Outdated Show resolved Hide resolved

lenary requested a review from tclin914 April 29, 2025 05:51

MaskRay reviewed May 7, 2025

View reviewed changes

lenary force-pushed the vendorsym branch from 0707192 to df04e31 Compare May 16, 2025 23:29

lenary changed the title ~~[RISCV] Add support for vendor relocations on Xqci extensions~~ [RISCV] Vendor Relocations for Xqci extension May 16, 2025

clang-format

1e43085

lenary removed their request for review May 17, 2025 01:46

MaskRay reviewed May 17, 2025

View reviewed changes

MaskRay mentioned this pull request May 19, 2025

RISCV,LoongArch: Encode RELAX relocation implicitly #140494

Merged

Merge branch 'main' into vendorsym

1133c3d

lenary requested a review from MaskRay May 28, 2025 05:00

MaskRay reviewed May 28, 2025

View reviewed changes

lenary added 2 commits May 28, 2025 11:02

Merge branch 'main' into vendorsym

98dfeed

Address Review Feedback

723863f

MaskRay reviewed May 29, 2025

View reviewed changes

Address review comments

123e312

Merge branch 'main' into vendorsym

0f84104

MaskRay reviewed Jun 3, 2025

View reviewed changes

lenary added 3 commits June 3, 2025 10:58

Merge branch 'main' into vendorsym

acc71d4

Address feedback

d408f17

clang-format

241ea1b

MaskRay approved these changes Jun 4, 2025

View reviewed changes

lenary added 2 commits June 3, 2025 21:11

Address review feedback

6b3ba87

Merge branch 'main' into vendorsym

7234bf4

lenary reviewed Jun 4, 2025

View reviewed changes

lenary merged commit c83c01f into llvm:main Jun 4, 2025
11 checks passed

svs-quic deleted the vendorsym branch June 6, 2025 05:18

[RISCV] Vendor Relocations for Xqci extension #135400

[RISCV] Vendor Relocations for Xqci extension #135400

Uh oh!

Conversation

svs-quic commented Apr 11, 2025 • edited by lenary Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lenary commented Apr 11, 2025

Uh oh!

lenary commented Apr 11, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaskRay May 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lenary commented Apr 29, 2025

Uh oh!

MaskRay May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

resistor commented May 15, 2025

Uh oh!

lenary commented May 16, 2025

Uh oh!

github-actions bot commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lenary commented May 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

svs-quic commented Apr 11, 2025 •

edited by lenary

Loading

llvmbot commented Apr 11, 2025 •

edited

Loading

MaskRay May 17, 2025 •

edited

Loading

MaskRay May 7, 2025 •

edited

Loading

github-actions bot commented May 16, 2025 •

edited

Loading