Use Module level target-abi to assign target features for codegenerated functions. #100833

hiraditya · 2024-07-26T23:30:01Z

It is currently not possible to provide any reasonable target-features for compiler generated functions (See: #69780). Having a target-abi will provide a way to add minimal requirements for target-features like +d for RISC-V.

llvmbot · 2024-07-26T23:30:35Z

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

@llvm/pr-subscribers-lto

Author: AdityaK (hiraditya)

Changes

Module can be used to query target-abi (follow up patch) which can be used to populate default subtarget features. It is currently not possible to provide any reasonable target-features for compiler generated functions (See: #69780) Having a target-abi will provide a way to add minimal requirements for target-features like +d for RISC-V.

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

7 Files Affected:

(modified) llvm/include/llvm/LTO/legacy/ThinLTOCodeGenerator.h (+1-1)
(modified) llvm/include/llvm/TargetParser/SubtargetFeature.h (+2-1)
(modified) llvm/lib/LTO/LTOBackend.cpp (+1-1)
(modified) llvm/lib/LTO/LTOCodeGenerator.cpp (+1-1)
(modified) llvm/lib/LTO/LTOModule.cpp (+1-1)
(modified) llvm/lib/LTO/ThinLTOCodeGenerator.cpp (+7-5)
(modified) llvm/lib/TargetParser/SubtargetFeature.cpp (+2-1)

diff --git a/llvm/include/llvm/LTO/legacy/ThinLTOCodeGenerator.h b/llvm/include/llvm/LTO/legacy/ThinLTOCodeGenerator.h
index f1337e82485c9..50375f701cecf 100644
--- a/llvm/include/llvm/LTO/legacy/ThinLTOCodeGenerator.h
+++ b/llvm/include/llvm/LTO/legacy/ThinLTOCodeGenerator.h
@@ -40,7 +40,7 @@ struct TargetMachineBuilder {
   std::optional<Reloc::Model> RelocModel;
   CodeGenOptLevel CGOptLevel = CodeGenOptLevel::Aggressive;
 
-  std::unique_ptr<TargetMachine> create() const;
+  std::unique_ptr<TargetMachine> create(const Module &M) const;
 };
 
 /// This class define an interface similar to the LTOCodeGenerator, but adapted
diff --git a/llvm/include/llvm/TargetParser/SubtargetFeature.h b/llvm/include/llvm/TargetParser/SubtargetFeature.h
index 2e1f00dad2df3..d75db9210495b 100644
--- a/llvm/include/llvm/TargetParser/SubtargetFeature.h
+++ b/llvm/include/llvm/TargetParser/SubtargetFeature.h
@@ -20,6 +20,7 @@
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/IR/Module.h"
 #include "llvm/Support/MathExtras.h"
 #include <array>
 #include <initializer_list>
@@ -195,7 +196,7 @@ class SubtargetFeatures {
   void dump() const;
 
   /// Adds the default features for the specified target triple.
-  void getDefaultSubtargetFeatures(const Triple& Triple);
+  void getDefaultSubtargetFeatures(const Module &M);
 
   /// Determine if a feature has a flag; '+' or '-'
   static bool hasFlag(StringRef Feature) {
diff --git a/llvm/lib/LTO/LTOBackend.cpp b/llvm/lib/LTO/LTOBackend.cpp
index d5d642f0d25e6..43fab8c941e77 100644
--- a/llvm/lib/LTO/LTOBackend.cpp
+++ b/llvm/lib/LTO/LTOBackend.cpp
@@ -201,7 +201,7 @@ static std::unique_ptr<TargetMachine>
 createTargetMachine(const Config &Conf, const Target *TheTarget, Module &M) {
   StringRef TheTriple = M.getTargetTriple();
   SubtargetFeatures Features;
-  Features.getDefaultSubtargetFeatures(Triple(TheTriple));
+  Features.getDefaultSubtargetFeatures(M);
   for (const std::string &A : Conf.MAttrs)
     Features.AddFeature(A);
 
diff --git a/llvm/lib/LTO/LTOCodeGenerator.cpp b/llvm/lib/LTO/LTOCodeGenerator.cpp
index 34aacb660144f..18826d0c78f1f 100644
--- a/llvm/lib/LTO/LTOCodeGenerator.cpp
+++ b/llvm/lib/LTO/LTOCodeGenerator.cpp
@@ -406,7 +406,7 @@ bool LTOCodeGenerator::determineTarget() {
   // Construct LTOModule, hand over ownership of module and target. Use MAttr as
   // the default set of features.
   SubtargetFeatures Features(join(Config.MAttrs, ""));
-  Features.getDefaultSubtargetFeatures(Triple);
+  Features.getDefaultSubtargetFeatures(*MergedModule);
   FeatureStr = Features.getString();
   if (Config.CPU.empty())
     Config.CPU = lto::getThinLTODefaultCPU(Triple);
diff --git a/llvm/lib/LTO/LTOModule.cpp b/llvm/lib/LTO/LTOModule.cpp
index eac78069f4d2b..e77c4a7118628 100644
--- a/llvm/lib/LTO/LTOModule.cpp
+++ b/llvm/lib/LTO/LTOModule.cpp
@@ -213,7 +213,7 @@ LTOModule::makeLTOModule(MemoryBufferRef Buffer, const TargetOptions &options,
 
   // construct LTOModule, hand over ownership of module and target
   SubtargetFeatures Features;
-  Features.getDefaultSubtargetFeatures(Triple);
+  Features.getDefaultSubtargetFeatures(*M.get());
   std::string FeatureStr = Features.getString();
   // Set a default CPU for Darwin triples.
   std::string CPU;
diff --git a/llvm/lib/LTO/ThinLTOCodeGenerator.cpp b/llvm/lib/LTO/ThinLTOCodeGenerator.cpp
index 57c94e1988b79..62b58de3ddbdd 100644
--- a/llvm/lib/LTO/ThinLTOCodeGenerator.cpp
+++ b/llvm/lib/LTO/ThinLTOCodeGenerator.cpp
@@ -582,7 +582,7 @@ void ThinLTOCodeGenerator::crossReferenceSymbol(StringRef Name) {
 }
 
 // TargetMachine factory
-std::unique_ptr<TargetMachine> TargetMachineBuilder::create() const {
+std::unique_ptr<TargetMachine> TargetMachineBuilder::create(const Module &M) const {
   std::string ErrMsg;
   const Target *TheTarget =
       TargetRegistry::lookupTarget(TheTriple.str(), ErrMsg);
@@ -592,7 +592,7 @@ std::unique_ptr<TargetMachine> TargetMachineBuilder::create() const {
 
   // Use MAttr as the default set of features.
   SubtargetFeatures Features(MAttr);
-  Features.getDefaultSubtargetFeatures(TheTriple);
+  Features.getDefaultSubtargetFeatures(M);
   std::string FeatureStr = Features.getString();
 
   std::unique_ptr<TargetMachine> TM(
@@ -920,7 +920,7 @@ void ThinLTOCodeGenerator::optimize(Module &TheModule) {
   initTMBuilder(TMBuilder, Triple(TheModule.getTargetTriple()));
 
   // Optimize now
-  optimizeModule(TheModule, *TMBuilder.create(), OptLevel, Freestanding,
+  optimizeModule(TheModule, *TMBuilder.create(TheModule), OptLevel, Freestanding,
                  DebugPassManager, nullptr);
 }
 
@@ -997,7 +997,8 @@ void ThinLTOCodeGenerator::run() {
                                              /*IsImporting*/ false);
 
         // CodeGen
-        auto OutputBuffer = codegenModule(*TheModule, *TMBuilder.create());
+        TargetMachine &TM = *TMBuilder.create(*TheModule);
+        auto OutputBuffer = codegenModule(*TheModule, TM);
         if (SavedObjectsDirectoryPath.empty())
           ProducedBinaries[count] = std::move(OutputBuffer);
         else
@@ -1185,9 +1186,10 @@ void ThinLTOCodeGenerator::run() {
         saveTempBitcode(*TheModule, SaveTempsDir, count, ".0.original.bc");
 
         auto &ImportList = ImportLists[ModuleIdentifier];
+        TargetMachine &TM = *TMBuilder.create(*TheModule);
         // Run the main process now, and generates a binary
         auto OutputBuffer = ProcessThinLTOModule(
-            *TheModule, *Index, ModuleMap, *TMBuilder.create(), ImportList,
+            *TheModule, *Index, ModuleMap, TM, ImportList,
             ExportList, GUIDPreservedSymbols,
             ModuleToDefinedGVSummaries[ModuleIdentifier], CacheOptions,
             DisableCodeGen, SaveTempsDir, Freestanding, OptLevel, count,
diff --git a/llvm/lib/TargetParser/SubtargetFeature.cpp b/llvm/lib/TargetParser/SubtargetFeature.cpp
index 2c51c403c1934..e1fd039199bf1 100644
--- a/llvm/lib/TargetParser/SubtargetFeature.cpp
+++ b/llvm/lib/TargetParser/SubtargetFeature.cpp
@@ -68,7 +68,8 @@ LLVM_DUMP_METHOD void SubtargetFeatures::dump() const {
 }
 #endif
 
-void SubtargetFeatures::getDefaultSubtargetFeatures(const Triple& Triple) {
+void SubtargetFeatures::getDefaultSubtargetFeatures(const Module &M) {
+  llvm::Triple Triple(M.getTargetTriple());
   // FIXME: This is an inelegant way of specifying the features of a
   // subtarget. It would be better if we could encode this information
   // into the IR.

github-actions · 2024-07-26T23:33:54Z

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

echristo · 2024-08-05T16:22:31Z

I need to take a closer look at this, but wanted to let you know I am reviewing. :)

ilovepi

Left some small nits and questions. This mostly seems fine, but lets see if anyone more experienced w/ the ABI features disagrees or sees an obvious foot-gun.

Off the top of my head @topperc has done some work in this area. Maybe he can suggest another person to check in with?

llvm/lib/LTO/LTOBackend.cpp

llvm/lib/TargetParser/SubtargetFeature.cpp

llvm/lib/LTO/LTOBackend.cpp

llvm/lib/TargetParser/SubtargetFeature.cpp

ilovepi

LGTM.

topperc

LGTM

llvm/lib/TargetParser/SubtargetFeature.cpp

topperc · 2024-09-25T16:31:38Z

The ABI might be in the TargetMachine via Options.MCOptions.getABIName().

RISCVTargetMachine::getSubtargetImpl checks that the module ABI string matches it.

llvm/lib/IR/Function.cpp

arichardson

Seems sensible overall but I would like to see more test coverage.

arichardson · 2025-01-30T17:54:33Z

llvm/lib/IR/Function.cpp

+  } else if (T.isRISCV32() && TargetABI.contains("ilp32f")) {
+    attr = "+f";
+  } else if (T.isARM() || T.isThumb()) {
+    attr = "+thumb-mode";


Why is thumb on by default for Arm? What happens when I build with -mno-thumb? Could you add test for Arm as well?

it was copied verbatim from https://github.com/llvm/llvm-project/pull/100833/files#diff-9a9f66909bad563f6fd55dff0dc0dc0a176d27514cf7aef192325c921007fd8eL94-L96

We shouldn't have any cpu specific code not in the backends like this. This really needs to be a part of individual backends or the code that constructs the IR depending on use.

that makes sense, i'm struggling to find any existing backend API that can be leveraged here.

Well, you can't use a backend API here because this is the middle-end and you don't know the backend exists.

Yeah, if the stub function has particular properties on a per target basis that aren't inherited from the calling function the pass should call into the backend for them if possible and merge with the calling function appropriately (ala some of the inlining checks). For the generic case they should probably just inherit (unless I'm missing something specific to the pass here @pcc?)

__cfi_check is normally called by the CFI runtime library so there isn't really a known caller per se. For __cfi_check I think the attributes should be generated by Clang based on what it would normally add to a function without target attributes, i.e. what #78253 is doing aside from the noted change for Thumb which could be implemented in a target-specific class e.g. as part of clang::TargetInfo.

Aha, I thought it was being inserted as part of the pass! Thanks. Yes, that's definitely what should be done here :)

so #78253 should have added +thumb-mode to features if not already present.

maybe that's why this pass adds "+thumb-mode". Makes sense. As of now it seems this patch can be reduced to asserting that the stub function exists? And maybe a separate patch to add +thumb-mode in clang.

maybe that's why this pass adds "+thumb-mode"

Yes, exactly.

And maybe a separate patch to add +thumb-mode in clang.

Yes, that would need to come at the same time as removing the code in this pass that adds +thumb-mode as it looks like that's the only thing that's fixing the potentially incorrect features currently coming from the frontend.

llvm/lib/IR/Function.cpp

topperc · 2025-02-04T22:24:14Z

llvm/lib/IR/Function.cpp

+  } else if (T.isRISCV32() && TargetABI.contains("ilp32f")) {
+    attr = "+f";
+  } else if (T.isARM() || T.isThumb()) {
+    attr = "+thumb-mode";


For RISC-V can we add +f/+d to the features inside the RISCVSubtarget constructor based on the ABI?

llvm/lib/IR/Function.cpp

…tFeatures It is currently not possible to provide any reasonable target-features for compiler generated functions (See: llvm#69780) Having a target-abi will provide a way to add minimal requirements for target-features like `+d` for RISC-V.

ilovepi · 2025-02-20T16:03:28Z

llvm/lib/IR/Function.cpp

+      Attr = "+d";
+    else if (TargetABI.equals_insensitive("ilp32f"))
+      Attr = "+f";
+  } else if (T.isARM() || T.isThumb()) {


Can we also get a test for the ARM cases?

hiraditya · 2025-02-27T22:23:34Z

Abandoning this as i wont have time to work on it in near future. Anyone feel free to take it up.

llvmbot added the LTO Link time optimization (regular/full LTO or ThinLTO) label Jul 26, 2024

hiraditya requested review from echristo, pcc and jyknight July 26, 2024 23:30

hiraditya force-pushed the pass_module_to_subtarget branch from 6052288 to d2619c2 Compare July 27, 2024 05:35

hiraditya changed the title ~~[Draft] Use Module to get Triple in getDefaultSubtargetFeatures~~ [Draft] Use TargetABI to assign default-target features in getDefaultSubtargetFeatures Jul 30, 2024

hiraditya force-pushed the pass_module_to_subtarget branch from d2619c2 to 087232c Compare July 31, 2024 17:56

hiraditya changed the title ~~[Draft] Use TargetABI to assign default-target features in getDefaultSubtargetFeatures~~ Use TargetABI to assign default-target features in getDefaultSubtargetFeatures Jul 31, 2024

hiraditya force-pushed the pass_module_to_subtarget branch from 087232c to 04a4523 Compare July 31, 2024 18:04

hiraditya requested review from nikic and removed request for echristo and pcc August 2, 2024 18:11

echristo self-requested a review August 5, 2024 16:51

hiraditya force-pushed the pass_module_to_subtarget branch 2 times, most recently from a882c96 to fb822ab Compare September 4, 2024 16:14

ilovepi reviewed Sep 5, 2024

View reviewed changes

llvm/lib/LTO/LTOBackend.cpp Outdated Show resolved Hide resolved

llvm/lib/TargetParser/SubtargetFeature.cpp Outdated Show resolved Hide resolved

topperc reviewed Sep 5, 2024

View reviewed changes

llvm/lib/LTO/LTOBackend.cpp Outdated Show resolved Hide resolved

llvm/lib/TargetParser/SubtargetFeature.cpp Outdated Show resolved Hide resolved

hiraditya force-pushed the pass_module_to_subtarget branch 2 times, most recently from 3e4125b to f9307a0 Compare September 5, 2024 05:31

llvmbot added the llvm:transforms label Sep 5, 2024

hiraditya force-pushed the pass_module_to_subtarget branch 4 times, most recently from d98b939 to 45e232d Compare September 5, 2024 17:51

ilovepi approved these changes Sep 6, 2024

View reviewed changes

hiraditya force-pushed the pass_module_to_subtarget branch from 45e232d to 09c2dde Compare September 6, 2024 21:04

topperc approved these changes Sep 6, 2024

View reviewed changes

nikic requested a review from efriedma-quic September 11, 2024 09:26

ilovepi reviewed Sep 25, 2024

View reviewed changes

llvm/lib/TargetParser/SubtargetFeature.cpp Outdated Show resolved Hide resolved

hiraditya mentioned this pull request Sep 26, 2024

LTO plugin uses wrong ABI for LTO objects on riscv #50591

Open

hiraditya force-pushed the pass_module_to_subtarget branch 2 times, most recently from be73339 to 4ac0899 Compare October 11, 2024 23:38

llvmbot added the backend:RISC-V label Oct 11, 2024

hiraditya force-pushed the pass_module_to_subtarget branch from 4ac0899 to 36556aa Compare October 11, 2024 23:49

hiraditya force-pushed the pass_module_to_subtarget branch 2 times, most recently from e11196d to 7cb0960 Compare December 18, 2024 01:26

llvmbot added the llvm:ir label Dec 18, 2024

hiraditya changed the title ~~Use TargetABI to assign default-target features in getDefaultSubtargetFeatures~~ Use Module level target-abi to assign target features for codegenerated functions. Dec 18, 2024

hiraditya force-pushed the pass_module_to_subtarget branch 3 times, most recently from 2430ee2 to 999b9b9 Compare December 18, 2024 01:58

lenary reviewed Dec 18, 2024

View reviewed changes

llvm/lib/IR/Function.cpp Outdated Show resolved Hide resolved

hiraditya force-pushed the pass_module_to_subtarget branch from 999b9b9 to 9c1029e Compare January 29, 2025 21:42

arichardson requested a review from jrtc27 January 30, 2025 17:53

arichardson requested changes Jan 30, 2025

View reviewed changes

topperc reviewed Feb 4, 2025

View reviewed changes

llvm/lib/IR/Function.cpp Outdated Show resolved Hide resolved

llvm/lib/IR/Function.cpp Outdated Show resolved Hide resolved

hiraditya force-pushed the pass_module_to_subtarget branch from 9c1029e to 0950354 Compare February 5, 2025 20:07

Address comments

bb7b821

hiraditya force-pushed the pass_module_to_subtarget branch from 0950354 to bb7b821 Compare February 6, 2025 21:27

kito-cheng mentioned this pull request Feb 12, 2025

[LTO] Override TargetABI from module flags if present when creating TargetMachine #126497

Merged

ilovepi reviewed Feb 20, 2025

View reviewed changes

hiraditya closed this Feb 27, 2025

pcc mentioned this pull request Mar 17, 2025

[CrossDSO CFI] Make sure __cfi_check has BTI attribute. #131224

Open

Use Module level target-abi to assign target features for codegenerated functions. #100833

Use Module level target-abi to assign target features for codegenerated functions. #100833

Uh oh!

Conversation

hiraditya commented Jul 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jul 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jul 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

echristo commented Aug 5, 2024

Uh oh!

ilovepi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ilovepi left a comment

Choose a reason for hiding this comment

Uh oh!

topperc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

topperc commented Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

arichardson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hiraditya Feb 4, 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!

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hiraditya commented Feb 27, 2025

Uh oh!

Uh oh!

hiraditya commented Jul 26, 2024 •

edited

Loading

llvmbot commented Jul 26, 2024 •

edited

Loading

github-actions bot commented Jul 26, 2024 •

edited

Loading

topperc commented Sep 25, 2024 •

edited

Loading

hiraditya Feb 4, 2025 •

edited

Loading