[Flang][Lower] Attach target_cpu and target_features attributes to MLIR functions #78289
Conversation
skatrak
requested review from
kiranchandramohan,
Leporacanthicus,
MacDue and
kparzysz
llvmbot
added
flang:driver
flang
|
@llvm/pr-subscribers-flang-codegen @llvm/pr-subscribers-flang-fir-hlfir Author: Sergio Afonso (skatrak) ChangesThis patch forwards the target CPU and features information from the Flang frontend to MLIR func.func operation attributes, which are later used to populate the target_cpu and target_features llvm.func attributes. This completes a full flow by which target CPU and features make it all the way from compiler options to LLVM IR function attributes. Full diff: https://github.com/llvm/llvm-project/pull/78289.diff 7 Files Affected:
diff --git a/flang/include/flang/Lower/Bridge.h b/flang/include/flang/Lower/Bridge.h
index 6c0d14d65edae1..4864a08d9977b8 100644
--- a/flang/include/flang/Lower/Bridge.h
+++ b/flang/include/flang/Lower/Bridge.h
@@ -21,10 +21,7 @@
#include "flang/Optimizer/Builder/FIRBuilder.h"
#include "flang/Optimizer/Dialect/Support/KindMapping.h"
#include "mlir/IR/BuiltinOps.h"
-
-namespace llvm {
-class DataLayout;
-} // namespace llvm
+#include "llvm/Target/TargetMachine.h"
namespace Fortran {
namespace common {
@@ -64,11 +61,11 @@ class LoweringBridge {
const Fortran::lower::LoweringOptions &loweringOptions,
const std::vector<Fortran::lower::EnvironmentDefault> &envDefaults,
const Fortran::common::LanguageFeatureControl &languageFeatures,
- const llvm::DataLayout *dataLayout = nullptr) {
+ const llvm::TargetMachine &targetMachine) {
return LoweringBridge(ctx, semanticsContext, defaultKinds, intrinsics,
targetCharacteristics, allCooked, triple, kindMap,
loweringOptions, envDefaults, languageFeatures,
- dataLayout);
+ targetMachine);
}
//===--------------------------------------------------------------------===//
@@ -110,6 +107,8 @@ class LoweringBridge {
return languageFeatures;
}
+ const llvm::TargetMachine &getTargetMachine() const { return targetMachine; }
+
/// Create a folding context. Careful: this is very expensive.
Fortran::evaluate::FoldingContext createFoldingContext() const;
@@ -147,7 +146,7 @@ class LoweringBridge {
const Fortran::lower::LoweringOptions &loweringOptions,
const std::vector<Fortran::lower::EnvironmentDefault> &envDefaults,
const Fortran::common::LanguageFeatureControl &languageFeatures,
- const llvm::DataLayout *dataLayout);
+ const llvm::TargetMachine &targetMachine);
LoweringBridge() = delete;
LoweringBridge(const LoweringBridge &) = delete;
@@ -164,6 +163,7 @@ class LoweringBridge {
const Fortran::lower::LoweringOptions &loweringOptions;
const std::vector<Fortran::lower::EnvironmentDefault> &envDefaults;
const Fortran::common::LanguageFeatureControl &languageFeatures;
+ const llvm::TargetMachine &targetMachine;
};
} // namespace lower
diff --git a/flang/lib/Frontend/FrontendActions.cpp b/flang/lib/Frontend/FrontendActions.cpp
index 74e3992d5ab62b..397e403847588e 100644
--- a/flang/lib/Frontend/FrontendActions.cpp
+++ b/flang/lib/Frontend/FrontendActions.cpp
@@ -285,8 +285,6 @@ bool CodeGenAction::beginSourceFileAction() {
ci.getSemanticsContext().defaultKinds();
fir::KindMapping kindMap(mlirCtx.get(), llvm::ArrayRef<fir::KindTy>{
fir::fromDefaultKinds(defKinds)});
- const llvm::DataLayout &dl = targetMachine.createDataLayout();
-
lower::LoweringBridge lb = Fortran::lower::LoweringBridge::create(
*mlirCtx, ci.getSemanticsContext(), defKinds,
ci.getSemanticsContext().intrinsics(),
@@ -294,7 +292,7 @@ bool CodeGenAction::beginSourceFileAction() {
ci.getParsing().allCooked(), ci.getInvocation().getTargetOpts().triple,
kindMap, ci.getInvocation().getLoweringOpts(),
ci.getInvocation().getFrontendOpts().envDefaults,
- ci.getInvocation().getFrontendOpts().features, &dl);
+ ci.getInvocation().getFrontendOpts().features, targetMachine);
// Fetch module from lb, so we can set
mlirModule = std::make_unique<mlir::ModuleOp>(lb.getModule());
diff --git a/flang/lib/Lower/Bridge.cpp b/flang/lib/Lower/Bridge.cpp
index 8006b9b426f4dc..27e6e46c52d825 100644
--- a/flang/lib/Lower/Bridge.cpp
+++ b/flang/lib/Lower/Bridge.cpp
@@ -54,6 +54,7 @@
#include "flang/Semantics/symbol.h"
#include "flang/Semantics/tools.h"
#include "mlir/Dialect/ControlFlow/IR/ControlFlowOps.h"
+#include "mlir/Dialect/LLVMIR/LLVMAttrs.h"
#include "mlir/IR/PatternMatch.h"
#include "mlir/Parser/Parser.h"
#include "mlir/Transforms/RegionUtils.h"
@@ -4290,6 +4291,18 @@ class FirConverter : public Fortran::lower::AbstractConverter {
assert(blockId == 0 && "invalid blockId");
assert(activeConstructStack.empty() && "invalid construct stack state");
+ // Set target_cpu and target_features attributes to be passed through to the
+ // llvm.func operation during lowering.
+ const llvm::TargetMachine &targetMachine = bridge.getTargetMachine();
+ if (auto targetCPU = targetMachine.getTargetCPU(); !targetCPU.empty())
+ func->setAttr("target_cpu",
+ mlir::StringAttr::get(func.getContext(), targetCPU));
+
+ if (auto targetFeatures = targetMachine.getTargetFeatureString();
+ !targetFeatures.empty())
+ func->setAttr("target_features", mlir::LLVM::TargetFeaturesAttr::get(
+ func.getContext(), targetFeatures));
+
// Manage floating point exception, halting mode, and rounding mode
// settings at function entry and exit.
if (!funit.isMainProgram())
@@ -5062,12 +5075,12 @@ Fortran::lower::LoweringBridge::LoweringBridge(
const Fortran::lower::LoweringOptions &loweringOptions,
const std::vector<Fortran::lower::EnvironmentDefault> &envDefaults,
const Fortran::common::LanguageFeatureControl &languageFeatures,
- const llvm::DataLayout *dataLayout)
+ const llvm::TargetMachine &targetMachine)
: semanticsContext{semanticsContext}, defaultKinds{defaultKinds},
intrinsics{intrinsics}, targetCharacteristics{targetCharacteristics},
cooked{&cooked}, context{context}, kindMap{kindMap},
loweringOptions{loweringOptions}, envDefaults{envDefaults},
- languageFeatures{languageFeatures} {
+ languageFeatures{languageFeatures}, targetMachine{targetMachine} {
// Register the diagnostic handler.
context.getDiagEngine().registerHandler([](mlir::Diagnostic &diag) {
llvm::raw_ostream &os = llvm::errs();
@@ -5118,6 +5131,6 @@ Fortran::lower::LoweringBridge::LoweringBridge(
assert(module.get() && "module was not created");
fir::setTargetTriple(*module.get(), triple);
fir::setKindMapping(*module.get(), kindMap);
- if (dataLayout)
- fir::support::setMLIRDataLayout(*module.get(), *dataLayout);
+ fir::support::setMLIRDataLayout(*module.get(),
+ targetMachine.createDataLayout());
}
diff --git a/flang/test/Driver/save-mlir-temps.f90 b/flang/test/Driver/save-mlir-temps.f90
index 1c8935fbd7aac9..e9478a6c521b2e 100644
--- a/flang/test/Driver/save-mlir-temps.f90
+++ b/flang/test/Driver/save-mlir-temps.f90
@@ -51,9 +51,9 @@
! Content to check from the MLIR outputs
!--------------------------
! MLIR-FIR-NOT: llvm.func
-! MLIR-FIR: func.func @{{.*}}main(){{.*}}{
+! MLIR-FIR: func.func @{{.*}}main(){{.*}}
-! MLIR-FIR-NOT: func.func
-! MLIR-LLVMIR: llvm.func @{{.*}}main(){{.*}}{
+! MLIR-LLVMIR-NOT: func.func
+! MLIR-LLVMIR: llvm.func @{{.*}}main(){{.*}}
end program
diff --git a/flang/test/Lower/target-features-amdgcn.f90 b/flang/test/Lower/target-features-amdgcn.f90
new file mode 100644
index 00000000000000..8bd6773a43d4e3
--- /dev/null
+++ b/flang/test/Lower/target-features-amdgcn.f90
@@ -0,0 +1,23 @@
+! REQUIRES: amdgpu-registered-target
+! RUN: %flang_fc1 -emit-fir %s -o - | FileCheck %s --check-prefixes=ALL,NONE
+! RUN: %flang_fc1 -emit-fir -triple amdgcn-amd-amdhsa %s -o - | FileCheck %s --check-prefixes=ALL,TRIPLE
+! RUN: %flang_fc1 -emit-fir -target-cpu gfx90a %s -o - | FileCheck %s --check-prefixes=ALL,CPU
+! RUN: %flang_fc1 -emit-fir -triple amdgcn-amd-amdhsa -target-cpu gfx90a %s -o - | FileCheck %s --check-prefixes=ALL,BOTH
+
+! ALL-LABEL: func.func @_QPfoo()
+
+! NONE-NOT: target_cpu
+! NONE-NOT: target_features
+
+! TRIPLE-SAME: target_cpu = "generic-hsa"
+! TRIPLE-NOT: target_features
+
+! CPU-SAME: target_cpu = "gfx90a"
+! CPU-NOT: target_features
+
+! BOTH-SAME: target_cpu = "gfx90a"
+! BOTH-SAME: target_features = #llvm.target_features<[
+! BOTH-SAME: "+gfx90a-insts"
+! BOTH-SAME: ]>
+subroutine foo
+end subroutine
diff --git a/flang/test/Lower/target-features-x86_64.f90 b/flang/test/Lower/target-features-x86_64.f90
new file mode 100644
index 00000000000000..c4190e83f6a6c7
--- /dev/null
+++ b/flang/test/Lower/target-features-x86_64.f90
@@ -0,0 +1,21 @@
+! REQUIRES: x86-registered-target
+! RUN: %flang_fc1 -emit-fir -triple x86_64-unknown-linux-gnu %s -o - | FileCheck %s --check-prefixes=ALL,NONE
+! RUN: %flang_fc1 -emit-fir -triple x86_64-unknown-linux-gnu -target-cpu x86-64 %s -o - | FileCheck %s --check-prefixes=ALL,CPU
+! RUN: %flang_fc1 -emit-fir -triple x86_64-unknown-linux-gnu -target-feature +sse %s -o - | FileCheck %s --check-prefixes=ALL,FEATURE
+! RUN: %flang_fc1 -emit-fir -triple x86_64-unknown-linux-gnu -target-cpu x86-64 -target-feature +sse %s -o - | FileCheck %s --check-prefixes=ALL,BOTH
+
+! ALL-LABEL: func.func @_QPfoo()
+
+! NONE-NOT: target_cpu
+! NONE-NOT: target_features
+
+! CPU-SAME: target_cpu = "x86-64"
+! CPU-NOT: target_features
+
+! FEATURE-NOT: target_cpu
+! FEATURE-SAME: target_features = #llvm.target_features<["+sse"]>
+
+! BOTH-SAME: target_cpu = "x86-64"
+! BOTH-SAME: target_features = #llvm.target_features<["+sse"]>
+subroutine foo
+end subroutine
diff --git a/flang/tools/bbc/bbc.cpp b/flang/tools/bbc/bbc.cpp
index b4ba837a3263f8..98d9258e023e55 100644
--- a/flang/tools/bbc/bbc.cpp
+++ b/flang/tools/bbc/bbc.cpp
@@ -331,7 +331,6 @@ static mlir::LogicalResult convertFortranSourceToMLIR(
auto &defKinds = semanticsContext.defaultKinds();
fir::KindMapping kindMap(
&ctx, llvm::ArrayRef<fir::KindTy>{fir::fromDefaultKinds(defKinds)});
- const llvm::DataLayout &dataLayout = targetMachine.createDataLayout();
std::string targetTriple = targetMachine.getTargetTriple().normalize();
// Use default lowering options for bbc.
Fortran::lower::LoweringOptions loweringOptions{};
@@ -342,7 +341,7 @@ static mlir::LogicalResult convertFortranSourceToMLIR(
ctx, semanticsContext, defKinds, semanticsContext.intrinsics(),
semanticsContext.targetCharacteristics(), parsing.allCooked(),
targetTriple, kindMap, loweringOptions, {},
- semanticsContext.languageFeatures(), &dataLayout);
+ semanticsContext.languageFeatures(), targetMachine);
burnside.lower(parseTree, semanticsContext);
mlir::ModuleOp mlirModule = burnside.getModule();
if (enableOpenMP) {
|
flang/lib/Lower/Bridge.cpp
Outdated
| // Set target_cpu and target_features attributes to be passed through to the | ||
| // llvm.func operation during lowering. | ||
| const llvm::TargetMachine &targetMachine = bridge.getTargetMachine(); | ||
| if (auto targetCPU = targetMachine.getTargetCPU(); !targetCPU.empty()) | ||
| func->setAttr("target_cpu", | ||
| mlir::StringAttr::get(func.getContext(), targetCPU)); | ||
|
|
||
| if (auto targetFeatures = targetMachine.getTargetFeatureString(); | ||
| !targetFeatures.empty()) | ||
| func->setAttr("target_features", mlir::LLVM::TargetFeaturesAttr::get( | ||
| func.getContext(), targetFeatures)); |
There was a problem hiding this comment.
The default question here would be whether we are going to use this information in HLFIR/FIR transformations and if not whether it is better to delay adding this information closer to conversion to LLVM dialect or potentially later.
If it is closer to conversion to LLVM dialect then you might be able to reuse the pass that added frame-pointer attribute. #74598
Please wait for opinion from others as well.
There was a problem hiding this comment.
In FrontendAction we already have the TargetMachine, and we use a part of it in the bridge, so this change does not introduce any new dependencies. Not that it would be wrong to do so, but it shows that we already use a part of the information from the target machine. I think it is logical to take it further, and include all relevant information from TargetMachine into the lowered MLIR.
Edit: added "relevant".
There was a problem hiding this comment.
@kiranchandramohan at this time, these function attributes would be added with the only purpose of being forwarded to the LLVM dialect and from there being translated into LLVM IR function attributes, so as you say it would be possible to delay their introduction to a later pass rather than doing it directly in lowering.
However, in my opinion, this is general information in the same category as the target triple, kind mapping or data layout that could be at some point needed earlier in the compilation flow, just like these other attributes are. I agree with @kparzysz that having this information available in the lowering bridge doesn't really introduce new dependencies, so I don't currently see any clear downsides. The benefit of this approach is that this information is available as soon as possible and we don't introduce intermediate MLIR representations where this information that is known in advance can't be accessed.
So, I think at the moment this is still my preferred approach, though I'm happy to change it if there are any other aspects I've missed or if the alternative seems to be generally preferred by the community.
There was a problem hiding this comment.
I am OK with it being here.
But as you can imagine there could be other opinions and also the fact that some function attributes are added in lowering, some in passes and what is the right place to do this for a future function attribute.
There was a problem hiding this comment.
I think this doesn't set the target info for function declarations (interfaces that are in a different source), which I need. I'm happy to add that as an addition on top. l
Are we getting the target CPU and target features for every function? Or just the once? I'm sure it's not a "heavy" operaiton, but seems a bit unnecessary to do repeatedly. Should be the same for ALL functions in a module...
There was a problem hiding this comment.
I am in favor of delaying this to something like codegen (or better, the target rewrite pass) based on Kiran point that functions may be generated during FIR to FIR passes and that this is a lot of noise.
Some target specific aspect are unavoidable in high level representation because it is needed for Fortran semantics (e.g., the data layout), but it does not mean we should start doing/adding a lot of target specific aspects in the IR before it is actually needed.
I would also prefer passing the target_cpu/target_features as strings to the target rewrite pass, the TargetMachine is not an easy object to come by, and makes testing dependent on the available targets in the LLVM build.
There was a problem hiding this comment.
Thank you all for sharing your suggestions and concerns, I can now understand better the problems with this approach so I'll work on following @jeanPerier's suggested alternative and move this logic from the lowering bridge to the target rewrite pass. This should also address the annotation of function declarations raised by @Leporacanthicus, which I tested to not be covered by this patch in its current form.
There was a problem hiding this comment.
The reason I suggested adding all relevant information from target machine early on is to become independent of it, or in other words, for the generated module to be more self-contained. We already get the target machine at this point, so we may extract all that we're going to need from it, until having it again becomes necessary (generation of LLVM IR?).
If the consensus is that this is not the right thing to do, then I'm ok with that.
Edit: reword
There was a problem hiding this comment.
We already get the target machine at this point, so we may extract all that we're going to need from it, until having it again becomes necessary (generation of LLVM IR?).
The issue is that this implies that every func.func created after the bridge/not in this very spot would not have these attributes while they should. So this would push the requirements of such target attribute setting to every spot creating fun.func. We could use a centralized helper, but you would still need to thread the info to the place calling it, and you cannot easily enforce that it is being used (especially if some FIR external passes are used).
So, much safer/consistent to do it in a pass IMHO.
I understand your point about self-contained module, and I am not opposed adding these attributes to the module op until we are ready to add them on the func.func (that is, before codegen when we should not emit new func.func), and to retrieve them from here instead of the driver.
There was a problem hiding this comment.
I understand your point about self-contained module, and I am not opposed adding these attributes to the module op until we are ready to add them on the func.func (that is, before codegen when we should not emit new func.func), and to retrieve them from here instead of the driver.
I think this is a good compromise. Since these attributes will have the same values for all functions, we can attach this information to the module, making it available as early as possible, but without having to deal with explicitly adding them to each function that is created. Then, the target rewrite pass only has to forward these module attributes to each function.
skatrak
force-pushed
the
users/skatrak/spr/target-attrs-llvm-dialect
branch
from
ae33bcc to
03c87c3
Compare
flang/lib/Lower/Bridge.cpp
Outdated
| // Set target_cpu and target_features attributes to be passed through to the | ||
| // llvm.func operation during lowering. | ||
| const llvm::TargetMachine &targetMachine = bridge.getTargetMachine(); | ||
| if (auto targetCPU = targetMachine.getTargetCPU(); !targetCPU.empty()) | ||
| func->setAttr("target_cpu", | ||
| mlir::StringAttr::get(func.getContext(), targetCPU)); | ||
|
|
||
| if (auto targetFeatures = targetMachine.getTargetFeatureString(); | ||
| !targetFeatures.empty()) | ||
| func->setAttr("target_features", mlir::LLVM::TargetFeaturesAttr::get( | ||
| func.getContext(), targetFeatures)); |
There was a problem hiding this comment.
I am OK with it being here.
But as you can imagine there could be other opinions and also the fact that some function attributes are added in lowering, some in passes and what is the right place to do this for a future function attribute.
skatrak
force-pushed
the
users/skatrak/spr/target-attrs-llvm-dialect
branch
from
03c87c3 to
ccde742
Compare
skatrak
force-pushed
the
users/skatrak/spr/target-attrs-flang
branch
from
a1701f9 to
ca41e03
Compare
|
I updated the patch trying to incorporate all review comments so far. Let me know if this addresses your concerns or if I should make any further changes before merging it. Thank you! |
|
@jeanPerier, @kiranchandramohan if you have some spare time this week, could you check to see whether these changes have addressed your concerns? |
|
@Leporacanthicus will look into this today. Looks OK to me. |
|
Thanks for looking at this and for the feedback, I'll rebase and merge it after running tests locally. |
…IR functions
This patch forwards the target CPU and features information from the Flang
frontend to MLIR func.func operation attributes, which are later used to
populate the target_cpu and target_features llvm.func attributes.
This is achieved in two stages:
1. Introduce the `fir.target_cpu` and `fir.target_features` module
attributes with information from the target machine immediately after the
initial creation of the MLIR module in the lowering bridge.
2. Update the target rewrite flang pass to get this information from the
module and pass it along to all func.func MLIR operations, respectively as
attributes named `target_cpu` and `target_features`. These attributes will
be automatically picked up during Func to LLVM dialect lowering and used to
initialize the corresponding llvm.func named attributes.
The target rewrite and FIR to LLVM lowering passes are updated with the ability
to override these module attributes, and the `CodeGenSpecifics` optimizer class
is augmented to make this information available to target-specific MLIR
transformations.
This completes a full flow by which target CPU and features make it all the way
from compiler options to LLVM IR function attributes.
skatrak
force-pushed
the
users/skatrak/spr/target-attrs-flang
branch
from
ca41e03 to
4246063
Compare
This patch forwards the target CPU and features information from the Flang frontend to MLIR func.func operation attributes, which are later used to populate the target_cpu and target_features llvm.func attributes.
This is achieved in two stages:
Introduce the
fir.target_cpuandfir.target_featuresmodule attributes with information from the target machine immediately after the initial creation of the MLIR module in the lowering bridge.Update the target rewrite flang pass to get this information from the module and pass it along to all func.func MLIR operations, respectively as attributes named
target_cpuandtarget_features. These attributes will be automatically picked up during Func to LLVM dialect lowering and used to initialize the corresponding llvm.func named attributes.The target rewrite and FIR to LLVM lowering passes are updated with the ability to override these module attributes, and the
CodeGenSpecificsoptimizer class is augmented to make this information available to target-specific MLIR transformations.This completes a full flow by which target CPU and features make it all the way from compiler options to LLVM IR function attributes.