Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RISCV] Use 'riscv-isa' module flag to set ELF flags and attributes. #85155

Merged
merged 1 commit into from
Mar 20, 2024

Conversation

topperc
Copy link
Collaborator

@topperc topperc commented Mar 13, 2024

Walk all the ISA strings and set the subtarget bits for any extension we find in any string.

This allows LTO output to have a ELF attributes from the union of all of the files used to compile it.

Walk all the ISA strings and set the subtarget bits for any extension
we find in any string.

This allows LTO output to have a ELF attributes from the union of
all of the files used to compile it.
@llvmbot
Copy link
Member

llvmbot commented Mar 13, 2024

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

Author: Craig Topper (topperc)

Changes

Walk all the ISA strings and set the subtarget bits for any extension we find in any string.

This allows LTO output to have a ELF attributes from the union of all of the files used to compile it.


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

3 Files Affected:

  • (modified) llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp (+28-4)
  • (added) llvm/test/CodeGen/RISCV/attributes-module-flag.ll (+17)
  • (added) llvm/test/CodeGen/RISCV/module-elf-flags.ll (+13)
diff --git a/llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp b/llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
index cb82e74b1efacd..5bf594c0b5eae3 100644
--- a/llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
+++ b/llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
@@ -100,7 +100,7 @@ class RISCVAsmPrinter : public AsmPrinter {
   bool emitDirectiveOptionArch();
 
 private:
-  void emitAttributes();
+  void emitAttributes(const MCSubtargetInfo &SubtargetInfo);
 
   void emitNTLHint(const MachineInstr *MI);
 
@@ -385,8 +385,32 @@ void RISCVAsmPrinter::emitStartOfAsmFile(Module &M) {
   if (const MDString *ModuleTargetABI =
           dyn_cast_or_null<MDString>(M.getModuleFlag("target-abi")))
     RTS.setTargetABI(RISCVABI::getTargetABI(ModuleTargetABI->getString()));
+
+  MCSubtargetInfo SubtargetInfo = *TM.getMCSubtargetInfo();
+
+  // Use module flag to update feature bits.
+  if (auto *MD = dyn_cast_or_null<MDNode>(M.getModuleFlag("riscv-isa"))) {
+    for (auto &ISA : MD->operands()) {
+      if (auto *ISAString = dyn_cast_or_null<MDString>(ISA)) {
+        auto ParseResult = llvm::RISCVISAInfo::parseArchString(
+            ISAString->getString(), /*EnableExperimentalExtension=*/true,
+            /*ExperimentalExtensionVersionCheck=*/true);
+        if (!errorToBool(ParseResult.takeError())) {
+          auto &ISAInfo = *ParseResult;
+          for (const auto &Feature : RISCVFeatureKV) {
+            if (ISAInfo->hasExtension(Feature.Key) &&
+                !SubtargetInfo.hasFeature(Feature.Value))
+              SubtargetInfo.ToggleFeature(Feature.Key);
+          }
+        }
+      }
+    }
+
+    RTS.setFlagsFromFeatures(SubtargetInfo);
+  }
+
   if (TM.getTargetTriple().isOSBinFormatELF())
-    emitAttributes();
+    emitAttributes(SubtargetInfo);
 }
 
 void RISCVAsmPrinter::emitEndOfAsmFile(Module &M) {
@@ -398,13 +422,13 @@ void RISCVAsmPrinter::emitEndOfAsmFile(Module &M) {
   EmitHwasanMemaccessSymbols(M);
 }
 
-void RISCVAsmPrinter::emitAttributes() {
+void RISCVAsmPrinter::emitAttributes(const MCSubtargetInfo &SubtargetInfo) {
   RISCVTargetStreamer &RTS =
       static_cast<RISCVTargetStreamer &>(*OutStreamer->getTargetStreamer());
   // Use MCSubtargetInfo from TargetMachine. Individual functions may have
   // attributes that differ from other functions in the module and we have no
   // way to know which function is correct.
-  RTS.emitTargetAttributes(*TM.getMCSubtargetInfo(), /*EmitStackAlign*/ true);
+  RTS.emitTargetAttributes(SubtargetInfo, /*EmitStackAlign*/ true);
 }
 
 void RISCVAsmPrinter::emitFunctionEntryLabel() {
diff --git a/llvm/test/CodeGen/RISCV/attributes-module-flag.ll b/llvm/test/CodeGen/RISCV/attributes-module-flag.ll
new file mode 100644
index 00000000000000..4580539fbb29b9
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/attributes-module-flag.ll
@@ -0,0 +1,17 @@
+; RUN: llc -mtriple=riscv32 %s -o - | FileCheck %s --check-prefix=RV32
+; RUN: llc -mtriple=riscv64 %s -o - | FileCheck %s --check-prefix=RV64
+
+; Test generation of ELF attribute from module metadata
+
+; RV32: .attribute 5, "rv32i2p1_m2p0_zba1p0"
+; RV64: .attribute 5, "rv64i2p1_m2p0_zba1p0"
+
+define i32 @addi(i32 %a) {
+  %1 = add i32 %a, 1
+  ret i32 %1
+}
+
+!llvm.module.flags = !{!0}
+
+!0 = !{i32 6, !"riscv-isa", !1}
+!1 = !{!"rv64i2p1_m2p0", !"rv64i2p1_zba1p0"}
diff --git a/llvm/test/CodeGen/RISCV/module-elf-flags.ll b/llvm/test/CodeGen/RISCV/module-elf-flags.ll
new file mode 100644
index 00000000000000..1b4bc9fd5466cc
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/module-elf-flags.ll
@@ -0,0 +1,13 @@
+; RUN: llc -mtriple=riscv32 -filetype=obj < %s | llvm-readelf -h - | FileCheck -check-prefixes=FLAGS %s
+
+; FLAGS: Flags: 0x11, RVC, TSO
+
+define i32 @addi(i32 %a) {
+  %1 = add i32 %a, 1
+  ret i32 %1
+}
+
+!llvm.module.flags = !{!0}
+
+!0 = !{i32 6, !"riscv-isa", !1}
+!1 = !{!"rv64i2p1_c2p0_ztso0p1"}

@dtcxzyw dtcxzyw requested a review from MaskRay March 14, 2024 06:31
Comment on lines +6 to +7
; RV32: .attribute 5, "rv32i2p1_m2p0_zba1p0"
; RV64: .attribute 5, "rv64i2p1_m2p0_zba1p0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to double check that we don't need to test the merging w/ llvm-lto2, do we? I //think// this test is equivalent to what would happen in that case, so I believe we're OK, but I'm not 100%.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My intent was that this test is what would happen in that case. I can add an llvm-lto2 test for the merging if reviewers want.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think that’s necessary. I’m pretty sure you’re right and this is equivalent.

Copy link
Contributor

@ilovepi ilovepi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@topperc topperc merged commit 891172d into llvm:main Mar 20, 2024
6 checks passed
@topperc topperc deleted the pr/module-flag branch March 20, 2024 18:35
chencha3 pushed a commit to chencha3/llvm-project that referenced this pull request Mar 23, 2024
…lvm#85155)

Walk all the ISA strings and set the subtarget bits for any extension we
find in any string.

This allows LTO output to have a ELF attributes from the union of all of
the files used to compile it.
@ilovepi
Copy link
Contributor

ilovepi commented May 7, 2024

@topperc should this be cherry-picked into the llvm 18 release branch?

@topperc
Copy link
Collaborator Author

topperc commented May 7, 2024

@topperc should this be cherry-picked into the llvm 18 release branch?

I guess it would be nice, but its not fixing a regression from a previous version so I'm not sure if it would be accepted this late.

@ilovepi
Copy link
Contributor

ilovepi commented May 7, 2024

Doesnt' this fix the issue w/ the Float ABI in LTO, where some LTO'd targets wouldn't get the ABI propagated? Also, if we land this in the release branch, Rust will pick it up, which is probably a win for anyone LTOing Rust code.

topperc added a commit to topperc/llvm-project that referenced this pull request May 8, 2024
…lvm#85155)

Walk all the ISA strings and set the subtarget bits for any extension we
find in any string.

This allows LTO output to have a ELF attributes from the union of all of
the files used to compile it.
topperc added a commit to topperc/llvm-project that referenced this pull request May 8, 2024
…lvm#85155)

Walk all the ISA strings and set the subtarget bits for any extension we
find in any string.

This allows LTO output to have a ELF attributes from the union of all of
the files used to compile it.
tstellar pushed a commit to topperc/llvm-project that referenced this pull request May 13, 2024
…lvm#85155)

Walk all the ISA strings and set the subtarget bits for any extension we
find in any string.

This allows LTO output to have a ELF attributes from the union of all of
the files used to compile it.
tstellar pushed a commit to topperc/llvm-project that referenced this pull request May 14, 2024
…lvm#85155)

Walk all the ISA strings and set the subtarget bits for any extension we
find in any string.

This allows LTO output to have a ELF attributes from the union of all of
the files used to compile it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants