[AArch64] Decouple feature dependency expansion. #94279
Merged
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
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-driver @llvm/pr-subscribers-clang Author: Alexandros Lamprineas (labrinea) ChangesThe dependency expansion step which was introduced by FMV has been erroneously used for non-FMV features, for example when parsing the target attribute. The PR #93695 has rectified most of the tests which were relying on dependency expansion of target features specified on the -cc1 command line. In this patch I am decoupling the dependency expansion of features specified on the target attribute from FMV. To do that first I am expanding FMV dependencies before passing the list of target features to initFeatureMap(). Similarly when parsing the target attribute I am reconstructing an ExtensionSet from the list of target features which was created during the command line option parsing. The attribute parsing may toggle bits of that ExtensionSet and at the end it is converted to a list of target features. Those are passed to initFeatureMap(), which no longer requires an override. A side effect of this refactoring is that features specified on the target_version attribute now supersede the command line options, which is what should be happening in the first place. Patch is 43.07 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/94279.diff 13 Files Affected:
diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h
index a1d1d1c51cd41..8bce4812f0d48 100644
--- a/clang/include/clang/AST/ASTContext.h
+++ b/clang/include/clang/AST/ASTContext.h
@@ -3203,9 +3203,6 @@ class ASTContext : public RefCountedBase<ASTContext> {
/// valid feature names.
ParsedTargetAttr filterFunctionTargetAttrs(const TargetAttr *TD) const;
- std::vector<std::string>
- filterFunctionTargetVersionAttrs(const TargetVersionAttr *TV) const;
-
void getFunctionFeatureMap(llvm::StringMap<bool> &FeatureMap,
const FunctionDecl *) const;
void getFunctionFeatureMap(llvm::StringMap<bool> &FeatureMap,
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index 73d3b152c49f1..842b94b4a6833 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -87,6 +87,7 @@
#include "llvm/Support/MD5.h"
#include "llvm/Support/MathExtras.h"
#include "llvm/Support/raw_ostream.h"
+#include "llvm/TargetParser/AArch64TargetParser.h"
#include "llvm/TargetParser/Triple.h"
#include <algorithm>
#include <cassert>
@@ -13663,17 +13664,18 @@ QualType ASTContext::getCorrespondingSignedFixedPointType(QualType Ty) const {
}
}
-std::vector<std::string> ASTContext::filterFunctionTargetVersionAttrs(
- const TargetVersionAttr *TV) const {
- assert(TV != nullptr);
- llvm::SmallVector<StringRef, 8> Feats;
- std::vector<std::string> ResFeats;
- TV->getFeatures(Feats);
- for (auto &Feature : Feats)
- if (Target->validateCpuSupports(Feature.str()))
- // Use '?' to mark features that came from TargetVersion.
- ResFeats.push_back("?" + Feature.str());
- return ResFeats;
+// Given a list of FMV features, add each of their backend features to the list.
+static void
+getFMVBackendFeaturesFor(const llvm::SmallVectorImpl<StringRef> &FMVFeatStrings,
+ std::vector<std::string>& BackendFeats) {
+ for (StringRef F : FMVFeatStrings) {
+ if (auto FMVExt = llvm::AArch64::parseArchExtension(F)) {
+ SmallVector<StringRef, 8> Feats;
+ FMVExt->DependentFeatures.split(Feats, ',', -1, false);
+ for (StringRef F : Feats)
+ BackendFeats.push_back(F.str());
+ }
+ }
}
ParsedTargetAttr
@@ -13708,10 +13710,12 @@ void ASTContext::getFunctionFeatureMap(llvm::StringMap<bool> &FeatureMap,
// Make a copy of the features as passed on the command line into the
// beginning of the additional features from the function to override.
- ParsedAttr.Features.insert(
- ParsedAttr.Features.begin(),
- Target->getTargetOpts().FeaturesAsWritten.begin(),
- Target->getTargetOpts().FeaturesAsWritten.end());
+ // AArch64 handles command line option features in parseTargetAttr().
+ if (!Target->getTriple().isAArch64())
+ ParsedAttr.Features.insert(
+ ParsedAttr.Features.begin(),
+ Target->getTargetOpts().FeaturesAsWritten.begin(),
+ Target->getTargetOpts().FeaturesAsWritten.end());
if (ParsedAttr.CPU != "" && Target->isValidCPUName(ParsedAttr.CPU))
TargetCPU = ParsedAttr.CPU;
@@ -13734,13 +13738,9 @@ void ASTContext::getFunctionFeatureMap(llvm::StringMap<bool> &FeatureMap,
} else if (const auto *TC = FD->getAttr<TargetClonesAttr>()) {
std::vector<std::string> Features;
if (Target->getTriple().isAArch64()) {
- // TargetClones for AArch64
llvm::SmallVector<StringRef, 8> Feats;
TC->getFeatures(Feats, GD.getMultiVersionIndex());
- for (StringRef Feat : Feats)
- if (Target->validateCpuSupports(Feat.str()))
- // Use '?' to mark features that came from AArch64 TargetClones.
- Features.push_back("?" + Feat.str());
+ getFMVBackendFeaturesFor(Feats, Features);
Features.insert(Features.begin(),
Target->getTargetOpts().FeaturesAsWritten.begin(),
Target->getTargetOpts().FeaturesAsWritten.end());
@@ -13753,11 +13753,14 @@ void ASTContext::getFunctionFeatureMap(llvm::StringMap<bool> &FeatureMap,
}
Target->initFeatureMap(FeatureMap, getDiagnostics(), TargetCPU, Features);
} else if (const auto *TV = FD->getAttr<TargetVersionAttr>()) {
- std::vector<std::string> Feats = filterFunctionTargetVersionAttrs(TV);
- Feats.insert(Feats.begin(),
- Target->getTargetOpts().FeaturesAsWritten.begin(),
- Target->getTargetOpts().FeaturesAsWritten.end());
- Target->initFeatureMap(FeatureMap, getDiagnostics(), TargetCPU, Feats);
+ llvm::SmallVector<StringRef, 8> Feats;
+ TV->getFeatures(Feats);
+ std::vector<std::string> Features;
+ getFMVBackendFeaturesFor(Feats, Features);
+ Features.insert(Features.begin(),
+ Target->getTargetOpts().FeaturesAsWritten.begin(),
+ Target->getTargetOpts().FeaturesAsWritten.end());
+ Target->initFeatureMap(FeatureMap, getDiagnostics(), TargetCPU, Features);
} else {
FeatureMap = Target->getTargetOpts().FeatureMap;
}
diff --git a/clang/lib/Basic/Targets/AArch64.cpp b/clang/lib/Basic/Targets/AArch64.cpp
index 5db1ce78c657f..d8bb4fa3a3b49 100644
--- a/clang/lib/Basic/Targets/AArch64.cpp
+++ b/clang/lib/Basic/Targets/AArch64.cpp
@@ -1050,51 +1050,6 @@ bool AArch64TargetInfo::handleTargetFeatures(std::vector<std::string> &Features,
return true;
}
-bool AArch64TargetInfo::initFeatureMap(
- llvm::StringMap<bool> &Features, DiagnosticsEngine &Diags, StringRef CPU,
- const std::vector<std::string> &FeaturesVec) const {
- std::vector<std::string> UpdatedFeaturesVec;
- // Parse the CPU and add any implied features.
- std::optional<llvm::AArch64::CpuInfo> CpuInfo = llvm::AArch64::parseCpu(CPU);
- if (CpuInfo) {
- auto Exts = CpuInfo->getImpliedExtensions();
- std::vector<StringRef> CPUFeats;
- llvm::AArch64::getExtensionFeatures(Exts, CPUFeats);
- for (auto F : CPUFeats) {
- assert((F[0] == '+' || F[0] == '-') && "Expected +/- in target feature!");
- UpdatedFeaturesVec.push_back(F.str());
- }
- }
-
- // Process target and dependent features. This is done in two loops collecting
- // them into UpdatedFeaturesVec: first to add dependent '+'features, second to
- // add target '+/-'features that can later disable some of features added on
- // the first loop. Function Multi Versioning features begin with '?'.
- for (const auto &Feature : FeaturesVec)
- if (((Feature[0] == '?' || Feature[0] == '+')) &&
- AArch64TargetInfo::doesFeatureAffectCodeGen(Feature.substr(1))) {
- StringRef DepFeatures =
- AArch64TargetInfo::getFeatureDependencies(Feature.substr(1));
- SmallVector<StringRef, 1> AttrFeatures;
- DepFeatures.split(AttrFeatures, ",");
- for (auto F : AttrFeatures)
- UpdatedFeaturesVec.push_back(F.str());
- }
- for (const auto &Feature : FeaturesVec)
- if (Feature[0] != '?') {
- std::string UpdatedFeature = Feature;
- if (Feature[0] == '+') {
- std::optional<llvm::AArch64::ExtensionInfo> Extension =
- llvm::AArch64::parseArchExtension(Feature.substr(1));
- if (Extension)
- UpdatedFeature = Extension->Feature.str();
- }
- UpdatedFeaturesVec.push_back(UpdatedFeature);
- }
-
- return TargetInfo::initFeatureMap(Features, Diags, CPU, UpdatedFeaturesVec);
-}
-
// Parse AArch64 Target attributes, which are a comma separated list of:
// "arch=<arch>" - parsed to features as per -march=..
// "cpu=<cpu>" - parsed to features as per -mcpu=.., with CPU set to <cpu>
@@ -1110,23 +1065,26 @@ ParsedTargetAttr AArch64TargetInfo::parseTargetAttr(StringRef Features) const {
bool FoundArch = false;
auto SplitAndAddFeatures = [](StringRef FeatString,
- std::vector<std::string> &Features) {
+ std::vector<std::string> &Features,
+ llvm::AArch64::ExtensionSet &FeatureBits) {
SmallVector<StringRef, 8> SplitFeatures;
FeatString.split(SplitFeatures, StringRef("+"), -1, false);
for (StringRef Feature : SplitFeatures) {
- StringRef FeatureName = llvm::AArch64::getArchExtFeature(Feature);
- if (!FeatureName.empty())
- Features.push_back(FeatureName.str());
+ if (FeatureBits.parseModifier(Feature))
+ continue;
+ // Pushing the original feature string to give a sema error later on
+ // when they get checked.
+ if (Feature.starts_with("no"))
+ Features.push_back("-" + Feature.drop_front(2).str());
else
- // Pushing the original feature string to give a sema error later on
- // when they get checked.
- if (Feature.starts_with("no"))
- Features.push_back("-" + Feature.drop_front(2).str());
- else
- Features.push_back("+" + Feature.str());
+ Features.push_back("+" + Feature.str());
}
};
+ llvm::AArch64::ExtensionSet FeatureBits;
+ // Reconstruct the bitset from the command line option features.
+ FeatureBits.reconstructFromParsedFeatures(getTargetOpts().FeaturesAsWritten);
+
for (auto &Feature : AttrFeatures) {
Feature = Feature.trim();
if (Feature.starts_with("fpmath="))
@@ -1149,9 +1107,9 @@ ParsedTargetAttr AArch64TargetInfo::parseTargetAttr(StringRef Features) const {
// Ret.Features.
if (!AI)
continue;
- Ret.Features.push_back(AI->ArchFeature.str());
+ FeatureBits.addArchDefaults(*AI);
// Add any extra features, after the +
- SplitAndAddFeatures(Split.second, Ret.Features);
+ SplitAndAddFeatures(Split.second, Ret.Features, FeatureBits);
} else if (Feature.starts_with("cpu=")) {
if (!Ret.CPU.empty())
Ret.Duplicate = "cpu=";
@@ -1161,7 +1119,10 @@ ParsedTargetAttr AArch64TargetInfo::parseTargetAttr(StringRef Features) const {
std::pair<StringRef, StringRef> Split =
Feature.split("=").second.trim().split("+");
Ret.CPU = Split.first;
- SplitAndAddFeatures(Split.second, Ret.Features);
+ if (auto CpuInfo = llvm::AArch64::parseCpu(Ret.CPU)) {
+ FeatureBits.addCPUDefaults(*CpuInfo);
+ SplitAndAddFeatures(Split.second, Ret.Features, FeatureBits);
+ }
}
} else if (Feature.starts_with("tune=")) {
if (!Ret.Tune.empty())
@@ -1169,25 +1130,19 @@ ParsedTargetAttr AArch64TargetInfo::parseTargetAttr(StringRef Features) const {
else
Ret.Tune = Feature.split("=").second.trim();
} else if (Feature.starts_with("+")) {
- SplitAndAddFeatures(Feature, Ret.Features);
- } else if (Feature.starts_with("no-")) {
- StringRef FeatureName =
- llvm::AArch64::getArchExtFeature(Feature.split("-").second);
- if (!FeatureName.empty())
- Ret.Features.push_back("-" + FeatureName.drop_front(1).str());
- else
- Ret.Features.push_back("-" + Feature.split("-").second.str());
+ SplitAndAddFeatures(Feature, Ret.Features, FeatureBits);
} else {
- // Try parsing the string to the internal target feature name. If it is
- // invalid, add the original string (which could already be an internal
- // name). These should be checked later by isValidFeatureName.
- StringRef FeatureName = llvm::AArch64::getArchExtFeature(Feature);
- if (!FeatureName.empty())
- Ret.Features.push_back(FeatureName.str());
+ if (FeatureBits.parseModifier(Feature))
+ continue;
+ // Pushing the original feature string to give a sema error later on
+ // when they get checked.
+ if (Feature.starts_with("no-"))
+ Ret.Features.push_back("-" + Feature.drop_front(3).str());
else
Ret.Features.push_back("+" + Feature.str());
}
}
+ FeatureBits.toLLVMFeatureList(Ret.Features);
return Ret;
}
diff --git a/clang/lib/Basic/Targets/AArch64.h b/clang/lib/Basic/Targets/AArch64.h
index 12fb50286f751..696553ef8038a 100644
--- a/clang/lib/Basic/Targets/AArch64.h
+++ b/clang/lib/Basic/Targets/AArch64.h
@@ -107,10 +107,6 @@ class LLVM_LIBRARY_VISIBILITY AArch64TargetInfo : public TargetInfo {
unsigned multiVersionSortPriority(StringRef Name) const override;
unsigned multiVersionFeatureCost() const override;
- bool
- initFeatureMap(llvm::StringMap<bool> &Features, DiagnosticsEngine &Diags,
- StringRef CPU,
- const std::vector<std::string> &FeaturesVec) const override;
bool useFP16ConversionIntrinsics() const override {
return false;
}
diff --git a/clang/test/CodeGen/aarch64-cpu-supports-target.c b/clang/test/CodeGen/aarch64-cpu-supports-target.c
index e023944b24e53..4a8f7cf8595ca 100644
--- a/clang/test/CodeGen/aarch64-cpu-supports-target.c
+++ b/clang/test/CodeGen/aarch64-cpu-supports-target.c
@@ -48,5 +48,5 @@ int test_versions() {
return code();
}
// CHECK: attributes #0 = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" }
-// CHECK: attributes #1 = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="+neon" }
+// CHECK: attributes #1 = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="+fp-armv8,+neon" }
// CHECK: attributes #2 = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="+fp-armv8,+fullfp16,+neon,+sve" }
diff --git a/clang/test/CodeGen/aarch64-sme-intrinsics/aarch64-sme-attrs.cpp b/clang/test/CodeGen/aarch64-sme-intrinsics/aarch64-sme-attrs.cpp
index af8933d93d6cb..9885ac45e6a0e 100644
--- a/clang/test/CodeGen/aarch64-sme-intrinsics/aarch64-sme-attrs.cpp
+++ b/clang/test/CodeGen/aarch64-sme-intrinsics/aarch64-sme-attrs.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sme \
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sme -target-feature +bf16 \
// RUN: -disable-O0-optnone -Werror -emit-llvm -o - %s \
// RUN: | opt -S -passes=mem2reg \
// RUN: | opt -S -passes=inline \
diff --git a/clang/test/CodeGen/aarch64-targetattr.c b/clang/test/CodeGen/aarch64-targetattr.c
index 3e7a209245607..f84e72bdac6b7 100644
--- a/clang/test/CodeGen/aarch64-targetattr.c
+++ b/clang/test/CodeGen/aarch64-targetattr.c
@@ -58,58 +58,57 @@ void v1msve() {}
// CHECK-LABEL: @plussve() #12
__attribute__((target("+sve")))
void plussve() {}
-// CHECK-LABEL: @plussveplussve2() #13
+// CHECK-LABEL: @plussveplussve2() #12
__attribute__((target("+sve+nosve2")))
void plussveplussve2() {}
-// CHECK-LABEL: @plussveminusnosve2() #13
+// CHECK-LABEL: @plussveminusnosve2() #12
__attribute__((target("sve,no-sve2")))
void plussveminusnosve2() {}
-// CHECK-LABEL: @plusfp16() #14
+// CHECK-LABEL: @plusfp16() #13
__attribute__((target("+fp16")))
void plusfp16() {}
-// CHECK-LABEL: @all() #15
+// CHECK-LABEL: @all() #14
__attribute__((target("cpu=neoverse-n1,tune=cortex-a710,arch=armv8.6-a+sve2")))
void all() {}
-// CHECK-LABEL: @allplusbranchprotection() #16
+// CHECK-LABEL: @allplusbranchprotection() #15
__attribute__((target("cpu=neoverse-n1,tune=cortex-a710,arch=armv8.6-a+sve2,branch-protection=standard")))
void allplusbranchprotection() {}
// These tests check that the user facing and internal llvm name are both accepted.
-// CHECK-LABEL: @plusnoneon() #17
+// CHECK-LABEL: @plusnoneon() #16
__attribute__((target("+noneon")))
void plusnoneon() {}
-// CHECK-LABEL: @plusnosimd() #17
+// CHECK-LABEL: @plusnosimd() #16
__attribute__((target("+nosimd")))
void plusnosimd() {}
-// CHECK-LABEL: @noneon() #17
+// CHECK-LABEL: @noneon() #16
__attribute__((target("no-neon")))
void noneon() {}
-// CHECK-LABEL: @nosimd() #17
+// CHECK-LABEL: @nosimd() #16
__attribute__((target("no-simd")))
void nosimd() {}
// This isn't part of the standard interface, but test that -arch features should not apply anything else.
-// CHECK-LABEL: @minusarch() #18
+// CHECK-LABEL: @minusarch() #17
__attribute__((target("no-v9.3a")))
void minusarch() {}
// CHECK: attributes #0 = { {{.*}} "target-features"="+crc,+fp-armv8,+lse,+neon,+ras,+rdm,+v8.1a,+v8.2a,+v8a" }
// CHECK: attributes #1 = { {{.*}} "target-features"="+crc,+fp-armv8,+fullfp16,+lse,+neon,+ras,+rdm,+sve,+v8.1a,+v8.2a,+v8a" }
// CHECK: attributes #2 = { {{.*}} "target-features"="+crc,+fp-armv8,+fullfp16,+lse,+neon,+ras,+rdm,+sve,+sve2,+v8.1a,+v8.2a,+v8a" }
-// CHECK: attributes #3 = { {{.*}} "target-features"="+bf16,+complxnum,+crc,+dotprod,+fp-armv8,+fullfp16,+i8mm,+jsconv,+lse,+neon,+pauth,+ras,+rcpc,+rdm,+sve,+sve2,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8.5a,+v8.6a,+v8a" }
-// CHECK: attributes #4 = { {{.*}} "target-cpu"="cortex-a710" "target-features"="+bf16,+complxnum,+crc,+dotprod,+flagm,+fp-armv8,+fp16fml,+fullfp16,+i8mm,+jsconv,+lse,+mte,+neon,+pauth,+ras,+rcpc,+rdm,+sb,+sve,+sve2,+sve2-bitperm" }
+// CHECK: attributes #3 = { {{.*}} "target-features"="+bf16,+complxnum,+crc,+dotprod,+fp-armv8,+fp16fml,+fullfp16,+i8mm,+jsconv,+lse,+neon,+pauth,+ras,+rcpc,+rdm,+sve,+sve2,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8.5a,+v8.6a,+v8a" }
+// CHECK: attributes #4 = { {{.*}} "target-cpu"="cortex-a710" "target-features"="+bf16,+complxnum,+crc,+dotprod,+flagm,+fp-armv8,+fp16fml,+fullfp16,+i8mm,+jsconv,+lse,+mte,+neon,+pauth,+ras,+rcpc,+rdm,+sb,+sve,+sve2,+sve2-bitperm,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8.5a,+v8a,+v9a" }
// CHECK: attributes #5 = { {{.*}} "tune-cpu"="cortex-a710" }
// CHECK: attributes #6 = { {{.*}} "target-cpu"="generic" }
// CHECK: attributes #7 = { {{.*}} "tune-cpu"="generic" }
-// CHECK: attributes #8 = { {{.*}} "target-cpu"="neoverse-n1" "target-features"="+aes,+crc,+dotprod,+fp-armv8,+fullfp16,+lse,+neon,+ras,+rcpc,+rdm,+sha2,+spe,+ssbs" "tune-cpu"="cortex-a710" }
+// CHECK: attributes #8 = { {{.*}} "target-cpu"="neoverse-n1" "target-features"="+aes,+crc,+dotprod,+fp-armv8,+fullfp16,+lse,+neon,+ras,+rcpc,+rdm,+sha2,+spe,+ssbs,+v8.1a,+v8.2a,+v8a" "tune-cpu"="cortex-a710" }
// CHECK: attributes #9 = { {{.*}} "target-features"="+fp-armv8,+fullfp16,+neon,+sve" "tune-cpu"="cortex-a710" }
-// CHECK: attributes #10 = { {{.*}} "target-cpu"="neoverse-v1" "target-features"="+aes,+bf16,+complxnum,+crc,+dotprod,+fp-armv8,+fp16fml,+fullfp16,+i8mm,+jsconv,+lse,+neon,+pauth,+rand,+ras,+rcpc,+rdm,+sha2,+sha3,+sm4,+spe,+ssbs,+sve,+sve2" }
-// CHECK: attributes #11 = { {{.*}} "target-cpu"="neoverse-v1" "target-features"="+aes,+bf16,+complxnum,+crc,+dotprod,+fp-armv8,+fp16fml,+fullfp16,+i8mm,+jsconv,+lse,+neon,+pauth,+rand,+ras,+rcpc,+rdm,+sha2,+sha3,+sm4,+spe,+ssbs,-sve" }
+// CHECK: attributes #10 = { {{.*}} "target-cpu"="neoverse-v1" "target-features"="+aes,+bf16,+complxnum,+crc,+dotprod,+fp-armv8,+fp16fml,+fullfp16,+i8mm,+jsconv,+lse,+neon,+pauth,+rand,+ras,+rcpc,+rdm,+sha2,+sha3,+sm4,+spe,+ssbs,+sve,+sve2,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8a" }
+// CHECK: attributes #11 = { {{.*}} "target-cpu"="neoverse-v1" "target-features"="+aes,+bf16,+complxnum,+crc,+dotprod,+fp-armv8,+fp16fml,+fullfp16,+i8mm,+jsconv,+lse,+neon,+pauth,+rand,+ras,+rcpc,+rdm,+sha2,+sha3,+sm4,+spe,+ssbs,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8a,-sve" }
// CHECK: attributes #12 = { {{.*}} "target-features"="+fp-armv8,+fullfp16,+neon,+sve" }
-// CHECK: attributes #13 = { {{.*}} "target-features"="+fp-armv8,+fullfp16,+neon,+sve,-sve2" }
-// CHECK: attributes #14 = { {{.*}} "target-features"="+fullfp16" }
-// CHECK: attributes #15 = { {{.*}} "target-cpu"="neoverse-n1" "target-features"="+aes,+bf16,+complxnum,+crc,+dotprod,+fp-armv8,+fullfp16,+i8mm,+jsconv,+lse,+neon,+pauth,+ras,+rcpc,+rdm,+sha2,+spe,+ssbs,+sve,+sve2,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8.5a,+v8.6a,+v8a" "tune-cpu"="cortex-a710" }
-// CHECK: attributes #16 = { {{.*}} "branch-target-enforcement"="true" "guarded-control-stack"="true" {{.*}} "target-features"="+aes,+bf16,+complxnum,+crc,+dotprod,+fp-armv8,+fullfp16,+i8mm,+jsconv,+lse,+neon,+pauth,+ras,+rcpc,+rdm,+sha2,+spe,+ssbs,+sve,+sve2,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8.5a,+v8.6a,+v8a" "tune-cpu"="cortex-a710" }
-// CHECK: attributes #17 = { {{.*}} "target-features"="-neon" }
-// CHECK: attributes #18 = { {{.*}} "target-features"="-v9.3a" }
+// CHECK: attributes #13 = { {{.*}} "target-features"="+fp-armv8,+fullfp16,+neon" }
+// CHECK: attributes #14 = { {{.*}} "target-cpu"="neoverse-n1" "target-features"="+aes,+bf16,+complxnum,+crc,+dotprod,+fp-armv8,+fullfp16,+i8mm,+jsconv,+lse,+neon,+pauth,+ras,+rcpc,+rdm,+sha2,+spe,+ssbs,+sve,+sve2,+v8.1a,+v8.2a,+v8...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
The dependency expansion step which was introduced by FMV has been erroneously used for non-FMV features, for example when parsing the target attribute. The PR llvm#93695 has rectified most of the tests which were relying on dependency expansion of target features specified on the -cc1 command line. In this patch I am decoupling the dependency expansion of features specified on the target attribute from FMV. To do that first I am expanding FMV dependencies before passing the list of target features to initFeatureMap(). Similarly when parsing the target attribute I am reconstructing an ExtensionSet from the list of target features which was created during the command line option parsing. The attribute parsing may toggle bits of that ExtensionSet and at the end it is converted to a list of target features. Those are passed to initFeatureMap(), which no longer requires an override. A side effect of this refactoring is that features specified on the target_version attribute now supersede the command line options, which is what should be happening in the first place.
labrinea
force-pushed
the
decouple-feature-dependency-expansion
branch
from
2f15ae2 to
a413428
Compare
labrinea
commented
Jun 3, 2024
| @@ -27,7 +27,7 @@ int main(void) { | |||
| (void)__builtin_cpu_supports("x86-64-v4"); | |||
| (void)__builtin_cpu_supports("x86-64-v5"); // expected-warning {{invalid cpu feature string for builtin}} | |||
| #else | |||
| if (__builtin_cpu_supports("neon")) // expected-warning {{invalid cpu feature string for builtin}} | |||
There was a problem hiding this comment.
If we want the target attribute to accept it then we should accept it here too, otherwise be consistent and reject in both cases.
labrinea
commented
Jun 3, 2024
labrinea
commented
Jun 3, 2024
| @@ -106,7 +106,7 @@ def FeatureFPARMv8 : Extension<"fp-armv8", "FPARMv8", | |||
| "Enable ARMv8 (FEAT_FP)", [], | |||
| "FEAT_FP", "+fp-armv8,+neon", 90>; | |||
|
|
|||
| let ArchExtKindSpelling = "AEK_SIMD", MArchName = "simd" in | |||
| let ArchExtKindSpelling = "AEK_SIMD", MArchName = "simd", MArchAlias = "neon" in | |||
There was a problem hiding this comment.
Necessary alias needed for the target attribute parsing.
There was a problem hiding this comment.
This will enable the alias on -march too. Instead we should disallow clang internal names in the target attribute.
labrinea
commented
Jun 4, 2024
labrinea
commented
Jun 4, 2024
| @@ -106,7 +106,7 @@ def FeatureFPARMv8 : Extension<"fp-armv8", "FPARMv8", | |||
| "Enable ARMv8 (FEAT_FP)", [], | |||
| "FEAT_FP", "+fp-armv8,+neon", 90>; | |||
|
|
|||
| let ArchExtKindSpelling = "AEK_SIMD", MArchName = "simd" in | |||
| let ArchExtKindSpelling = "AEK_SIMD", MArchName = "simd", MArchAlias = "neon" in | |||
There was a problem hiding this comment.
This will enable the alias on -march too. Instead we should disallow clang internal names in the target attribute.
* return getFMVBackendFeaturesFor by value * add comments in AArch64TargetInfo::parseTargetAttr() * reject 'neon' in target attribute * add constness to the argument of reconstructFromParsedFeatures() * split ExtensionSet::parseModifier in two (cmdline vs attribute) * disable AEK_FP when disabling AEK_SIMD and adjust tests
llvmbot
added
the
clang:driver
* clang format * Do not make AEK_FP depend on AEK_SIMD.
tmatheson-arm
approved these changes
Jun 5, 2024
There was a problem hiding this comment.
LGTM. The main change to point out is that the target attribute will no longer accept internal feature names. I don't think it should ever have done so, but we should get input from others. @davemgreen? There are references to existing code in D137617 but no details. If this has been used for e.g. intrinsics definitions, I am surprised there are not more test failures.
* Unified parseAttributeModifier with parseCmdLineOptModifier * Changed a comment for getFMVBackendFeaturesFor()
labrinea
commented
Jun 5, 2024
| StringRef FeatureName = llvm::AArch64::getArchExtFeature(Feature); | ||
| if (!FeatureName.empty()) | ||
| Features.push_back(FeatureName.str()); | ||
| if (FeatureBits.parseModifier(Feature, /* AllowNoDashForm = */ true)) |
There was a problem hiding this comment.
AllowNoDashForm should perhaps be set to false here? I am not sure.
davemgreen
reviewed
Jun 5, 2024
There was a problem hiding this comment.
LGTM. The main change to point out is that the target attribute will no longer accept internal feature names. I don't think it should ever have done so, but we should get input from others. @davemgreen? There are references to existing code in D137617 but no details. If this has been used for e.g. intrinsics definitions, I am surprised there are not more test failures.
Hi - It was intentional to support older versions of clang. The target attributes already had users before I fixed them to support the same formats as GCC for AArch64, and was aiming at not breaking the existing code. IIRC There are quite a few uses of things like target("crypto") out there (without the + that gcc wants to include).
I'm not sure if that extends to internal feature names a lot. Not supporting "neon" as a name would seem like a mistake if it was removed, but I don't believe this patch does that. If it only effects negative features those have never worked particularly well.
It does. See #94279 (comment) for more context. There's even a semantic error when using neon on the command line. Why would the target attribute be the exception ? |
|
Yeah I had just seen that error message before you edited your comment. There are some examples of neon I found in a quick search, which were presumably added for AArch32: But like I said, if I try this patch locally then |
|
You are right, |
labrinea
added a commit
to labrinea/llvm-project
that referenced
this pull request
Jun 10, 2024
This reverts commit 2cf1439 since it broke the llvm test suite: SingleSource/UnitTests/AArch64/acle-fmv-features.c:59:9: error: instruction requires: altnzcv SingleSource/UnitTests/AArch64/acle-fmv-features.c:117:10: error: instruction requires: aes ... Looks like the FMV dependencies were used in the target attribute and now features that are FMVOnly (have AEK_NONE) cannot be expanded in parseTargetAttr using the ExtensionSet. This suggests that either the tests are wrong (they are using an FMVOnly feature in a target attribute), or that we need to turn the FMVOnly features into Extensions (these two are tablegen classes).
labrinea
added a commit
that referenced
this pull request
Jun 10, 2024
…95056) This reverts commit 2cf1439 since it broke the llvm test suite: SingleSource/UnitTests/AArch64/acle-fmv-features.c:59:9: error: instruction requires: altnzcv SingleSource/UnitTests/AArch64/acle-fmv-features.c:117:10: error: instruction requires: aes ... Looks like the FMV dependencies were used in the target attribute and now features that are FMVOnly (have AEK_NONE) cannot be expanded in parseTargetAttr using the ExtensionSet. This suggests that either the tests are wrong (they are using an FMVOnly feature in a target attribute), or that we need to turn the FMVOnly features into Extensions (these two are tablegen classes).
Lukacma
pushed a commit
to Lukacma/llvm-project
that referenced
this pull request
Jun 12, 2024
Introduced by 2cf1439 (llvm#94279). See also 6c369cf. The build system cannot track transitive dependencies on generated headers for some reason.
Lukacma
pushed a commit
to Lukacma/llvm-project
that referenced
this pull request
Jun 12, 2024
llvm#95056) This reverts commit 2cf1439 since it broke the llvm test suite: SingleSource/UnitTests/AArch64/acle-fmv-features.c:59:9: error: instruction requires: altnzcv SingleSource/UnitTests/AArch64/acle-fmv-features.c:117:10: error: instruction requires: aes ... Looks like the FMV dependencies were used in the target attribute and now features that are FMVOnly (have AEK_NONE) cannot be expanded in parseTargetAttr using the ExtensionSet. This suggests that either the tests are wrong (they are using an FMVOnly feature in a target attribute), or that we need to turn the FMVOnly features into Extensions (these two are tablegen classes).
labrinea
added a commit
to labrinea/llvm-project
that referenced
this pull request
Jun 12, 2024
My reverted attempt to decouple feature dependency expansion (see llvm#95056) made it evident that some features are still using the FMV dependencies in the target attribute. The original commit broke the llvm test suite. This was addressed here: llvm/llvm-test-suite#133. I am now relanding it.
labrinea
added a commit
that referenced
this pull request
Jun 12, 2024
…95231) My reverted attempt to decouple feature dependency expansion (see #95056) made it evident that some features are still using the FMV dependencies in the target attribute. The original commit broke the llvm test suite. This was addressed here: llvm/llvm-test-suite#133. I am now relanding it.
Closed
MaskRay
added a commit
that referenced
this pull request
Jun 13, 2024
)" (#95231)" This reverts commit 7051073. The following code is now incorrectly rejected. ``` % cat neon.c #include <arm_neon.h> __attribute__((target("arch=armv8-a"))) uint64x2_t foo(uint64x2_t a, uint64x2_t b) { return veorq_u64(a, b); } % newclang --target=aarch64-linux-gnu -c neon.c neon.c:5:10: error: always_inline function 'veorq_u64' requires target feature 'outline-atomics', but would be inlined into function 'foo' that is compiled without support for 'outline-atomics' 5 | return veorq_u64(a, b); | ^ 1 error generated. ``` "+outline-atomics" seems misleading here.
labrinea
added a commit
to labrinea/llvm-project
that referenced
this pull request
Jun 14, 2024
This is the second attempt. We should be inserting the Driver
features in front of the features of a parsed target attribute
to avoid errors like the following:
```
% cat neon.c
__attribute__((target("arch=armv8-a")))
uint64x2_t foo(uint64x2_t a, uint64x2_t b) { return veorq_u64(a, b); }
% clang --target=aarch64-linux-gnu -c neon.c
error: always_inline function 'veorq_u64' requires target feature
'outline-atomics', but would be inlined into function 'foo'
that is compiled without support for 'outline-atomics'
```
labrinea
added a commit
that referenced
this pull request
Jun 18, 2024
…95519) This is the second attempt. When parsing the target attribute we should be letting cc1 features which don't correspond to Extensions pass through to avoid errors like the following: % cat neon.c __attribute__((target("arch=armv8-a"))) uint64x2_t foo(uint64x2_t a, uint64x2_t b) { return veorq_u64(a, b); } % clang --target=aarch64-linux-gnu -c neon.c error: always_inline function 'veorq_u64' requires target feature 'outline-atomics', but would be inlined into function 'foo' that is compiled without support for 'outline-atomics' Co-authored-by: Tomas Matheson <Tomas.Matheson@arm.com>
AlexisPerry
pushed a commit
to llvm-project-tlp/llvm-project
that referenced
this pull request
Jul 9, 2024
llvm#95519) This is the second attempt. When parsing the target attribute we should be letting cc1 features which don't correspond to Extensions pass through to avoid errors like the following: % cat neon.c __attribute__((target("arch=armv8-a"))) uint64x2_t foo(uint64x2_t a, uint64x2_t b) { return veorq_u64(a, b); } % clang --target=aarch64-linux-gnu -c neon.c error: always_inline function 'veorq_u64' requires target feature 'outline-atomics', but would be inlined into function 'foo' that is compiled without support for 'outline-atomics' Co-authored-by: Tomas Matheson <Tomas.Matheson@arm.com>
EthanLuisMcDonough
pushed a commit
to EthanLuisMcDonough/llvm-project
that referenced
this pull request
Aug 13, 2024
…m#94279)" (llvm#95231)" This reverts commit 7051073. The following code is now incorrectly rejected. ``` % cat neon.c #include <arm_neon.h> __attribute__((target("arch=armv8-a"))) uint64x2_t foo(uint64x2_t a, uint64x2_t b) { return veorq_u64(a, b); } % newclang --target=aarch64-linux-gnu -c neon.c neon.c:5:10: error: always_inline function 'veorq_u64' requires target feature 'outline-atomics', but would be inlined into function 'foo' that is compiled without support for 'outline-atomics' 5 | return veorq_u64(a, b); | ^ 1 error generated. ``` "+outline-atomics" seems misleading 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.
The dependency expansion step which was introduced by FMV has been erroneously used for non-FMV features, for example when parsing the target attribute. The PR #93695 has rectified most of the tests which were relying on dependency expansion of target features specified on the -cc1 command line. In this patch I am decoupling the dependency expansion of features specified on the target attribute from FMV.
To do that first I am expanding FMV dependencies before passing the list of target features to initFeatureMap(). Similarly when parsing the target attribute I am reconstructing an ExtensionSet from the list of target features which was created during the command line option parsing. The attribute parsing may toggle bits of that ExtensionSet and at the end it is converted to a list of target features. Those are passed to initFeatureMap(), which no longer requires an override.
A side effect of this refactoring is that features specified on the target_version attribute now supersede the command line options, which is what should be happening in the first place.