[PATCH] D145538: [NFC][AArch64] Document and improve FMV code.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG124b46a897a7: [NFC][AArch64] Document and improve FMV code.
(authored by ilinpv).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D145538/new/
https://reviews.llvm.org/D145538
Files:
clang/include/clang/Basic/TargetInfo.h
clang/lib/AST/ASTContext.cpp
clang/lib/Basic/Targets/AArch64.cpp
clang/lib/Basic/Targets/AArch64.h
clang/lib/Sema/SemaDeclAttr.cpp
llvm/include/llvm/TargetParser/AArch64TargetParser.h
Index: llvm/include/llvm/TargetParser/AArch64TargetParser.h
===
--- llvm/include/llvm/TargetParser/AArch64TargetParser.h
+++ llvm/include/llvm/TargetParser/AArch64TargetParser.h
@@ -25,6 +25,9 @@
class Triple;
namespace AArch64 {
+// Function Multi Versioning CPU features. They must be kept in sync with
+// compiler-rt enum CPUFeatures in lib/builtins/cpu_model.c with FEAT_MAX as
+// sentinel.
enum CPUFeatures {
FEAT_RNG,
FEAT_FLAGM,
@@ -87,6 +90,9 @@
FEAT_MAX
};
+static_assert(FEAT_MAX <= 64,
+ "CPUFeatures enum must not have more than 64 entries");
+
// Arch extension modifiers for CPUs. These are labelled with their Arm ARM
// feature name (though the canonical reference for those is AArch64.td)
// clang-format off
@@ -155,17 +161,18 @@
// SubtargetFeature which may represent either an actual extension or some
// internal LLVM property.
struct ExtensionInfo {
- StringRef Name; // Human readable name, e.g. "profile".
- ArchExtKind ID; // Corresponding to the ArchExtKind, this extensions
-// representation in the bitfield.
- StringRef Feature;// -mattr enable string, e.g. "+spe"
- StringRef NegFeature; // -mattr disable string, e.g. "-spe"
-
- // FIXME These were added by D127812 FMV support and need documenting:
- CPUFeatures CPUFeature; // Bitfield value set in __aarch64_cpu_features
- StringRef DependentFeatures;
- unsigned FmvPriority;
- static constexpr unsigned MaxFMVPriority = 1000;
+ StringRef Name; // Human readable name, e.g. "profile".
+ ArchExtKind ID; // Corresponding to the ArchExtKind, this
+ // extensions representation in the bitfield.
+ StringRef Feature; // -mattr enable string, e.g. "+spe"
+ StringRef NegFeature;// -mattr disable string, e.g. "-spe"
+ CPUFeatures CPUFeature; // Function Multi Versioning (FMV) bitfield value
+ // set in __aarch64_cpu_features
+ StringRef DependentFeatures; // FMV enabled features string,
+ // e.g. "+dotprod,+fp-armv8,+neon"
+ unsigned FmvPriority;// FMV feature priority
+ static constexpr unsigned MaxFMVPriority =
+ 1000; // Maximum priority for FMV feature
};
// clang-format off
@@ -559,6 +566,10 @@
void fillValidCPUArchList(SmallVectorImpl );
bool isX18ReservedByDefault(const Triple );
+
+// 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, ...).
uint64_t getCpuSupportsMask(ArrayRef FeatureStrs);
} // namespace AArch64
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -3508,6 +3508,7 @@
enum SecondParam { None, CPU, Tune };
enum ThirdParam { Target, TargetClones };
HasCommas = HasCommas || Str.contains(',');
+ const TargetInfo = Context.getTargetInfo();
// Warn on empty at the beginning of a string.
if (Str.size() == 0)
return Diag(LiteralLoc, diag::warn_unsupported_target_attribute)
@@ -3517,9 +3518,9 @@
while (!Parts.second.empty()) {
Parts = Parts.second.split(',');
StringRef Cur = Parts.first.trim();
-SourceLocation CurLoc = Literal->getLocationOfByte(
-Cur.data() - Literal->getString().data(), getSourceManager(),
-getLangOpts(), Context.getTargetInfo());
+SourceLocation CurLoc =
+Literal->getLocationOfByte(Cur.data() - Literal->getString().data(),
+ getSourceManager(), getLangOpts(), TInfo);
bool DefaultIsDupe = false;
bool HasCodeGenImpact = false;
@@ -3527,7 +3528,7 @@
return Diag(CurLoc, diag::warn_unsupported_target_attribute)
<< Unsupported << None << "" << TargetClones;
-if (Context.getTargetInfo().getTriple().isAArch64()) {
+if (TInfo.getTriple().isAArch64()) {
// AArch64 target clones specific
if (Cur == "default") {
DefaultIsDupe = HasDefault;
@@ -3542,13 +3543,12 @@
while (!CurParts.second.empty()) {
CurParts = CurParts.second.split('+');
StringRef CurFeature = CurParts.first.trim();
- if
[PATCH] D145538: [NFC][AArch64] Document and improve FMV code.
ilinpv marked 5 inline comments as done. ilinpv added inline comments. Comment at: llvm/include/llvm/TargetParser/AArch64TargetParser.h:567-568 + +// For given features returns a mask to check if CPU support them. The mask is +// used in Function Multi Versioning resolver conditions code generation. uint64_t getCpuSupportsMask(ArrayRef FeatureStrs); tmatheson wrote: > `CPUFeatures` has 60 entries, which means the return value here will overflow > if we add a few more entries. We should probably have a > `static_assert(FEAT_MAX <= 64)` in the implementation. Or should the > `CPUFeatures` values actually be bitmasks, like ArchExtKind? static_assert added. I think changing values to masks could be done separately, it would be good to have if we eventually come to CPUFeatures and ArchExtKind unification. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145538/new/ https://reviews.llvm.org/D145538 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D145538: [NFC][AArch64] Document and improve FMV code.
ilinpv updated this revision to Diff 503446.
ilinpv added a comment.
Rebasing and addressing comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D145538/new/
https://reviews.llvm.org/D145538
Files:
clang/include/clang/Basic/TargetInfo.h
clang/lib/AST/ASTContext.cpp
clang/lib/Basic/Targets/AArch64.cpp
clang/lib/Basic/Targets/AArch64.h
clang/lib/Sema/SemaDeclAttr.cpp
llvm/include/llvm/TargetParser/AArch64TargetParser.h
Index: llvm/include/llvm/TargetParser/AArch64TargetParser.h
===
--- llvm/include/llvm/TargetParser/AArch64TargetParser.h
+++ llvm/include/llvm/TargetParser/AArch64TargetParser.h
@@ -25,6 +25,9 @@
class Triple;
namespace AArch64 {
+// Function Multi Versioning CPU features. They must be kept in sync with
+// compiler-rt enum CPUFeatures in lib/builtins/cpu_model.c with FEAT_MAX as
+// sentinel.
enum CPUFeatures {
FEAT_RNG,
FEAT_FLAGM,
@@ -87,6 +90,9 @@
FEAT_MAX
};
+static_assert(FEAT_MAX <= 64,
+ "CPUFeatures enum must not have more than 64 entries");
+
// Arch extension modifiers for CPUs. These are labelled with their Arm ARM
// feature name (though the canonical reference for those is AArch64.td)
// clang-format off
@@ -155,17 +161,18 @@
// SubtargetFeature which may represent either an actual extension or some
// internal LLVM property.
struct ExtensionInfo {
- StringRef Name; // Human readable name, e.g. "profile".
- ArchExtKind ID; // Corresponding to the ArchExtKind, this extensions
-// representation in the bitfield.
- StringRef Feature;// -mattr enable string, e.g. "+spe"
- StringRef NegFeature; // -mattr disable string, e.g. "-spe"
-
- // FIXME These were added by D127812 FMV support and need documenting:
- CPUFeatures CPUFeature; // Bitfield value set in __aarch64_cpu_features
- StringRef DependentFeatures;
- unsigned FmvPriority;
- static constexpr unsigned MaxFMVPriority = 1000;
+ StringRef Name; // Human readable name, e.g. "profile".
+ ArchExtKind ID; // Corresponding to the ArchExtKind, this
+ // extensions representation in the bitfield.
+ StringRef Feature; // -mattr enable string, e.g. "+spe"
+ StringRef NegFeature;// -mattr disable string, e.g. "-spe"
+ CPUFeatures CPUFeature; // Function Multi Versioning (FMV) bitfield value
+ // set in __aarch64_cpu_features
+ StringRef DependentFeatures; // FMV enabled features string,
+ // e.g. "+dotprod,+fp-armv8,+neon"
+ unsigned FmvPriority;// FMV feature priority
+ static constexpr unsigned MaxFMVPriority =
+ 1000; // Maximum priority for FMV feature
};
// clang-format off
@@ -559,6 +566,10 @@
void fillValidCPUArchList(SmallVectorImpl );
bool isX18ReservedByDefault(const Triple );
+
+// 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, ...).
uint64_t getCpuSupportsMask(ArrayRef FeatureStrs);
} // namespace AArch64
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -3508,6 +3508,7 @@
enum SecondParam { None, CPU, Tune };
enum ThirdParam { Target, TargetClones };
HasCommas = HasCommas || Str.contains(',');
+ const TargetInfo = Context.getTargetInfo();
// Warn on empty at the beginning of a string.
if (Str.size() == 0)
return Diag(LiteralLoc, diag::warn_unsupported_target_attribute)
@@ -3517,9 +3518,9 @@
while (!Parts.second.empty()) {
Parts = Parts.second.split(',');
StringRef Cur = Parts.first.trim();
-SourceLocation CurLoc = Literal->getLocationOfByte(
-Cur.data() - Literal->getString().data(), getSourceManager(),
-getLangOpts(), Context.getTargetInfo());
+SourceLocation CurLoc =
+Literal->getLocationOfByte(Cur.data() - Literal->getString().data(),
+ getSourceManager(), getLangOpts(), TInfo);
bool DefaultIsDupe = false;
bool HasCodeGenImpact = false;
@@ -3527,7 +3528,7 @@
return Diag(CurLoc, diag::warn_unsupported_target_attribute)
<< Unsupported << None << "" << TargetClones;
-if (Context.getTargetInfo().getTriple().isAArch64()) {
+if (TInfo.getTriple().isAArch64()) {
// AArch64 target clones specific
if (Cur == "default") {
DefaultIsDupe = HasDefault;
@@ -3542,13 +3543,12 @@
while (!CurParts.second.empty()) {
CurParts = CurParts.second.split('+');
StringRef CurFeature = CurParts.first.trim();
- if (!Context.getTargetInfo().validateCpuSupports(CurFeature)) {
+ if
[PATCH] D145538: [NFC][AArch64] Document and improve FMV code.
tmatheson accepted this revision. tmatheson added a comment. This revision is now accepted and ready to land. LGTM, thanks for making these changes. Comment at: llvm/include/llvm/TargetParser/AArch64TargetParser.h:567-568 + +// For given features returns a mask to check if CPU support them. The mask is +// used in Function Multi Versioning resolver conditions code generation. uint64_t getCpuSupportsMask(ArrayRef FeatureStrs); `CPUFeatures` has 60 entries, which means the return value here will overflow if we add a few more entries. We should probably have a `static_assert(FEAT_MAX <= 64)` in the implementation. Or should the `CPUFeatures` values actually be bitmasks, like ArchExtKind? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145538/new/ https://reviews.llvm.org/D145538 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D145538: [NFC][AArch64] Document and improve FMV code.
aaron.ballman added a comment.
Generally seems reasonable to me, but I'll give others a chance to comment.
Comment at: clang/lib/AST/ASTContext.cpp:13459
if (Target->validateCpuSupports(Feature.str()))
+ // Use '?' to mark features that came from TargetVersion
ResFeats.push_back("?" + Feature.str());
Comment at: clang/lib/AST/ASTContext.cpp:13529
VFeature = VFeature.trim();
+ // Use '?' to mark features that came from AArch64 TargetClones
Features.push_back((StringRef{"?"} + VFeature).str());
Comment at: clang/lib/Basic/Targets/AArch64.cpp:610-612
+ for (const auto : llvm::AArch64::Extensions)
+if (Name == E.Name)
+ return !E.DependentFeatures.empty();
Good place for `llvm::find_if` rather than a manual loop?
Comment at: clang/lib/Basic/Targets/AArch64.cpp:617
+StringRef AArch64TargetInfo::getFeatureDependencies(StringRef Name) const {
+ for (const auto : llvm::AArch64::Extensions)
+if (Name == E.Name)
Good place for `llvm::find_if` rather than a manual loop?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D145538/new/
https://reviews.llvm.org/D145538
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D145538: [NFC][AArch64] Document and improve FMV code.
ilinpv created this revision.
ilinpv added reviewers: tmatheson, danielkiss.
Herald added a subscriber: kristof.beyls.
Herald added a reviewer: aaron.ballman.
Herald added a project: All.
ilinpv requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D145538
Files:
clang/include/clang/Basic/TargetInfo.h
clang/lib/AST/ASTContext.cpp
clang/lib/Basic/Targets/AArch64.cpp
clang/lib/Basic/Targets/AArch64.h
clang/lib/Sema/SemaDeclAttr.cpp
llvm/include/llvm/TargetParser/AArch64TargetParser.h
Index: llvm/include/llvm/TargetParser/AArch64TargetParser.h
===
--- llvm/include/llvm/TargetParser/AArch64TargetParser.h
+++ llvm/include/llvm/TargetParser/AArch64TargetParser.h
@@ -25,6 +25,9 @@
class Triple;
namespace AArch64 {
+// Function Multi Versioning CPU features. They must be kept in sync with
+// compiler-rt enum CPUFeatures in lib/builtins/cpu_model.c with FEAT_MAX as
+// sentinel.
enum CPUFeatures {
FEAT_RNG,
FEAT_FLAGM,
@@ -155,17 +158,18 @@
// SubtargetFeature which may represent either an actual extension or some
// internal LLVM property.
struct ExtensionInfo {
- StringRef Name; // Human readable name, e.g. "profile".
- ArchExtKind ID; // Corresponding to the ArchExtKind, this extensions
-// representation in the bitfield.
- StringRef Feature;// -mattr enable string, e.g. "+spe"
- StringRef NegFeature; // -mattr disable string, e.g. "-spe"
-
- // FIXME These were added by D127812 FMV support and need documenting:
- CPUFeatures CPUFeature; // Bitfield value set in __aarch64_cpu_features
- StringRef DependentFeatures;
- unsigned FmvPriority;
- static constexpr unsigned MaxFMVPriority = 1000;
+ StringRef Name; // Human readable name, e.g. "profile".
+ ArchExtKind ID; // Corresponding to the ArchExtKind, this
+ // extensions representation in the bitfield.
+ StringRef Feature; // -mattr enable string, e.g. "+spe"
+ StringRef NegFeature;// -mattr disable string, e.g. "-spe"
+ CPUFeatures CPUFeature; // Function Multi Versioning (FMV) bitfield value
+ // set in __aarch64_cpu_features
+ StringRef DependentFeatures; // FMV enabled features string,
+ // e.g. "+dotprod,+fp-armv8,+neon"
+ unsigned FmvPriority;// FMV feature priority
+ static constexpr unsigned MaxFMVPriority =
+ 1000; // Maximum priority for FMV feature
};
// clang-format off
@@ -559,6 +563,9 @@
void fillValidCPUArchList(SmallVectorImpl );
bool isX18ReservedByDefault(const Triple );
+
+// For given features returns a mask to check if CPU support them. The mask is
+// used in Function Multi Versioning resolver conditions code generation.
uint64_t getCpuSupportsMask(ArrayRef FeatureStrs);
} // namespace AArch64
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -3508,6 +3508,7 @@
enum SecondParam { None, CPU, Tune };
enum ThirdParam { Target, TargetClones };
HasCommas = HasCommas || Str.contains(',');
+ const TargetInfo = Context.getTargetInfo();
// Warn on empty at the beginning of a string.
if (Str.size() == 0)
return Diag(LiteralLoc, diag::warn_unsupported_target_attribute)
@@ -3517,9 +3518,9 @@
while (!Parts.second.empty()) {
Parts = Parts.second.split(',');
StringRef Cur = Parts.first.trim();
-SourceLocation CurLoc = Literal->getLocationOfByte(
-Cur.data() - Literal->getString().data(), getSourceManager(),
-getLangOpts(), Context.getTargetInfo());
+SourceLocation CurLoc =
+Literal->getLocationOfByte(Cur.data() - Literal->getString().data(),
+ getSourceManager(), getLangOpts(), TInfo);
bool DefaultIsDupe = false;
bool HasCodeGenImpact = false;
@@ -3527,7 +3528,7 @@
return Diag(CurLoc, diag::warn_unsupported_target_attribute)
<< Unsupported << None << "" << TargetClones;
-if (Context.getTargetInfo().getTriple().isAArch64()) {
+if (TInfo.getTriple().isAArch64()) {
// AArch64 target clones specific
if (Cur == "default") {
DefaultIsDupe = HasDefault;
@@ -3542,13 +3543,12 @@
while (!CurParts.second.empty()) {
CurParts = CurParts.second.split('+');
StringRef CurFeature = CurParts.first.trim();
- if (!Context.getTargetInfo().validateCpuSupports(CurFeature)) {
+ if (!TInfo.validateCpuSupports(CurFeature)) {
Diag(CurLoc, diag::warn_unsupported_target_attribute)
<< Unsupported << None << CurFeature << TargetClones;
continue;
}
-
