[RISCV] Overwrite cpu target features for full arch string in target attribute #77426
Conversation
This file contains hidden or 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
…attribute This patch reworks RISCVTargetInfo::initFeatureMap to fix the issue described in llvm#74889 (review) (and is an alternative to llvm#75804) When a full arch string is specified, a "full" list of extensions is now passed after the __RISCV_TargetAttrNeedOverride marker feature, which includes any negative features that disable ISA extensions. In initFeatureMap, there are now two code paths: 1. If the arch string was overriden, use the "full" list of override features, only adding back any non-isa features that were specified. Using the full list of positive and negative features will mean that the target-cpu will have no effect on the final arch, e.g. __attribute__((target("arch=rv64i"))) with -mcpu=sifive-x280 will have the features for rv64i, not a mix of both. 2. Otherwise, parse and *append* the list of implied features. By appending, we turn back on any features that might have been disabled by a negative extension, i.e. this handles the case fixed in llvm#74889.
lukel97
requested review from
asb,
BeMg,
kito-cheng,
MaskRay,
preames and
topperc
llvmbot
added
clang
|
@llvm/pr-subscribers-backend-risc-v @llvm/pr-subscribers-clang Author: Luke Lau (lukel97) ChangesThis patch reworks RISCVTargetInfo::initFeatureMap to fix the issue described When a full arch string is specified, a "full" list of extensions is now passed In initFeatureMap, there are now two code paths:
Using the full list of positive and negative features will mean that the
Full diff: https://github.com/llvm/llvm-project/pull/77426.diff 2 Files Affected:
diff --git a/clang/lib/Basic/Targets/RISCV.cpp b/clang/lib/Basic/Targets/RISCV.cpp
index daaa8639ae8358..b56c1d465ad77a 100644
--- a/clang/lib/Basic/Targets/RISCV.cpp
+++ b/clang/lib/Basic/Targets/RISCV.cpp
@@ -235,39 +235,6 @@ ArrayRef<Builtin::Info> RISCVTargetInfo::getTargetBuiltins() const {
clang::RISCV::LastTSBuiltin - Builtin::FirstTSBuiltin);
}
-static std::vector<std::string>
-collectNonISAExtFeature(ArrayRef<std::string> FeaturesNeedOverride, int XLen) {
- std::vector<std::string> NonISAExtFeatureVec;
-
- auto IsNonISAExtFeature = [](const std::string &Feature) {
- assert(Feature.size() > 1 && (Feature[0] == '+' || Feature[0] == '-'));
- StringRef Ext = StringRef(Feature).drop_front(); // drop the +/-
- return !llvm::RISCVISAInfo::isSupportedExtensionFeature(Ext);
- };
- llvm::copy_if(FeaturesNeedOverride, std::back_inserter(NonISAExtFeatureVec),
- IsNonISAExtFeature);
-
- return NonISAExtFeatureVec;
-}
-
-static std::vector<std::string>
-resolveTargetAttrOverride(const std::vector<std::string> &FeaturesVec,
- int XLen) {
- auto I = llvm::find(FeaturesVec, "__RISCV_TargetAttrNeedOverride");
- if (I == FeaturesVec.end())
- return FeaturesVec;
-
- ArrayRef<std::string> FeaturesNeedOverride(&*FeaturesVec.begin(), &*I);
- std::vector<std::string> NonISAExtFeature =
- collectNonISAExtFeature(FeaturesNeedOverride, XLen);
-
- std::vector<std::string> ResolvedFeature(++I, FeaturesVec.end());
- ResolvedFeature.insert(ResolvedFeature.end(), NonISAExtFeature.begin(),
- NonISAExtFeature.end());
-
- return ResolvedFeature;
-}
-
bool RISCVTargetInfo::initFeatureMap(
llvm::StringMap<bool> &Features, DiagnosticsEngine &Diags, StringRef CPU,
const std::vector<std::string> &FeaturesVec) const {
@@ -281,10 +248,27 @@ bool RISCVTargetInfo::initFeatureMap(
Features["32bit"] = true;
}
- std::vector<std::string> NewFeaturesVec =
- resolveTargetAttrOverride(FeaturesVec, XLen);
+ // If a target attribute specified a full arch string, override all the ISA
+ // extension target features.
+ const auto I = llvm::find(FeaturesVec, "__RISCV_TargetAttrNeedOverride");
+ if (I != FeaturesVec.end()) {
+ std::vector OverrideFeatures = std::vector(std::next(I), FeaturesVec.end());
+
+ // Add back any non ISA extension features, e.g. +relax.
+ auto IsNonISAExtFeature = [](const std::string &Feature) {
+ assert(Feature.size() > 1 && (Feature[0] == '+' || Feature[0] == '-'));
+ std::string Ext = Feature.substr(1); // drop the +/-
+ return !llvm::RISCVISAInfo::isSupportedExtensionFeature(Ext);
+ };
+ llvm::copy_if(llvm::make_range(FeaturesVec.begin(), I),
+ std::back_inserter(OverrideFeatures), IsNonISAExtFeature);
- auto ParseResult = llvm::RISCVISAInfo::parseFeatures(XLen, NewFeaturesVec);
+ return TargetInfo::initFeatureMap(Features, Diags, CPU, OverrideFeatures);
+ }
+
+ // Otherwise, parse the features and add any implied extensions.
+ std::vector AllFeatures = FeaturesVec;
+ auto ParseResult = llvm::RISCVISAInfo::parseFeatures(XLen, FeaturesVec);
if (!ParseResult) {
std::string Buffer;
llvm::raw_string_ostream OutputErrMsg(Buffer);
@@ -295,21 +279,9 @@ bool RISCVTargetInfo::initFeatureMap(
return false;
}
- // RISCVISAInfo makes implications for ISA features
- std::vector<std::string> ImpliedFeatures = (*ParseResult)->toFeatures();
-
- // parseFeatures normalizes the feature set by dropping any explicit
- // negatives, and non-extension features. We need to preserve the later
- // for correctness and want to preserve the former for consistency.
- for (auto &Feature : NewFeaturesVec) {
- StringRef ExtName = Feature;
- assert(ExtName.size() > 1 && (ExtName[0] == '+' || ExtName[0] == '-'));
- ExtName = ExtName.drop_front(1); // Drop '+' or '-'
- if (!llvm::is_contained(ImpliedFeatures, ("+" + ExtName).str()) &&
- !llvm::is_contained(ImpliedFeatures, ("-" + ExtName).str()))
- ImpliedFeatures.push_back(Feature);
- }
- return TargetInfo::initFeatureMap(Features, Diags, CPU, ImpliedFeatures);
+ // Append all features, not just new ones, so we override any negatives.
+ llvm::append_range(AllFeatures, (*ParseResult)->toFeatures());
+ return TargetInfo::initFeatureMap(Features, Diags, CPU, AllFeatures);
}
std::optional<std::pair<unsigned, unsigned>>
@@ -413,7 +385,9 @@ static void handleFullArchString(StringRef FullArchStr,
// Forward the invalid FullArchStr.
Features.push_back("+" + FullArchStr.str());
} else {
- std::vector<std::string> FeatStrings = (*RII)->toFeatures();
+ // Append a full list of features, including any negative extensions so that
+ // we override the CPU's features.
+ std::vector<std::string> FeatStrings = (*RII)->toFeatures(true);
Features.insert(Features.end(), FeatStrings.begin(), FeatStrings.end());
}
}
diff --git a/clang/test/CodeGen/RISCV/riscv-func-attr-target.c b/clang/test/CodeGen/RISCV/riscv-func-attr-target.c
index 759c33a2250600..54a0aad8a18b43 100644
--- a/clang/test/CodeGen/RISCV/riscv-func-attr-target.c
+++ b/clang/test/CodeGen/RISCV/riscv-func-attr-target.c
@@ -40,8 +40,8 @@ __attribute__((target("cpu=sifive-u54"))) void testAttrCpuOnly() {}
// CHECK: attributes #1 = { {{.*}}"target-cpu"="rocket-rv64" "target-features"="+64bit,+a,+d,+f,+m,+save-restore,+v,+zicsr,+zifencei,+zve32f,+zve32x,+zve64d,+zve64f,+zve64x,+zvl128b,+zvl32b,+zvl64b,-relax,-zbb,-zfa" "tune-cpu"="generic-rv64" }
// CHECK: attributes #2 = { {{.*}}"target-features"="+64bit,+a,+m,+save-restore,+zbb,+zifencei,-relax,-zfa" }
// CHECK: attributes #3 = { {{.*}}"target-features"="+64bit,+a,+d,+experimental-zicond,+f,+m,+save-restore,+v,+zbb,+zicsr,+zifencei,+zve32f,+zve32x,+zve64d,+zve64f,+zve64x,+zvl128b,+zvl32b,+zvl64b,-relax,-zfa" }
-// CHECK: attributes #4 = { {{.*}}"target-features"="+64bit,+a,+c,+d,+f,+m,+save-restore,+zbb,+zicsr,+zifencei,-relax" }
-// CHECK: attributes #5 = { {{.*}}"target-features"="+64bit,+m,+save-restore,-relax" }
+// CHECK: attributes #4 = { {{.*}}"target-features"="+64bit,+a,+c,+d,+f,+m,+save-restore,+zbb,+zicsr,+zifencei,-e,-experimental-zacas,-experimental-zcmop,-experimental-zfbfmin,-experimental-zicfilp,-experimental-zicfiss,-experimental-zicond,-experimental-zimop,-experimental-ztso,-experimental-zvfbfmin,-experimental-zvfbfwma,-h,-relax,-smaia,-ssaia,-svinval,-svnapot,-svpbmt,-v,-xcvalu,-xcvbi,-xcvbitmanip,-xcvelw,-xcvmac,-xcvmem,-xcvsimd,-xsfvcp,-xsfvfnrclipxfqf,-xsfvfwmaccqqq,-xsfvqmaccdod,-xsfvqmaccqoq,-xtheadba,-xtheadbb,-xtheadbs,-xtheadcmo,-xtheadcondmov,-xtheadfmemidx,-xtheadmac,-xtheadmemidx,-xtheadmempair,-xtheadsync,-xtheadvdot,-xventanacondops,-zawrs,-zba,-zbc,-zbkb,-zbkc,-zbkx,-zbs,-zca,-zcb,-zcd,-zce,-zcf,-zcmp,-zcmt,-zdinx,-zfa,-zfh,-zfhmin,-zfinx,-zhinx,-zhinxmin,-zicbom,-zicbop,-zicboz,-zicntr,-zihintntl,-zihintpause,-zihpm,-zk,-zkn,-zknd,-zkne,-zknh,-zkr,-zks,-zksed,-zksh,-zkt,-zmmul,-zvbb,-zvbc,-zve32f,-zve32x,-zve64d,-zve64f,-zve64x,-zvfh,-zvfhmin,-zvkb,-zvkg,-zvkn,-zvknc,-zvkned,-zvkng,-zvknha,-zvknhb,-zvks,-zvksc,-zvksed,-zvksg,-zvksh,-zvkt,-zvl1024b,-zvl128b,-zvl16384b,-zvl2048b,-zvl256b,-zvl32768b,-zvl32b,-zvl4096b,-zvl512b,-zvl64b,-zvl65536b,-zvl8192b" }
+// CHECK: attributes #5 = { {{.*}}"target-features"="+64bit,+m,+save-restore,-a,-c,-d,-e,-experimental-zacas,-experimental-zcmop,-experimental-zfbfmin,-experimental-zicfilp,-experimental-zicfiss,-experimental-zicond,-experimental-zimop,-experimental-ztso,-experimental-zvfbfmin,-experimental-zvfbfwma,-f,-h,-relax,-smaia,-ssaia,-svinval,-svnapot,-svpbmt,-v,-xcvalu,-xcvbi,-xcvbitmanip,-xcvelw,-xcvmac,-xcvmem,-xcvsimd,-xsfvcp,-xsfvfnrclipxfqf,-xsfvfwmaccqqq,-xsfvqmaccdod,-xsfvqmaccqoq,-xtheadba,-xtheadbb,-xtheadbs,-xtheadcmo,-xtheadcondmov,-xtheadfmemidx,-xtheadmac,-xtheadmemidx,-xtheadmempair,-xtheadsync,-xtheadvdot,-xventanacondops,-zawrs,-zba,-zbb,-zbc,-zbkb,-zbkc,-zbkx,-zbs,-zca,-zcb,-zcd,-zce,-zcf,-zcmp,-zcmt,-zdinx,-zfa,-zfh,-zfhmin,-zfinx,-zhinx,-zhinxmin,-zicbom,-zicbop,-zicboz,-zicntr,-zicsr,-zifencei,-zihintntl,-zihintpause,-zihpm,-zk,-zkn,-zknd,-zkne,-zknh,-zkr,-zks,-zksed,-zksh,-zkt,-zmmul,-zvbb,-zvbc,-zve32f,-zve32x,-zve64d,-zve64f,-zve64x,-zvfh,-zvfhmin,-zvkb,-zvkg,-zvkn,-zvknc,-zvkned,-zvkng,-zvknha,-zvknhb,-zvks,-zvksc,-zvksed,-zvksg,-zvksh,-zvkt,-zvl1024b,-zvl128b,-zvl16384b,-zvl2048b,-zvl256b,-zvl32768b,-zvl32b,-zvl4096b,-zvl512b,-zvl64b,-zvl65536b,-zvl8192b" }
// CHECK: attributes #6 = { {{.*}}"target-cpu"="sifive-u54" "target-features"="+64bit,+a,+m,+save-restore,+zbb,+zifencei,-relax,-zfa" }
-// CHECK: attributes #7 = { {{.*}}"target-cpu"="sifive-u54" "target-features"="+64bit,+m,+save-restore,-relax" }
-// CHECK: attributes #8 = { {{.*}}"target-cpu"="sifive-u54" "target-features"="+64bit,+a,+c,+d,+f,+m,+save-restore,+zicsr,+zifencei,-relax" }
+// CHECK: attributes #7 = { {{.*}}"target-cpu"="sifive-u54" "target-features"="+64bit,+m,+save-restore,-a,-c,-d,-e,-experimental-zacas,-experimental-zcmop,-experimental-zfbfmin,-experimental-zicfilp,-experimental-zicfiss,-experimental-zicond,-experimental-zimop,-experimental-ztso,-experimental-zvfbfmin,-experimental-zvfbfwma,-f,-h,-relax,-smaia,-ssaia,-svinval,-svnapot,-svpbmt,-v,-xcvalu,-xcvbi,-xcvbitmanip,-xcvelw,-xcvmac,-xcvmem,-xcvsimd,-xsfvcp,-xsfvfnrclipxfqf,-xsfvfwmaccqqq,-xsfvqmaccdod,-xsfvqmaccqoq,-xtheadba,-xtheadbb,-xtheadbs,-xtheadcmo,-xtheadcondmov,-xtheadfmemidx,-xtheadmac,-xtheadmemidx,-xtheadmempair,-xtheadsync,-xtheadvdot,-xventanacondops,-zawrs,-zba,-zbb,-zbc,-zbkb,-zbkc,-zbkx,-zbs,-zca,-zcb,-zcd,-zce,-zcf,-zcmp,-zcmt,-zdinx,-zfa,-zfh,-zfhmin,-zfinx,-zhinx,-zhinxmin,-zicbom,-zicbop,-zicboz,-zicntr,-zicsr,-zifencei,-zihintntl,-zihintpause,-zihpm,-zk,-zkn,-zknd,-zkne,-zknh,-zkr,-zks,-zksed,-zksh,-zkt,-zmmul,-zvbb,-zvbc,-zve32f,-zve32x,-zve64d,-zve64f,-zve64x,-zvfh,-zvfhmin,-zvkb,-zvkg,-zvkn,-zvknc,-zvkned,-zvkng,-zvknha,-zvknhb,-zvks,-zvksc,-zvksed,-zvksg,-zvksh,-zvkt,-zvl1024b,-zvl128b,-zvl16384b,-zvl2048b,-zvl256b,-zvl32768b,-zvl32b,-zvl4096b,-zvl512b,-zvl64b,-zvl65536b,-zvl8192b" }
+// CHECK: attributes #8 = { {{.*}}"target-cpu"="sifive-u54" "target-features"="+64bit,+a,+c,+d,+f,+m,+save-restore,+zicsr,+zifencei,-e,-experimental-zacas,-experimental-zcmop,-experimental-zfbfmin,-experimental-zicfilp,-experimental-zicfiss,-experimental-zicond,-experimental-zimop,-experimental-ztso,-experimental-zvfbfmin,-experimental-zvfbfwma,-h,-relax,-smaia,-ssaia,-svinval,-svnapot,-svpbmt,-v,-xcvalu,-xcvbi,-xcvbitmanip,-xcvelw,-xcvmac,-xcvmem,-xcvsimd,-xsfvcp,-xsfvfnrclipxfqf,-xsfvfwmaccqqq,-xsfvqmaccdod,-xsfvqmaccqoq,-xtheadba,-xtheadbb,-xtheadbs,-xtheadcmo,-xtheadcondmov,-xtheadfmemidx,-xtheadmac,-xtheadmemidx,-xtheadmempair,-xtheadsync,-xtheadvdot,-xventanacondops,-zawrs,-zba,-zbb,-zbc,-zbkb,-zbkc,-zbkx,-zbs,-zca,-zcb,-zcd,-zce,-zcf,-zcmp,-zcmt,-zdinx,-zfa,-zfh,-zfhmin,-zfinx,-zhinx,-zhinxmin,-zicbom,-zicbop,-zicboz,-zicntr,-zihintntl,-zihintpause,-zihpm,-zk,-zkn,-zknd,-zkne,-zknh,-zkr,-zks,-zksed,-zksh,-zkt,-zmmul,-zvbb,-zvbc,-zve32f,-zve32x,-zve64d,-zve64f,-zve64x,-zvfh,-zvfhmin,-zvkb,-zvkg,-zvkn,-zvknc,-zvkned,-zvkng,-zvknha,-zvknhb,-zvks,-zvksc,-zvksed,-zvksg,-zvksh,-zvkt,-zvl1024b,-zvl128b,-zvl16384b,-zvl2048b,-zvl256b,-zvl32768b,-zvl32b,-zvl4096b,-zvl512b,-zvl64b,-zvl65536b,-zvl8192b" }
|
topperc
reviewed
Jan 11, 2024
topperc
reviewed
Jan 11, 2024
topperc
reviewed
Jan 17, 2024
clang/lib/Basic/Targets/RISCV.cpp
Outdated
| // extension target features. | ||
| const auto I = llvm::find(FeaturesVec, "__RISCV_TargetAttrNeedOverride"); | ||
| if (I != FeaturesVec.end()) { | ||
| std::vector<std::string> OverrideFeatures = |
There was a problem hiding this comment.
std::vector<std::string> OverrideFeatures(std::next(I), FeaturesVec.end());
clang/lib/Basic/Targets/RISCV.cpp
Outdated
| std::vector(std::next(I), FeaturesVec.end()); | ||
|
|
||
| // Add back any non ISA extension features, e.g. +relax. | ||
| auto IsNonISAExtFeature = [](const std::string &Feature) { |
clang/lib/Basic/Targets/RISCV.cpp
Outdated
| // Add back any non ISA extension features, e.g. +relax. | ||
| auto IsNonISAExtFeature = [](const std::string &Feature) { | ||
| assert(Feature.size() > 1 && (Feature[0] == '+' || Feature[0] == '-')); | ||
| std::string Ext = Feature.substr(1); // drop the +/- |
There was a problem hiding this comment.
StringRef Ext = Feature.drop_front(1)
topperc
reviewed
Jan 17, 2024
clang/lib/Basic/Targets/RISCV.cpp
Outdated
| } | ||
|
|
||
| // Otherwise, parse the features and add any implied extensions. | ||
| std::vector AllFeatures = FeaturesVec; |
There was a problem hiding this comment.
This might need to be std::vector<std::string> Also I'm not we should copy the vector here just to rename it?
There was a problem hiding this comment.
FeaturesVec is a const reference argument so we can't mutate it unfortunately. Is there another way to avoid the copy here?
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.
This patch reworks RISCVTargetInfo::initFeatureMap to fix the issue described
in #74889 (review)
(and is an alternative to #75804)
When a full arch string is specified, a "full" list of extensions is now passed
after the __RISCV_TargetAttrNeedOverride marker feature, which includes any
negative features that disable ISA extensions.
In initFeatureMap, there are now two code paths:
only adding back any non-isa features that were specified.
Using the full list of positive and negative features will mean that the
target-cpu will have no effect on the final arch, e.g.
attribute((target("arch=rv64i"))) with -mcpu=sifive-x280 will have the
features for rv64i, not a mix of both.
turn back on any features that might have been disabled by a negative
extension, i.e. this handles the case fixed in [RISCV] Enable target attribute when invoked through clang driver #74889.