[RISCV] Resolve an explicit I+E extension set to I

[tclin914]

Linking an rv32i object with an rv32e object merges their arch attributes into an extension map holding both I and E, and createFromExtMap()'s updateImplication() resolved that to E, giving an rv32e output. That is wrong: RVE (16 GPRs) is a subset of RVI (32 GPRs), so a binary combining RVE and RVI code needs the full RVI base.

updateImplication() erased I unconditionally to strip the default I that the generic-rv32/rv64 CPU injects when compiling -march=rv32e, so that the explicitly requested E wins. Move that stripping to parseFeatures(), where the default I and the requested E actually meet: drop I whenever E is present. The only remaining way both bases reach updateImplication() is an explicit extension map (e.g. merged arch attributes), so resolve that in favor of I instead.

[llvmorg-github-actions]

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

@llvm/pr-subscribers-lld

Author: Jim Lin (tclin914)

Changes

Linking an rv32i object with an rv32e object merges their arch attributes into an extension map holding both I and E, and createFromExtMap()'s updateImplication() resolved that to E, giving an rv32e output. That is wrong: RVE (16 GPRs) is a subset of RVI (32 GPRs), so a binary combining RVE and RVI code needs the full RVI base.

updateImplication() erased I unconditionally to strip the default I that the generic-rv32/rv64 CPU injects when compiling -march=rv32e, so that the explicitly requested E wins. Move that stripping to parseFeatures(), where the default I and the requested E actually meet: drop I whenever E is present. The only remaining way both bases reach updateImplication() is an explicit extension map (e.g. merged arch attributes), so resolve that in favor of I instead.

---

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

2 Files Affected:

• (modified) lld/test/ELF/riscv-attributes.s (+31)
• (modified) llvm/lib/TargetParser/RISCVISAInfo.cpp (+8-3)

diff --git a/lld/test/ELF/riscv-attributes.s b/lld/test/ELF/riscv-attributes.s
index c6666063396d4..867b1b1a3434a 100644
--- a/lld/test/ELF/riscv-attributes.s
+++ b/lld/test/ELF/riscv-attributes.s
@@ -30,6 +30,14 @@
 # RUN: ld.lld -e 0 unrecognized_version.o merge_version_test_input.o -o out3 2>&1 | count 0
 # RUN: llvm-readobj --arch-specific out3 | FileCheck %s --check-prefix=CHECK3
 
+## Merging RVE and RVI yields RVI, because RVE is a subset of RVI.
+# RUN: llvm-mc -filetype=obj -triple=riscv32 rv32i.s -o rv32i.o
+# RUN: llvm-mc -filetype=obj -triple=riscv32 rv32e.s -o rv32e.o
+# RUN: ld.lld -e 0 rv32i.o rv32e.o -o rv32ie 2>&1 | count 0
+# RUN: llvm-readobj --arch-specific rv32ie | FileCheck %s --check-prefix=RV32IE
+# RUN: ld.lld -e 0 rv32e.o rv32i.o -o rv32ei 2>&1 | count 0
+# RUN: llvm-readobj --arch-specific rv32ei | FileCheck %s --check-prefix=RV32IE
+
 # RUN: llvm-mc -filetype=obj -triple=riscv64 invalid_arch1.s -o invalid_arch1.o
 # RUN: not ld.lld invalid_arch1.o 2>&1 | FileCheck %s --check-prefix=INVALID_ARCH1 --implicit-check-not=error:
 # INVALID_ARCH1: error: invalid_arch1.o:(.riscv.attributes): rv64i2: extension lacks version in expected format
@@ -304,6 +312,29 @@
 .asciz "rv64i2p1"
 .Lend:
 
+#--- rv32i.s
+.attribute arch, "rv32i2p1"
+
+#--- rv32e.s
+.attribute arch, "rv32e2p0"
+
+# RV32IE:      BuildAttributes {
+# RV32IE-NEXT:   FormatVersion: 0x41
+# RV32IE-NEXT:   Section 1 {
+# RV32IE-NEXT:     SectionLength: 25
+# RV32IE-NEXT:     Vendor: riscv
+# RV32IE-NEXT:     Tag: Tag_File (0x1)
+# RV32IE-NEXT:     Size: 15
+# RV32IE-NEXT:     FileAttributes {
+# RV32IE-NEXT:       Attribute {
+# RV32IE-NEXT:         Tag: 5
+# RV32IE-NEXT:         TagName: arch
+# RV32IE-NEXT:         Value: rv32i2p1{{$}}
+# RV32IE-NEXT:       }
+# RV32IE-NEXT:     }
+# RV32IE-NEXT:   }
+# RV32IE-NEXT: }
+
 #--- invalid_arch1.s
 .section .riscv.attributes,"",@0x70000003
 .byte 0x41
diff --git a/llvm/lib/TargetParser/RISCVISAInfo.cpp b/llvm/lib/TargetParser/RISCVISAInfo.cpp
index 3e9664e6ba070..52a3d31632c39 100644
--- a/llvm/lib/TargetParser/RISCVISAInfo.cpp
+++ b/llvm/lib/TargetParser/RISCVISAInfo.cpp
@@ -466,6 +466,11 @@ RISCVISAInfo::parseFeatures(unsigned XLen,
       ISAInfo->Exts.erase(ExtName.str());
   }
 
+  // I and E are mutually exclusive. When E is requested, drop the default I
+  // (e.g. injected by the generic CPU) so the two do not conflict.
+  if (ISAInfo->Exts.count("e"))
+    ISAInfo->Exts.erase("i");
+
   return RISCVISAInfo::postProcessAndChecking(std::move(ISAInfo));
 }
 
@@ -966,10 +971,10 @@ void RISCVISAInfo::updateImplication() {
   if (!HasE && !HasI) {
     auto Version = findDefaultVersion("i");
     Exts["i"] = *Version;
+  } else if (HasE && HasI) {
+    // Keep the 32-register i, of which e is a subset.
+    Exts.erase("e");
   }
-
-  if (HasE && HasI)
-    Exts.erase("i");
 }
 
 static constexpr StringLiteral CombineIntoExts[] = {

[lenary]

Why do we need special handling for I/E when the tag_arch build attribute can end up having conflicting extensions anyway?

I guess we need to make sure we set the e_flags correctly (likely already tested), but I'm not sure what we need beyond that?

Does this help disassembly be more accurate? That might be a reason to do it.

Broadly, the fix you propose looks like it does what you claim, I'm just not sure how we're supposed to deal with I/E conflicts in build attributes, and if there even is a principled way to do so.

[jrtc27]

I do wonder why we even bother with the build attribute at this point. All it does is cause headaches, just so we can get out a meaningless, internally-inconsistent, impossible, unused, useless pile of ASCII that alleges to be a RISC-V ISA string. When it was first added to the psABI, it was to satisfy the request that came down from RVI on high, that recognised even back then the diverse set of extensions available and the potential for poor UX when running on unsupported hardware, so wanted binaries to encode their requirements in order to get out a friendly error message. But that never materialised, because the idea this could be done was fundamentally flawed and ignored the realities of how real software is built and linked. So why are we still wasting precious time and effort on something that has no intrinsic value (with the only thing it was ever vaguely good for, influencing disassembly, rendered entirely redundant by the precise and accurate mapping symbols)?

[jrtc27]

Hm, the spec in fact still says:

The linker should merge the different architectures into a superset when object files are merged, and should report errors if the merge result contains conflict extensions.

which means you can't do dynamic dispatch between F and Zfinx, so even despite the overly-relaxed rules it still blocks actual reasonable uses. And this is something LLD is still enforcing today.

[jrtc27]

Isn't this what updateImplication is for? Why does it need to be done here too?

[jrtc27]

Is the one character fix for this bug not just changing the i to an e here, which is obviously the correct thing to do in order to get the psABI-mandated superset?

[jrtc27]

Regarding generic-rv32/64 in the summary: that just sounds like hacking around a broken design, so I am not in favour of this.

[topperc]

Is this for the tests that pass -mattr=+e?