Reland "[AArch64] Decouple feature dependency expansion. (llvm#94279)" (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 <[email protected]>

Author: 2 people

clang/include/clang/AST/ASTContext.h

@@ -3210,9 +3210,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,

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>
@@ -13676,17 +13677,20 @@ 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, return a concatenated list of the
+// corresponding backend features (which may contain duplicates).
+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;
+      FMVExt->DependentFeatures.split(Feats, ',', -1, false);
+      for (StringRef F : Feats)
+        BackendFeats.push_back(F.str());
+    }
+  }
+  return BackendFeats;
 }
 
 ParsedTargetAttr
@@ -13721,10 +13725,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;
@@ -13745,32 +13751,31 @@ 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()) {
-      // 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());
+      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>()) {
-    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.insert(Features.begin(),
+                    Target->getTargetOpts().FeaturesAsWritten.begin(),
+                    Target->getTargetOpts().FeaturesAsWritten.end());
+    Target->initFeatureMap(FeatureMap, getDiagnostics(), TargetCPU, Features);
   } else {
     FeatureMap = Target->getTargetOpts().FeatureMap;
   }

clang/lib/AST/CMakeLists.txt

@@ -139,4 +139,6 @@ add_clang_library(clangAST
   omp_gen
   ClangDriverOptions
   intrinsics_gen
+  # These generated headers are included transitively.
+  AArch64TargetParserTableGen
   )

clang/lib/Basic/Targets/AArch64.cpp

@@ -1064,57 +1064,17 @@ 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>
 //  "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. If the feature does not yield a successful parse then it
+// is passed through.
 ParsedTargetAttr AArch64TargetInfo::parseTargetAttr(StringRef Features) const {
   ParsedTargetAttr Ret;
   if (Features == "default")
@@ -1124,23 +1084,31 @@ 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;
+      // Pass through anything that failed to parse so that we can emit
+      // diagnostics, as well as valid internal feature names.
+      //
+      // FIXME: We should consider rejecting internal feature names like
+      //        neon, v8a, etc.
+      // FIXME: We should consider emitting diagnostics here.
+      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,
+                                            Ret.Features);
+
   for (auto &Feature : AttrFeatures) {
     Feature = Feature.trim();
     if (Feature.starts_with("fpmath="))
@@ -1163,9 +1131,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=";
@@ -1175,33 +1143,34 @@ 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())
         Ret.Duplicate = "tune=";
       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, /* AllowNoDashForm = */ true))
+        continue;
+      // Pass through anything that failed to parse so that we can emit
+      // diagnostics, as well as valid internal feature names.
+      //
+      // FIXME: We should consider rejecting internal feature names like
+      //        neon, v8a, etc.
+      // FIXME: We should consider emitting diagnostics here.
+      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;
 }
 

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;
   }

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 #2 = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="+fp-armv8,+fullfp16,+neon,+sve" }
+// 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,+sve" }

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 \