RISCV: Don't use MCTargetOptions::ABIName in the ELF target streamer

[arsenm]

The abi name should come from the target-abi module flag in codegen,
which should be set up in the AsmPrinter. The ABI name field should
only be of practical use in the assembler, which reads the flag in
onBeginOfFile.

Co-Authored-By: Claude noreply@anthropic.com (Claude Opus 4.8)

[arsenm]

• RISCV: Don't use MCTargetOptions::ABIName in the ELF target streamer #224704 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[llvmorg-github-actions]

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

Author: Matt Arsenault (arsenm)

Changes

The abi name should come from the target-abi module flag in codegen,
which should be set up in the AsmPrinter. The ABI name field should
only be of practical use in the assembler, which reads the flag in
onBeginOfFile.

Co-Authored-By: Claude <noreply@anthropic.com> (Claude Opus 4.8)

---

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

2 Files Affected:

• (modified) llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp (-1)
• (modified) llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp (-18)

diff --git a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
index 4f610a49f96998..b184e8d7869e79 100644
--- a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
+++ b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
@@ -344,7 +344,6 @@ class RISCVAsmParser : public MCTargetAsmParser {
   // location instead of being printed with no location information.
   void onBeginOfFile() override {
     // If the target streamer already has a resolved ABI (e.g. set by
-    // RISCVTargetELFStreamer for a valid -target-abi, or set by
     // RISCVAsmPrinter during codegen), skip ABI validation.
     if (getTargetStreamer().hasTargetABI())
       return;
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
index 1fb45627a1d03f..3a86e2e3e56314 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
@@ -11,7 +11,6 @@
 //===----------------------------------------------------------------------===//
 
 #include "RISCVELFStreamer.h"
-#include "RISCVAsmBackend.h"
 #include "RISCVBaseInfo.h"
 #include "RISCVMCTargetDesc.h"
 #include "llvm/BinaryFormat/ELF.h"
@@ -28,23 +27,6 @@ using namespace llvm;
 RISCVTargetELFStreamer::RISCVTargetELFStreamer(MCStreamer &S,
                                                const MCSubtargetInfo &STI)
     : RISCVTargetStreamer(S), CurrentVendor("riscv") {
-  MCAssembler &MCA = getStreamer().getAssembler();
-  auto &MAB = static_cast<RISCVAsmBackend &>(MCA.getBackend());
-  StringRef ABIName = MAB.getTargetOptions().getABIName();
-  // We have to recompute the ABI rather than casting STI to RISCVSubtarget
-  // since MC tools like llvm-mc call this when STI is MCSubtargetInfo instead.
-  // Using RISCVSubtarget requires a TargetMachine, which the MC-only tools
-  // deliberately don't link.
-  // TODO: Might be cleaner to have callers set the ABI instead of computing
-  // it twice which introduces a chance of it being out of sync.
-  if (auto ABIOrErr = RISCVABI::computeTargetABI(STI, ABIName)) {
-    setTargetABI(*ABIOrErr);
-  } else {
-    // Do not set TargetABI here if invalid: RISCVSubtarget/RISCVAsmPrinter
-    // (in codegen) or RISCVAsmParser::onBeginOfFile() (in llvm-mc) will
-    // resolve or diagnose it with proper contexts.
-    consumeError(ABIOrErr.takeError());
-  }
   setFlagsFromFeatures(STI);
 
   // Compute the initial ISA string.  This serves two purposes:

[lenary]

I don't know if this is a problem but I'm going to comment about it anyway.

It's not clear what we do here for setting the e_flags correctly on assembly files. This is done in RISCVTargetELFStreamer::finish, and right now ABI_Unknown will cause that function to fault in asserts builds.

I guess most files won't be assembled with -target-abi today anyway, so maybe the hard float bits won't be set usually.

[arsenm]

I don't know if this is a problem but I'm going to comment about it anyway.

It's not clear what we do here for setting the e_flags correctly on assembly files. This is done in RISCVTargetELFStreamer::finish, and right now ABI_Unknown will cause that function to fault in asserts builds.

I guess most files won't be assembled with -target-abi today anyway, so maybe the hard float bits won't be set usually.

I would hope if it's really needed there would be an assembler directive this would be recoverable from in the asm file

[arsenm]

onBeginOfFile also computes the ABI, so this just leaves it deferred to that point. The real in tree MC and codegen uses can't get there. Not sure if this matters for some other strange tool

[arichardson]

I'd like to see this cleanup land but I've seen some odd interactions with LTO and inline ASM. Give me a bit of time to investigate. Currently working on some tests and cleanups for this (motivated by #223606)

[arichardson]

The current diff will break llc -filetype=obj -target-abi=<abi> when the input IR does not have a "target-abi" module flag (I added test coverage for this in #225128).

Not sure if we should make the target-abi metadata mandatory for RISC-V and emit an error to avoid those issues? Or maybe we just fall back to the command line option if it is missing from the input IR?

[lenary]

The current diff will break llc -filetype=obj -target-abi=<abi> when the input IR does not have a "target-abi" module flag (I added test coverage for this in #225128).

Not sure if we should make the target-abi metadata mandatory for RISC-V and emit an error to avoid those issues? Or maybe we just fall back to the command line option if it is missing from the input IR?

Presumably in that case we can turn the -target-abi choice into Module metadata in LLC?

[jrtc27]

Treating it like we do -target(/-triple/-mtriple?) would seem sensible (especially with the "warn on override" part)

[arsenm]

Not sure if we should make the target-abi metadata mandatory for RISC-V and emit an error to avoid those issues?

A mandatory flag would be pretty terrible

Or maybe we just fall back to the command line option if it is missing from the input IR?
Presumably in that case we can turn the -target-abi choice into Module metadata in LLC?

Yes, the flag is just a lit test convenience and that's how the other similar flags are handled. The ultimate goal is remove the MCTargetOptions field completely, with the target-abi module flag winning when present. verifyOptionsConsistency is supposed to ensure the module flag and MCTargetOptions field agree today

[lenary]

LGTM