[NFC][clang][FMV][TargetInfo] Refactor API for FMV feature priority. #116257
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently we have code with target hooks in CodeGenModule shared between X86 and AArch64 for sorting MultiVersionResolverOptions. Those are used when generating IFunc resolvers for FMV. The RISCV target has different criteria for sorting, therefore it repeats sorting after calling CodeGenFunction::EmitMultiVersionResolver. I am moving the FMV priority logic in TargetInfo, so that it can be implemented by the TargetParser which then makes it possible to query it from llvm. Here is an example why this is handy: llvm#87939
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-clang Author: Alexandros Lamprineas (labrinea) ChangesCurrently we have code with target hooks in CodeGenModule shared between X86 and AArch64 for sorting MultiVersionResolverOptions. Those are used when generating IFunc resolvers for FMV. The RISCV target has different criteria for sorting, therefore it repeats sorting after calling CodeGenFunction::EmitMultiVersionResolver. I am moving the FMV priority logic in TargetInfo, so that it can be implemented by the TargetParser which then makes it possible to query it from llvm. Here is an example why this is handy: #87939 Full diff: https://github.com/llvm/llvm-project/pull/116257.diff 12 Files Affected:
diff --git a/clang/include/clang/Basic/TargetInfo.h b/clang/include/clang/Basic/TargetInfo.h
index 25eda907d20a7b..c27f7eb591f43d 100644
--- a/clang/include/clang/Basic/TargetInfo.h
+++ b/clang/include/clang/Basic/TargetInfo.h
@@ -1514,14 +1514,10 @@ class TargetInfo : public TransferrableTargetInfo,
// Return the target-specific priority for features/cpus/vendors so
// that they can be properly sorted for checking.
- virtual unsigned multiVersionSortPriority(StringRef Name) const {
+ virtual unsigned getFMVPriority(ArrayRef<StringRef> Features) const {
return 0;
}
- // Return the target-specific cost for feature
- // that taken into account in priority sorting.
- virtual unsigned multiVersionFeatureCost() const { return 0; }
-
// Validate the contents of the __builtin_cpu_is(const char*)
// argument.
virtual bool validateCpuIs(StringRef Name) const { return false; }
diff --git a/clang/lib/Basic/Targets/AArch64.cpp b/clang/lib/Basic/Targets/AArch64.cpp
index eb8a3ada034482..4efc1841c836d0 100644
--- a/clang/lib/Basic/Targets/AArch64.cpp
+++ b/clang/lib/Basic/Targets/AArch64.cpp
@@ -705,18 +705,8 @@ AArch64TargetInfo::getVScaleRange(const LangOptions &LangOpts) const {
return std::nullopt;
}
-unsigned AArch64TargetInfo::multiVersionSortPriority(StringRef Name) const {
- if (Name == "default")
- return 0;
- if (auto Ext = llvm::AArch64::parseFMVExtension(Name))
- return Ext->Priority;
- return 0;
-}
-
-unsigned AArch64TargetInfo::multiVersionFeatureCost() const {
- // Take the maximum priority as per feature cost, so more features win.
- constexpr unsigned MaxFMVPriority = 1000;
- return MaxFMVPriority;
+unsigned AArch64TargetInfo::getFMVPriority(ArrayRef<StringRef> Features) const {
+ return llvm::AArch64::getFMVPriority(Features);
}
bool AArch64TargetInfo::doesFeatureAffectCodeGen(StringRef Name) const {
diff --git a/clang/lib/Basic/Targets/AArch64.h b/clang/lib/Basic/Targets/AArch64.h
index 4c25bdb5bb16df..68a8b1ebad8cde 100644
--- a/clang/lib/Basic/Targets/AArch64.h
+++ b/clang/lib/Basic/Targets/AArch64.h
@@ -137,8 +137,7 @@ class LLVM_LIBRARY_VISIBILITY AArch64TargetInfo : public TargetInfo {
void fillValidCPUList(SmallVectorImpl<StringRef> &Values) const override;
bool setCPU(const std::string &Name) override;
- unsigned multiVersionSortPriority(StringRef Name) const override;
- unsigned multiVersionFeatureCost() const override;
+ unsigned getFMVPriority(ArrayRef<StringRef> Features) const override;
bool useFP16ConversionIntrinsics() const override {
return false;
diff --git a/clang/lib/Basic/Targets/RISCV.cpp b/clang/lib/Basic/Targets/RISCV.cpp
index eaaba7642bd7b2..d5312bb171af6c 100644
--- a/clang/lib/Basic/Targets/RISCV.cpp
+++ b/clang/lib/Basic/Targets/RISCV.cpp
@@ -476,6 +476,21 @@ ParsedTargetAttr RISCVTargetInfo::parseTargetAttr(StringRef Features) const {
return Ret;
}
+unsigned RISCVTargetInfo::getFMVPriority(ArrayRef<StringRef> Features) const {
+ SmallVector<StringRef, 8> Attrs;
+ Features[0].split(Attrs, ';');
+ // Default Priority is zero.
+ unsigned Priority = 0;
+ for (auto Attr : Attrs) {
+ if (Attr.consume_front("priority=")) {
+ unsigned Result;
+ if (!Attr.getAsInteger(0, Result))
+ Priority = Result;
+ }
+ }
+ return Priority;
+}
+
TargetInfo::CallingConvCheckResult
RISCVTargetInfo::checkCallingConvention(CallingConv CC) const {
switch (CC) {
diff --git a/clang/lib/Basic/Targets/RISCV.h b/clang/lib/Basic/Targets/RISCV.h
index 3b418585ab4a39..0a6caeb54bbd5d 100644
--- a/clang/lib/Basic/Targets/RISCV.h
+++ b/clang/lib/Basic/Targets/RISCV.h
@@ -122,6 +122,7 @@ class RISCVTargetInfo : public TargetInfo {
void fillValidTuneCPUList(SmallVectorImpl<StringRef> &Values) const override;
bool supportsTargetAttributeTune() const override { return true; }
ParsedTargetAttr parseTargetAttr(StringRef Str) const override;
+ unsigned getFMVPriority(ArrayRef<StringRef> Features) const override;
std::pair<unsigned, unsigned> hardwareInterferenceSizes() const override {
return std::make_pair(32, 32);
diff --git a/clang/lib/Basic/Targets/X86.cpp b/clang/lib/Basic/Targets/X86.cpp
index 086b4415412e67..ce5ccd0447b437 100644
--- a/clang/lib/Basic/Targets/X86.cpp
+++ b/clang/lib/Basic/Targets/X86.cpp
@@ -1363,19 +1363,28 @@ static llvm::X86::ProcessorFeatures getFeature(StringRef Name) {
// correct, so it asserts if the value is out of range.
}
-unsigned X86TargetInfo::multiVersionSortPriority(StringRef Name) const {
- // Valid CPUs have a 'key feature' that compares just better than its key
- // feature.
- using namespace llvm::X86;
- CPUKind Kind = parseArchX86(Name);
- if (Kind != CK_None) {
- ProcessorFeatures KeyFeature = getKeyFeature(Kind);
- return (getFeaturePriority(KeyFeature) << 1) + 1;
- }
-
- // Now we know we have a feature, so get its priority and shift it a few so
- // that we have sufficient room for the CPUs (above).
- return getFeaturePriority(getFeature(Name)) << 1;
+unsigned X86TargetInfo::getFMVPriority(ArrayRef<StringRef> Features) const {
+ auto getPriority = [this](StringRef Feature) -> unsigned {
+ if (Feature.empty())
+ return 0;
+
+ // Valid CPUs have a 'key feature' that compares just better than its key
+ // feature.
+ using namespace llvm::X86;
+ CPUKind Kind = parseArchX86(Feature);
+ if (Kind != CK_None) {
+ ProcessorFeatures KeyFeature = getKeyFeature(Kind);
+ return (getFeaturePriority(KeyFeature) << 1) + 1;
+ }
+ // Now we know we have a feature, so get its priority and shift it a few so
+ // that we have sufficient room for the CPUs (above).
+ return getFeaturePriority(getFeature(Feature)) << 1;
+ };
+
+ unsigned Priority = 0;
+ for (StringRef Feature : Features)
+ Priority = std::max(Priority, getPriority(Feature));
+ return Priority;
}
bool X86TargetInfo::validateCPUSpecificCPUDispatch(StringRef Name) const {
diff --git a/clang/lib/Basic/Targets/X86.h b/clang/lib/Basic/Targets/X86.h
index 06a7eed8177cb2..3ed36c8fa724b5 100644
--- a/clang/lib/Basic/Targets/X86.h
+++ b/clang/lib/Basic/Targets/X86.h
@@ -384,7 +384,7 @@ class LLVM_LIBRARY_VISIBILITY X86TargetInfo : public TargetInfo {
return CPU != llvm::X86::CK_None;
}
- unsigned multiVersionSortPriority(StringRef Name) const override;
+ unsigned getFMVPriority(ArrayRef<StringRef> Features) const override;
bool setFPMath(StringRef Name) override;
diff --git a/clang/lib/CodeGen/ABIInfo.cpp b/clang/lib/CodeGen/ABIInfo.cpp
index edd7146dc1ac76..8e76cf15b642c6 100644
--- a/clang/lib/CodeGen/ABIInfo.cpp
+++ b/clang/lib/CodeGen/ABIInfo.cpp
@@ -218,8 +218,8 @@ void ABIInfo::appendAttributeMangling(StringRef AttrStr,
// only have "+" prefixes here.
assert(LHS.starts_with("+") && RHS.starts_with("+") &&
"Features should always have a prefix.");
- return TI.multiVersionSortPriority(LHS.substr(1)) >
- TI.multiVersionSortPriority(RHS.substr(1));
+ return TI.getFMVPriority({LHS.substr(1)}) >
+ TI.getFMVPriority({RHS.substr(1)});
});
bool IsFirst = true;
diff --git a/clang/lib/CodeGen/CodeGenFunction.cpp b/clang/lib/CodeGen/CodeGenFunction.cpp
index 6ead45793742d6..7ac60e8dc0508e 100644
--- a/clang/lib/CodeGen/CodeGenFunction.cpp
+++ b/clang/lib/CodeGen/CodeGenFunction.cpp
@@ -2904,24 +2904,6 @@ void CodeGenFunction::EmitMultiVersionResolver(
}
}
-static unsigned getPriorityFromAttrString(StringRef AttrStr) {
- SmallVector<StringRef, 8> Attrs;
-
- AttrStr.split(Attrs, ';');
-
- // Default Priority is zero.
- unsigned Priority = 0;
- for (auto Attr : Attrs) {
- if (Attr.consume_front("priority=")) {
- unsigned Result;
- if (!Attr.getAsInteger(0, Result))
- Priority = Result;
- }
- }
-
- return Priority;
-}
-
void CodeGenFunction::EmitRISCVMultiVersionResolver(
llvm::Function *Resolver, ArrayRef<MultiVersionResolverOption> Options) {
@@ -2939,20 +2921,10 @@ void CodeGenFunction::EmitRISCVMultiVersionResolver(
bool HasDefault = false;
unsigned DefaultIndex = 0;
- SmallVector<CodeGenFunction::MultiVersionResolverOption, 10> CurrOptions(
- Options);
-
- llvm::stable_sort(
- CurrOptions, [](const CodeGenFunction::MultiVersionResolverOption &LHS,
- const CodeGenFunction::MultiVersionResolverOption &RHS) {
- return getPriorityFromAttrString(LHS.Conditions.Features[0]) >
- getPriorityFromAttrString(RHS.Conditions.Features[0]);
- });
-
// Check the each candidate function.
- for (unsigned Index = 0; Index < CurrOptions.size(); Index++) {
+ for (unsigned Index = 0; Index < Options.size(); Index++) {
- if (CurrOptions[Index].Conditions.Features[0].starts_with("default")) {
+ if (Options[Index].Conditions.Features[0].starts_with("default")) {
HasDefault = true;
DefaultIndex = Index;
continue;
@@ -2963,7 +2935,7 @@ void CodeGenFunction::EmitRISCVMultiVersionResolver(
std::vector<std::string> TargetAttrFeats =
getContext()
.getTargetInfo()
- .parseTargetAttr(CurrOptions[Index].Conditions.Features[0])
+ .parseTargetAttr(Options[Index].Conditions.Features[0])
.Features;
if (TargetAttrFeats.empty())
@@ -3004,7 +2976,7 @@ void CodeGenFunction::EmitRISCVMultiVersionResolver(
llvm::BasicBlock *RetBlock = createBasicBlock("resolver_return", Resolver);
CGBuilderTy RetBuilder(*this, RetBlock);
CreateMultiVersionResolverReturn(
- CGM, Resolver, RetBuilder, CurrOptions[Index].Function, SupportsIFunc);
+ CGM, Resolver, RetBuilder, Options[Index].Function, SupportsIFunc);
llvm::BasicBlock *ElseBlock = createBasicBlock("resolver_else", Resolver);
Builder.SetInsertPoint(CurBlock);
@@ -3017,7 +2989,7 @@ void CodeGenFunction::EmitRISCVMultiVersionResolver(
if (HasDefault) {
Builder.SetInsertPoint(CurBlock);
CreateMultiVersionResolverReturn(CGM, Resolver, Builder,
- CurrOptions[DefaultIndex].Function,
+ Options[DefaultIndex].Function,
SupportsIFunc);
return;
}
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index ba376f9ecfacde..fa569810da6296 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -4216,22 +4216,11 @@ static void ReplaceUsesOfNonProtoTypeWithRealFunction(llvm::GlobalValue *Old,
llvm::Function *NewFn);
static unsigned
-TargetMVPriority(const TargetInfo &TI,
- const CodeGenFunction::MultiVersionResolverOption &RO) {
- unsigned Priority = 0;
- unsigned NumFeatures = 0;
- for (StringRef Feat : RO.Conditions.Features) {
- Priority = std::max(Priority, TI.multiVersionSortPriority(Feat));
- NumFeatures++;
- }
-
- if (!RO.Conditions.Architecture.empty())
- Priority = std::max(
- Priority, TI.multiVersionSortPriority(RO.Conditions.Architecture));
-
- Priority += TI.multiVersionFeatureCost() * NumFeatures;
-
- return Priority;
+getFMVPriority(const TargetInfo &TI,
+ const CodeGenFunction::MultiVersionResolverOption &RO) {
+ llvm::SmallVector<StringRef, 8> Features{RO.Conditions.Features};
+ Features.push_back(RO.Conditions.Architecture);
+ return TI.getFMVPriority(Features);
}
// Multiversion functions should be at most 'WeakODRLinkage' so that a different
@@ -4362,7 +4351,7 @@ void CodeGenModule::emitMultiVersionFunctions() {
llvm::stable_sort(
Options, [&TI](const CodeGenFunction::MultiVersionResolverOption &LHS,
const CodeGenFunction::MultiVersionResolverOption &RHS) {
- return TargetMVPriority(TI, LHS) > TargetMVPriority(TI, RHS);
+ return getFMVPriority(TI, LHS) > getFMVPriority(TI, RHS);
});
CodeGenFunction CGF(*this);
CGF.EmitMultiVersionResolver(ResolverFunc, Options);
diff --git a/llvm/include/llvm/TargetParser/AArch64TargetParser.h b/llvm/include/llvm/TargetParser/AArch64TargetParser.h
index d7b1ba511f95d3..1311329821828f 100644
--- a/llvm/include/llvm/TargetParser/AArch64TargetParser.h
+++ b/llvm/include/llvm/TargetParser/AArch64TargetParser.h
@@ -268,6 +268,9 @@ void fillValidCPUArchList(SmallVectorImpl<StringRef> &Values);
bool isX18ReservedByDefault(const Triple &TT);
+// Return the priority for a given set of FMV features.
+unsigned getFMVPriority(ArrayRef<StringRef> Features);
+
// For given feature names, return a bitmask corresponding to the entries of
// AArch64::CPUFeatures. The values in CPUFeatures are not bitmasks
// themselves, they are sequential (0, 1, 2, 3, ...).
diff --git a/llvm/lib/TargetParser/AArch64TargetParser.cpp b/llvm/lib/TargetParser/AArch64TargetParser.cpp
index 45ecc4f24c2afd..fe5ab0fabefa6e 100644
--- a/llvm/lib/TargetParser/AArch64TargetParser.cpp
+++ b/llvm/lib/TargetParser/AArch64TargetParser.cpp
@@ -48,6 +48,19 @@ std::optional<AArch64::ArchInfo> AArch64::ArchInfo::findBySubArch(StringRef SubA
return {};
}
+unsigned AArch64::getFMVPriority(ArrayRef<StringRef> Features) {
+ constexpr unsigned MaxFMVPriority = 1000;
+ unsigned Priority = 0;
+ unsigned NumFeatures = 0;
+ for (StringRef Feature : Features) {
+ if (auto Ext = parseFMVExtension(Feature)) {
+ Priority = std::max(Priority, Ext->Priority);
+ NumFeatures++;
+ }
+ }
+ return Priority + MaxFMVPriority * NumFeatures;
+}
+
uint64_t AArch64::getCpuSupportsMask(ArrayRef<StringRef> FeatureStrs) {
uint64_t FeaturesMask = 0;
for (const StringRef &FeatureStr : FeatureStrs) {
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
BeMg
reviewed
Nov 15, 2024
labrinea
commented
Nov 15, 2024
| return TI.multiVersionSortPriority(LHS.substr(1)) > | ||
| TI.multiVersionSortPriority(RHS.substr(1)); | ||
| return TI.getFMVPriority({LHS.substr(1)}) > | ||
| TI.getFMVPriority({RHS.substr(1)}); |
There was a problem hiding this comment.
Perhaps there is value in adding another hook getFMVPriority(StringRef feature) which would be called on each feature of ArrayRef<StringRef> Features from the other hook, in order to avoid this inefficiency here of having to create a single element smallvector each time?
erichkeane
reviewed
Nov 15, 2024
jroelofs
reviewed
Nov 15, 2024
jroelofs
approved these changes
Nov 15, 2024
There was a problem hiding this comment.
LGTM as a refactor. We don't need to solve the spec clarification thing in this PR:
https://github.com/llvm/llvm-project/pull/116257/files#r1844489795
moved condition outside of lambda
* Parse riscv fmv attributes before passing to resolver options * Simplify resolver options struct
|
The latest changes c147f0c may go a bit beyond the scope of this refactoring. I wanted to address the matter raised on this thread #116257 (comment). That said I am happy to revert to the previous revision, unless people think otherwise. |
|
ping |
erichkeane
reviewed
Nov 25, 2024
There was a problem hiding this comment.
I THINK this looks right? X86 code ownership should probably make sure this is still correct for x86. @phoebewang as the defacto owner of x86 function multiversioning priority checking.
phoebewang
reviewed
Nov 26, 2024
|
@BeMg, does the RISC-V code look okay to you? |
BeMg
reviewed
Nov 26, 2024
* Do not split by ',' the target_clones attribute on x86. * Rename x86 specific parsing methods.
Use parseTargetAttr for already delimited attribute strings.
labrinea
added a commit
to labrinea/llvm-project
that referenced
this pull request
Nov 28, 2024
Fixes regression in sanitizer buildbots caused by llvm#116257.
labrinea
added a commit
that referenced
this pull request
Nov 28, 2024
Fixes regression in sanitizer buildbots caused by #116257.
kaladron
pushed a commit
to kaladron/llvm-project
that referenced
this pull request
Dec 23, 2024
Fixes regression in sanitizer buildbots caused by llvm#116257.
AdamGlass
pushed a commit
to AdamGlass/llvm-project
that referenced
this pull request
Dec 30, 2024
Fixes regression in sanitizer buildbots caused by llvm#116257.
AdamGlass
pushed a commit
to AdamGlass/llvm-project
that referenced
this pull request
Dec 30, 2024
Fixes regression in sanitizer buildbots caused by llvm#116257.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently we have code with target hooks in CodeGenModule shared between X86 and AArch64 for sorting MultiVersionResolverOptions. Those are used when generating IFunc resolvers for FMV. The RISCV target has different criteria for sorting, therefore it repeats sorting after calling CodeGenFunction::EmitMultiVersionResolver.
I am moving the FMV priority logic in TargetInfo, so that it can be implemented by the TargetParser which then makes it possible to query it from llvm. Here is an example why this is handy: #87939