[AArch64] Decouple feature dependency expansion.

clang/lib/AST/ASTContext.cpp

@@ -13665,9 +13665,9 @@ QualType ASTContext::getCorrespondingSignedFixedPointType(QualType Ty) const {
 }
 
 // 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) {
+static std::vector<std::string>
+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;
@@ -13676,6 +13676,7 @@ getFMVBackendFeaturesFor(const llvm::SmallVectorImpl<StringRef> &FMVFeatStrings,
         BackendFeats.push_back(F.str());
     }
   }
+  return BackendFeats;
 }
 
 ParsedTargetAttr
@@ -13736,27 +13737,27 @@ void ASTContext::getFunctionFeatureMap(llvm::StringMap<bool> &FeatureMap,
                     Target->getTargetOpts().FeaturesAsWritten.end());
     Target->initFeatureMap(FeatureMap, getDiagnostics(), TargetCPU, Features);
   } else if (const auto *TC = FD->getAttr<TargetClonesAttr>()) {
-    std::vector<std::string> Features;
     if (Target->getTriple().isAArch64()) {
       llvm::SmallVector<StringRef, 8> Feats;
       TC->getFeatures(Feats, GD.getMultiVersionIndex());
-      getFMVBackendFeaturesFor(Feats, Features);
+      std::vector<std::string> Features = getFMVBackendFeaturesFor(Feats);
       Features.insert(Features.begin(),
                       Target->getTargetOpts().FeaturesAsWritten.begin(),
                       Target->getTargetOpts().FeaturesAsWritten.end());
+      Target->initFeatureMap(FeatureMap, getDiagnostics(), TargetCPU, Features);
     } else {
+      std::vector<std::string> Features;
       StringRef VersionStr = TC->getFeatureStr(GD.getMultiVersionIndex());
       if (VersionStr.starts_with("arch="))
         TargetCPU = VersionStr.drop_front(sizeof("arch=") - 1);
       else if (VersionStr != "default")
         Features.push_back((StringRef{"+"} + VersionStr).str());
+      Target->initFeatureMap(FeatureMap, getDiagnostics(), TargetCPU, Features);
     }
-    Target->initFeatureMap(FeatureMap, getDiagnostics(), TargetCPU, Features);
   } else if (const auto *TV = FD->getAttr<TargetVersionAttr>()) {
     llvm::SmallVector<StringRef, 8> Feats;
     TV->getFeatures(Feats);
-    std::vector<std::string> Features;
-    getFMVBackendFeaturesFor(Feats, Features);
+    std::vector<std::string> Features = getFMVBackendFeaturesFor(Feats);
     Features.insert(Features.begin(),
                     Target->getTargetOpts().FeaturesAsWritten.begin(),
                     Target->getTargetOpts().FeaturesAsWritten.end());

clang/lib/Basic/Targets/AArch64.cpp

@@ -1056,6 +1056,12 @@ bool AArch64TargetInfo::handleTargetFeatures(std::vector<std::string> &Features,
 //  "tune=<cpu>" - TuneCPU set to <cpu>
 //  "feature", "no-feature" - Add (or remove) feature.
 //  "+feature", "+nofeature" - Add (or remove) feature.
+//
+// A feature may correspond to an Extension (anything with a corresponding
+// AEK_), in which case an ExtensionSet is used to parse it and expand its
+// dependencies. Otherwise the feature is passed through (e.g. +v8.1a,
+// +outline-atomics, -fmv, etc). Features coming from the command line are
+// already parsed, therefore their dependencies do not need expansion.
 ParsedTargetAttr AArch64TargetInfo::parseTargetAttr(StringRef Features) const {
   ParsedTargetAttr Ret;
   if (Features == "default")
@@ -1070,10 +1076,10 @@ ParsedTargetAttr AArch64TargetInfo::parseTargetAttr(StringRef Features) const {
     SmallVector<StringRef, 8> SplitFeatures;
     FeatString.split(SplitFeatures, StringRef("+"), -1, false);
     for (StringRef Feature : SplitFeatures) {
-      if (FeatureBits.parseModifier(Feature))
+      if (FeatureBits.parseAttributeModifier(Feature))
         continue;
-      // Pushing the original feature string to give a sema error later on
-      // when they get checked.
+      // Pass through features that are not extensions, e.g. +v8.1a,
+      // +outline-atomics, -fmv, etc.
       if (Feature.starts_with("no"))
         Features.push_back("-" + Feature.drop_front(2).str());
       else
@@ -1132,10 +1138,10 @@ ParsedTargetAttr AArch64TargetInfo::parseTargetAttr(StringRef Features) const {
     } else if (Feature.starts_with("+")) {
       SplitAndAddFeatures(Feature, Ret.Features, FeatureBits);
     } else {
-      if (FeatureBits.parseModifier(Feature))
+      if (FeatureBits.parseAttributeModifier(Feature))
         continue;
-      // Pushing the original feature string to give a sema error later on
-      // when they get checked.
+      // Pass through features that are not extensions, e.g. +v8.1a,
+      // +outline-atomics, -fmv, etc.
       if (Feature.starts_with("no-"))
         Ret.Features.push_back("-" + Feature.drop_front(3).str());
       else

clang/lib/Driver/ToolChains/Arch/AArch64.cpp

@@ -81,7 +81,7 @@ static bool DecodeAArch64Features(const Driver &D, StringRef text,
       D.Diag(clang::diag::err_drv_no_neon_modifier);
       continue;
     }
-    if (!Extensions.parseModifier(Feature))
+    if (!Extensions.parseCmdLineOptModifier(Feature))
       return false;
   }
 

clang/test/CodeGen/aarch64-targetattr.c

@@ -75,16 +75,9 @@ void all() {}
 __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() #16
-__attribute__((target("+noneon")))
-void plusnoneon() {}
 // CHECK-LABEL: @plusnosimd() #16
 __attribute__((target("+nosimd")))
 void plusnosimd() {}
-// CHECK-LABEL: @noneon() #16
-__attribute__((target("no-neon")))
-void noneon() {}
 // CHECK-LABEL: @nosimd() #16
 __attribute__((target("no-simd")))
 void nosimd() {}

clang/test/Preprocessor/aarch64-target-features.c

@@ -261,18 +261,6 @@
 // CHECK-FULLFP16-VECTOR-SCALAR: #define __ARM_FP 0xE
 // CHECK-FULLFP16-VECTOR-SCALAR: #define __ARM_FP16_FORMAT_IEEE 1
 
-// +fp16fml+nosimd doesn't make sense as the fp16fml instructions all require SIMD.
-// However, as +fp16fml implies +fp16 there is a set of defines that we would expect.
-// RUN: %clang --target=aarch64 -march=armv8-a+fp16fml+nosimd -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-FULLFP16-SCALAR %s
-// RUN: %clang --target=aarch64 -march=armv8-a+fp16+nosimd -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-FULLFP16-SCALAR %s
-// RUN: %clang --target=aarch64 -march=armv8.4-a+fp16fml+nosimd -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-FULLFP16-SCALAR %s
-// RUN: %clang --target=aarch64 -march=armv8.4-a+fp16+nosimd -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-FULLFP16-SCALAR %s
-// CHECK-FULLFP16-SCALAR-NOT:   #define __ARM_FEATURE_FP16_FML 1
-// CHECK-FULLFP16-SCALAR:       #define __ARM_FEATURE_FP16_SCALAR_ARITHMETIC 1
-// CHECK-FULLFP16-SCALAR-NOT:   #define __ARM_FEATURE_FP16_VECTOR_ARITHMETIC 1
-// CHECK-FULLFP16-SCALAR:       #define __ARM_FP 0xE
-// CHECK-FULLFP16-SCALAR:       #define __ARM_FP16_FORMAT_IEEE 1
-
 // RUN: %clang --target=aarch64 -march=armv8.2-a -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-FULLFP16-NOFML-VECTOR-SCALAR %s
 // RUN: %clang --target=aarch64 -march=armv8.2-a+nofp16 -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-FULLFP16-NOFML-VECTOR-SCALAR %s
 // RUN: %clang --target=aarch64 -march=armv8.2-a+nofp16fml -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-FULLFP16-NOFML-VECTOR-SCALAR %s
@@ -464,7 +452,7 @@
 // RUN: %clang -target aarch64 -mcpu=cortex-a53+noSIMD -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-MCPU-3 %s
 // CHECK-MCPU-1: "-cc1"{{.*}} "-triple" "aarch64{{.*}}" "-target-feature" "-aes"{{.*}} "-target-feature" "-sha2"
 // CHECK-MCPU-2: "-cc1"{{.*}} "-triple" "aarch64{{.*}}" "-target-feature" "+crc" "-target-feature" "+fp-armv8" "-target-feature" "+neon"
-// CHECK-MCPU-3: "-cc1"{{.*}} "-triple" "aarch64{{.*}}" "-target-feature" "+v8a" "-target-feature" "-aes" "-target-feature" "+crc" "-target-feature" "+fp-armv8" "-target-feature" "-sha2" "-target-feature" "-neon"
+// CHECK-MCPU-3: "-cc1"{{.*}} "-triple" "aarch64{{.*}}" "-target-feature" "+v8a" "-target-feature" "-aes" "-target-feature" "+crc" "-target-feature" "-fp-armv8" "-target-feature" "-sha2" "-target-feature" "-neon"
 
 // RUN: %clang -target aarch64 -mcpu=cyclone+nocrc+nocrypto -march=armv8-a -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-MCPU-MARCH %s
 // RUN: %clang -target aarch64 -march=armv8-a -mcpu=cyclone+nocrc+nocrypto  -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-MCPU-MARCH %s
@@ -494,7 +482,7 @@
 // RUN: %clang -target aarch64 -march=armv8.1a+noSIMD -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V81A-FEATURE-3 %s
 // CHECK-V81A-FEATURE-1: "-cc1"{{.*}} "-triple" "aarch64{{.*}}" "-target-feature" "+v8.1a" "-target-feature" "+aes" "-target-feature" "+crc" "-target-feature" "+crypto" "-target-feature" "+fp-armv8" "-target-feature" "+lse" "-target-feature" "+rdm" "-target-feature" "+sha2" "-target-feature" "+neon"
 // CHECK-V81A-FEATURE-2: "-cc1"{{.*}} "-triple" "aarch64{{.*}}" "-target-feature" "+v8.1a" "-target-feature" "+crc" "-target-feature" "+fp-armv8" "-target-feature" "+lse" "-target-feature" "+rdm" "-target-feature" "+neon"
-// CHECK-V81A-FEATURE-3: "-cc1"{{.*}} "-triple" "aarch64{{.*}}" "-target-feature" "+v8.1a" "-target-feature" "+crc" "-target-feature" "+fp-armv8" "-target-feature" "+lse" "-target-feature" "-rdm" "-target-feature" "-neon"
+// CHECK-V81A-FEATURE-3: "-cc1"{{.*}} "-triple" "aarch64{{.*}}" "-target-feature" "+v8.1a" "-target-feature" "+crc" "-target-feature" "-fp-armv8" "-target-feature" "+lse" "-target-feature" "-rdm" "-target-feature" "-neon"
 
 // ================== Check Memory Tagging Extensions (MTE).
 // RUN: %clang -target arm64-none-linux-gnu -march=armv8.5-a+memtag -x c -E -dM %s -o - 2>&1 | FileCheck -check-prefix=CHECK-MEMTAG %s

clang/test/Sema/builtin-cpu-supports.c

@@ -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"))
+  if (__builtin_cpu_supports("neon")) // expected-warning {{invalid cpu feature string for builtin}}
     a("vsx");
 
   if (__builtin_cpu_is("cortex-x3")) // expected-error {{builtin is not supported on this target}}

llvm/include/llvm/TargetParser/AArch64TargetParser.h

@@ -571,10 +571,20 @@ struct ExtensionSet {
   // Add or remove a feature based on a modifier string. The string must be of
   // the form "<name>" to enable a feature or "no<name>" to disable it. This
   // will also enable or disable any features as required by the dependencies
-  // between them.
-  bool parseModifier(StringRef Modifier);
+  // between them. The function is used for command line option parsing.
+  bool parseCmdLineOptModifier(StringRef Modifier);
 
-  void reconstructFromParsedFeatures(std::vector<std::string> &Features);
+  // Add or remove a feature based on a modifier string. The string must be of
+  // the form "<name>" to enable a feature or "no(-)<name>" to disable it. This
+  // will also enable or disable any features as required by the dependencies
+  // between them. The function is used for target(_version/_clones) attribute
+  // parsing.
+  bool parseAttributeModifier(StringRef Modifier);
+
+  // Constructs a new ExtensionSet by toggling the corresponding bits for every
+  // feature in the \p Features list without expanding their dependencies. Used
+  // for reconstructing an ExtensionSet from the output of toLLVMFeatures().
+  void reconstructFromParsedFeatures(const std::vector<std::string> &Features);
 
   // Convert the set of enabled extension to an LLVM feature list, appending
   // them to Features.

llvm/lib/Target/AArch64/AArch64Features.td

@@ -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", MArchAlias = "neon" in
+let ArchExtKindSpelling = "AEK_SIMD", MArchName = "simd" in
 def FeatureNEON : Extension<"neon", "NEON",
   "Enable Advanced SIMD instructions (FEAT_AdvSIMD)", [FeatureFPARMv8],
   "FEAT_SIMD", "+fp-armv8,+neon", 100>;

llvm/lib/TargetParser/AArch64TargetParser.cpp

@@ -220,6 +220,9 @@ void AArch64::ExtensionSet::disable(ArchExtKind E) {
   Touched.set(E);
   Enabled.reset(E);
 
+  if (E == AEK_SIMD)
+    disable(AEK_FP);
+
   // Recursively disable all features that depends on this one.
   for (auto Dep : ExtensionDependencies)
     if (E == Dep.Earlier)
@@ -245,20 +248,39 @@ void AArch64::ExtensionSet::addArchDefaults(const ArchInfo &Arch) {
       enable(E.ID);
 }
 
-bool AArch64::ExtensionSet::parseModifier(StringRef Modifier) {
-  LLVM_DEBUG(llvm::dbgs() << "parseModifier(" << Modifier << ")\n");
+bool AArch64::ExtensionSet::parseCmdLineOptModifier(StringRef Modifier) {
+  LLVM_DEBUG(llvm::dbgs() << "parseCmdLineOptModifier(" << Modifier << ")\n");
+
+  bool IsNegated = Modifier.starts_with("no");
+  StringRef ArchExt = IsNegated ? Modifier.drop_front(2) : Modifier;
+
+  if (auto AE = parseArchExtension(ArchExt)) {
+    if (AE->Feature.empty() || AE->NegFeature.empty())
+      return false;
+    if (IsNegated)
+      disable(AE->ID);
+    else
+      enable(AE->ID);
+    return true;
+  }
+  return false;
+}
+
+bool AArch64::ExtensionSet::parseAttributeModifier(StringRef Modifier) {
+  LLVM_DEBUG(llvm::dbgs() << "parseAttributeModifier(" << Modifier << ")\n");
 
   size_t NChars = 0;
   if (Modifier.starts_with("no-"))
     NChars = 3;
   else if (Modifier.starts_with("no"))
     NChars = 2;
+  bool IsNegated = NChars != 0;
   StringRef ArchExt = Modifier.drop_front(NChars);
 
   if (auto AE = parseArchExtension(ArchExt)) {
     if (AE->Feature.empty() || AE->NegFeature.empty())
       return false;
-    if (NChars)
+    if (IsNegated)
       disable(AE->ID);
     else
       enable(AE->ID);
@@ -268,7 +290,7 @@ bool AArch64::ExtensionSet::parseModifier(StringRef Modifier) {
 }
 
 void AArch64::ExtensionSet::reconstructFromParsedFeatures(
-    std::vector<std::string> &Features) {
+    const std::vector<std::string> &Features) {
   assert(Touched.none() && "Bitset already initialized");
   for (auto &F : Features) {
     bool IsNegated = F[0] == '-';

llvm/unittests/TargetParser/TargetParserTest.cpp

@@ -2303,7 +2303,7 @@ TEST_P(AArch64ExtensionDependenciesBaseArchTestFixture,
   llvm::AArch64::ExtensionSet Extensions;
   Extensions.addArchDefaults(Params.Arch);
   for (auto M : Params.Modifiers) {
-    bool success = Extensions.parseModifier(M);
+    bool success = Extensions.parseCmdLineOptModifier(M);
     EXPECT_TRUE(success);
   }
   std::vector<StringRef> Features;
@@ -2337,7 +2337,7 @@ TEST_P(AArch64ExtensionDependenciesBaseCPUTestFixture,
   EXPECT_TRUE(CPU);
   Extensions.addCPUDefaults(*CPU);
   for (auto M : Params.Modifiers) {
-    bool success = Extensions.parseModifier(M);
+    bool success = Extensions.parseCmdLineOptModifier(M);
     EXPECT_TRUE(success);
   }
   std::vector<StringRef> Features;