Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "[AArch64] Decouple feature dependency expansion. (#94279)" #95056

Merged

Conversation

labrinea
Copy link
Collaborator

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).

@llvmbot llvmbot added clang Clang issues not falling into any other category backend:AArch64 clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jun 10, 2024
@llvmbot
Copy link
Member

llvmbot commented Jun 10, 2024

@llvm/pr-subscribers-backend-aarch64

@llvm/pr-subscribers-clang

Author: Alexandros Lamprineas (labrinea)

Changes

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).


Patch is 43.29 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/95056.diff

11 Files Affected:

  • (modified) clang/include/clang/AST/ASTContext.h (+3)
  • (modified) clang/lib/AST/ASTContext.cpp (+27-32)
  • (modified) clang/lib/Basic/Targets/AArch64.cpp (+72-33)
  • (modified) clang/lib/Basic/Targets/AArch64.h (+4)
  • (modified) clang/test/CodeGen/aarch64-cpu-supports-target.c (+2-2)
  • (modified) clang/test/CodeGen/aarch64-sme-intrinsics/aarch64-sme-attrs.cpp (+1-1)
  • (modified) clang/test/CodeGen/aarch64-targetattr.c (+28-20)
  • (modified) clang/test/CodeGen/attr-target-version.c (+23-23)
  • (modified) clang/test/Sema/aarch64-neon-target.c (+2-2)
  • (modified) llvm/include/llvm/TargetParser/AArch64TargetParser.h (+42-65)
  • (modified) llvm/lib/TargetParser/AArch64TargetParser.cpp (+18-33)
diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h
index 8bce4812f0d48..a1d1d1c51cd41 100644
--- a/clang/include/clang/AST/ASTContext.h
+++ b/clang/include/clang/AST/ASTContext.h
@@ -3203,6 +3203,9 @@ 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 cd76b8aa271da..bf74e56a14799 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -87,7 +87,6 @@
 #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>
@@ -13664,20 +13663,17 @@ QualType ASTContext::getCorrespondingSignedFixedPointType(QualType Ty) const {
   }
 }
 
-// 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;
+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;
 }
 
 ParsedTargetAttr
@@ -13712,12 +13708,10 @@ 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.
-    // 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());
+    ParsedAttr.Features.insert(
+        ParsedAttr.Features.begin(),
+        Target->getTargetOpts().FeaturesAsWritten.begin(),
+        Target->getTargetOpts().FeaturesAsWritten.end());
 
     if (ParsedAttr.CPU != "" && Target->isValidCPUName(ParsedAttr.CPU))
       TargetCPU = ParsedAttr.CPU;
@@ -13738,31 +13732,32 @@ 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());
-      std::vector<std::string> Features = getFMVBackendFeaturesFor(Feats);
+      for (StringRef Feat : Feats)
+        if (Target->validateCpuSupports(Feat.str()))
+          // Use '?' to mark features that came from AArch64 TargetClones.
+          Features.push_back("?" + Feat.str());
       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);
     }
-  } else if (const auto *TV = FD->getAttr<TargetVersionAttr>()) {
-    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 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);
   } else {
     FeatureMap = Target->getTargetOpts().FeatureMap;
   }
diff --git a/clang/lib/Basic/Targets/AArch64.cpp b/clang/lib/Basic/Targets/AArch64.cpp
index 6fba5fff7bcc1..6afe1e0b0da5e 100644
--- a/clang/lib/Basic/Targets/AArch64.cpp
+++ b/clang/lib/Basic/Targets/AArch64.cpp
@@ -1052,18 +1052,57 @@ 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. 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")
@@ -1073,26 +1112,23 @@ ParsedTargetAttr AArch64TargetInfo::parseTargetAttr(StringRef Features) const {
   bool FoundArch = false;
 
   auto SplitAndAddFeatures = [](StringRef FeatString,
-                                std::vector<std::string> &Features,
-                                llvm::AArch64::ExtensionSet &FeatureBits) {
+                                std::vector<std::string> &Features) {
     SmallVector<StringRef, 8> SplitFeatures;
     FeatString.split(SplitFeatures, StringRef("+"), -1, false);
     for (StringRef Feature : SplitFeatures) {
-      if (FeatureBits.parseModifier(Feature, /* AllowNoDashForm = */ true))
-        continue;
-      // 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());
+      StringRef FeatureName = llvm::AArch64::getArchExtFeature(Feature);
+      if (!FeatureName.empty())
+        Features.push_back(FeatureName.str());
       else
-        Features.push_back("+" + Feature.str());
+        // 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());
     }
   };
 
-  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="))
@@ -1115,9 +1151,9 @@ ParsedTargetAttr AArch64TargetInfo::parseTargetAttr(StringRef Features) const {
       // Ret.Features.
       if (!AI)
         continue;
-      FeatureBits.addArchDefaults(*AI);
+      Ret.Features.push_back(AI->ArchFeature.str());
       // Add any extra features, after the +
-      SplitAndAddFeatures(Split.second, Ret.Features, FeatureBits);
+      SplitAndAddFeatures(Split.second, Ret.Features);
     } else if (Feature.starts_with("cpu=")) {
       if (!Ret.CPU.empty())
         Ret.Duplicate = "cpu=";
@@ -1127,10 +1163,7 @@ ParsedTargetAttr AArch64TargetInfo::parseTargetAttr(StringRef Features) const {
         std::pair<StringRef, StringRef> Split =
             Feature.split("=").second.trim().split("+");
         Ret.CPU = Split.first;
-        if (auto CpuInfo = llvm::AArch64::parseCpu(Ret.CPU)) {
-          FeatureBits.addCPUDefaults(*CpuInfo);
-          SplitAndAddFeatures(Split.second, Ret.Features, FeatureBits);
-        }
+        SplitAndAddFeatures(Split.second, Ret.Features);
       }
     } else if (Feature.starts_with("tune=")) {
       if (!Ret.Tune.empty())
@@ -1138,19 +1171,25 @@ ParsedTargetAttr AArch64TargetInfo::parseTargetAttr(StringRef Features) const {
       else
         Ret.Tune = Feature.split("=").second.trim();
     } else if (Feature.starts_with("+")) {
-      SplitAndAddFeatures(Feature, Ret.Features, FeatureBits);
+      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());
     } else {
-      if (FeatureBits.parseModifier(Feature, /* AllowNoDashForm = */ true))
-        continue;
-      // 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());
+      // 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());
       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 696553ef8038a..12fb50286f751 100644
--- a/clang/lib/Basic/Targets/AArch64.h
+++ b/clang/lib/Basic/Targets/AArch64.h
@@ -107,6 +107,10 @@ 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 28187bcf74533..e023944b24e53 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"="+fp-armv8,+neon" }
-// CHECK: attributes #2 = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="+fp-armv8,+fullfp16,+sve" }
+// 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" }
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 9885ac45e6a0e..af8933d93d6cb 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 -target-feature +bf16 \
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sme \
 // 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 644e6a692c3be..3e7a209245607 100644
--- a/clang/test/CodeGen/aarch64-targetattr.c
+++ b/clang/test/CodeGen/aarch64-targetattr.c
@@ -58,50 +58,58 @@ void v1msve() {}
 // CHECK-LABEL: @plussve() #12
 __attribute__((target("+sve")))
 void plussve() {}
-// CHECK-LABEL: @plussveplussve2() #12
+// CHECK-LABEL: @plussveplussve2() #13
 __attribute__((target("+sve+nosve2")))
 void plussveplussve2() {}
-// CHECK-LABEL: @plussveminusnosve2() #12
+// CHECK-LABEL: @plussveminusnosve2() #13
 __attribute__((target("sve,no-sve2")))
 void plussveminusnosve2() {}
-// CHECK-LABEL: @plusfp16() #13
+// CHECK-LABEL: @plusfp16() #14
 __attribute__((target("+fp16")))
 void plusfp16() {}
 
-// CHECK-LABEL: @all() #14
+// CHECK-LABEL: @all() #15
 __attribute__((target("cpu=neoverse-n1,tune=cortex-a710,arch=armv8.6-a+sve2")))
 void all() {}
-// CHECK-LABEL: @allplusbranchprotection() #15
+// CHECK-LABEL: @allplusbranchprotection() #16
 __attribute__((target("cpu=neoverse-n1,tune=cortex-a710,arch=armv8.6-a+sve2,branch-protection=standard")))
 void allplusbranchprotection() {}
 
-// CHECK-LABEL: @plusnosimd() #16
+// These tests check that the user facing and internal llvm name are both accepted.
+// CHECK-LABEL: @plusnoneon() #17
+__attribute__((target("+noneon")))
+void plusnoneon() {}
+// CHECK-LABEL: @plusnosimd() #17
 __attribute__((target("+nosimd")))
 void plusnosimd() {}
-// CHECK-LABEL: @nosimd() #16
+// CHECK-LABEL: @noneon() #17
+__attribute__((target("no-neon")))
+void noneon() {}
+// CHECK-LABEL: @nosimd() #17
 __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() #17
+// CHECK-LABEL: @minusarch() #18
 __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,+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 #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 #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,+v8.1a,+v8.2a,+v8a" "tune-cpu"="cortex-a710" }
-// CHECK: attributes #9 = { {{.*}} "target-features"="+fp-armv8,+fullfp16,+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,+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,+sve" }
-// CHECK: attributes #13 = { {{.*}} "target-features"="+fp-armv8,+fullfp16" }
-// 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.3a,+v8.4a,+v8.5a,+v8.6a,+v8a" "tune-cpu"="cortex-a710" }
-// CHECK: attributes #15 = { {{.*}} "branch-target-enforcement"="true" "guarded-control-stac...
[truncated]

Copy link

github-actions bot commented Jun 10, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@labrinea labrinea force-pushed the revert-decouple-feature-dependency-expansion branch from 6f4900f to dc660f1 Compare June 10, 2024 22:46
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 labrinea force-pushed the revert-decouple-feature-dependency-expansion branch from dc660f1 to 8d58882 Compare June 10, 2024 22:48
@labrinea labrinea merged commit 48aebd4 into llvm:main Jun 10, 2024
5 of 6 checks passed
@labrinea labrinea deleted the revert-decouple-feature-dependency-expansion branch June 10, 2024 23:52
labrinea added a commit to labrinea/llvm-project that referenced this pull request Jun 11, 2024
When we moved the extension information into tablegen in llvm#90987, some
features (FEAT_DPB, FEAT_DPB2, FEAT_FLAGM2, FEAT_FRINTTS, FEAT_RCPC2)
were defined as FMVOnlyExtension despite already having an equivalent
SubtargetFeature in place. This patch is fusing these duplications.

Moreover 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. FEAT_PMULL and FEAT_SSBS2 are
such examples. I have rectified those to reland the mentioned patch.
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.
@HerrCai0907 HerrCai0907 mentioned this pull request Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:AArch64 clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants