[AArch64] Simplify Clang's description of architecture extensions #79311

atrosinenko · 2024-01-24T15:49:16Z

Core LLVM has AArch64TargetParser.h header describing the mapping from Armv8.x and Armv9.x architecture extensions to the particular list of features that are mandatory for the extension.

Clang partially reimplements this in AArch64TargetInfo::setArchFeatures() function. This patch simplifies setArchFeatures() by dropping obvious cases of duplication, though it seems not all the dependencies are listed in AArch64TargetParser.h yet.

Specifically, this patch drops HasX = true statements for the following features:

v8.1-a: CRC, LSE, RDM
v8.3-a: RCPC, NeonMode (it is enabled by the base Armv8-a and indirectly by -target-feature +fcma)
v8.4-a: DOTPROD
v8.6-a: BF16, MATMUL

In Clang's AArch64TargetInfo::handleTargetFeatures() function, the string literal "+jscvt" is changed to "+jsconv" and "+cccp" to "+ccpp", so the information is conveyed from TargetParser.

Additionally, this patch makes __ARM_FEATURE_BTI and __ARM_FEATURE_JCVT conditional on the particular HasX flags instead of an architecture extension as a whole, as using TargetParser makes it possible to specify the requested architecture like "armv8.5+nojscvt".

Core LLVM has AArch64TargetParser.h header describing the mapping from Armv8.x and Armv9.x architecture extensions to the particular list of features that are mandatory for the extension. Clang partially reimplements this in AArch64TargetInfo::setArchFeatures() function. This patch simplifies setArchFeatures() by dropping obvious cases of duplication, though it seems not all the dependencies are listed in AArch64TargetParser.h yet. Specifically, this patch drops `HasX = true` statements for the following features: * v8.1-a: CRC, LSE, RDM * v8.3-a: RCPC, NeonMode (it is enabled by the base Armv8-a and indirectly by -target-feature +fcma) * v8.4-a: DOTPROD * v8.6-a: BF16, MATMUL In Clang's AArch64TargetInfo::handleTargetFeatures() function, the string literal "+jscvt" is changed to "+jsconv" and "+cccp" to "+ccpp", so the information is conveyed from TargetParser. Additionally, this patch makes __ARM_FEATURE_BTI and __ARM_FEATURE_JCVT conditional on the particular HasX flags instead of an architecture extension as a whole, as using TargetParser makes it possible to specify the requested architecture like "armv8.5+nojscvt".

llvmbot · 2024-01-25T10:12:36Z

@llvm/pr-subscribers-backend-aarch64

@llvm/pr-subscribers-clang

Author: Anatoly Trosinenko (atrosinenko)

Changes

Core LLVM has AArch64TargetParser.h header describing the mapping from Armv8.x and Armv9.x architecture extensions to the particular list of features that are mandatory for the extension.

Clang partially reimplements this in AArch64TargetInfo::setArchFeatures() function. This patch simplifies setArchFeatures() by dropping obvious cases of duplication, though it seems not all the dependencies are listed in AArch64TargetParser.h yet.

Specifically, this patch drops HasX = true statements for the following features:

v8.1-a: CRC, LSE, RDM
v8.3-a: RCPC, NeonMode (it is enabled by the base Armv8-a and indirectly by -target-feature +fcma)
v8.4-a: DOTPROD
v8.6-a: BF16, MATMUL

In Clang's AArch64TargetInfo::handleTargetFeatures() function, the string literal "+jscvt" is changed to "+jsconv" and "+cccp" to "+ccpp", so the information is conveyed from TargetParser.

Additionally, this patch makes __ARM_FEATURE_BTI and __ARM_FEATURE_JCVT conditional on the particular HasX flags instead of an architecture extension as a whole, as using TargetParser makes it possible to specify the requested architecture like "armv8.5+nojscvt".

Full diff: https://github.com/llvm/llvm-project/pull/79311.diff

1 Files Affected:

(modified) clang/lib/Basic/Targets/AArch64.cpp (+15-34)

diff --git a/clang/lib/Basic/Targets/AArch64.cpp b/clang/lib/Basic/Targets/AArch64.cpp
index d47181bfca4fc8..76bb60db41099e 100644
--- a/clang/lib/Basic/Targets/AArch64.cpp
+++ b/clang/lib/Basic/Targets/AArch64.cpp
@@ -55,24 +55,18 @@ static constexpr Builtin::Info BuiltinInfo[] = {
 };
 
 void AArch64TargetInfo::setArchFeatures() {
+  // FIXME Could we drop this function altogether and migrate everything
+  //       to AArch64TargetParser in LLVM core?
   if (*ArchInfo == llvm::AArch64::ARMV8R) {
-    HasDotProd = true;
     HasDIT = true;
     HasFlagM = true;
-    HasRCPC = true;
-    FPU |= NeonMode;
     HasCCPP = true;
-    HasCRC = true;
     HasLSE = true;
-    HasRDM = true;
   } else if (ArchInfo->Version.getMajor() == 8) {
     if (ArchInfo->Version.getMinor() >= 7u) {
       HasWFxT = true;
     }
-    if (ArchInfo->Version.getMinor() >= 6u) {
-      HasBFloat16 = true;
-      HasMatMul = true;
-    }
+    // No special cases for Armv8.6-a
     if (ArchInfo->Version.getMinor() >= 5u) {
       HasAlternativeNZCV = true;
       HasFRInt3264 = true;
@@ -82,30 +76,19 @@ void AArch64TargetInfo::setArchFeatures() {
       HasBTI = true;
     }
     if (ArchInfo->Version.getMinor() >= 4u) {
-      HasDotProd = true;
       HasDIT = true;
       HasFlagM = true;
     }
-    if (ArchInfo->Version.getMinor() >= 3u) {
-      HasRCPC = true;
-      FPU |= NeonMode;
-    }
+    // No special cases for Armv8.3-a
     if (ArchInfo->Version.getMinor() >= 2u) {
       HasCCPP = true;
     }
-    if (ArchInfo->Version.getMinor() >= 1u) {
-      HasCRC = true;
-      HasLSE = true;
-      HasRDM = true;
-    }
+    // No special cases for Armv8.1-a
   } else if (ArchInfo->Version.getMajor() == 9) {
     if (ArchInfo->Version.getMinor() >= 2u) {
       HasWFxT = true;
     }
-    if (ArchInfo->Version.getMinor() >= 1u) {
-      HasBFloat16 = true;
-      HasMatMul = true;
-    }
+    // No special cases for Armv9.1-a
     FPU |= SveMode;
     HasSVE2 = true;
     HasFullFP16 = true;
@@ -115,15 +98,9 @@ void AArch64TargetInfo::setArchFeatures() {
     HasSB = true;
     HasPredRes = true;
     HasBTI = true;
-    HasDotProd = true;
     HasDIT = true;
     HasFlagM = true;
-    HasRCPC = true;
-    FPU |= NeonMode;
     HasCCPP = true;
-    HasCRC = true;
-    HasLSE = true;
-    HasRDM = true;
   }
 }
 
@@ -257,7 +234,6 @@ void AArch64TargetInfo::getTargetDefinesARMV82A(const LangOptions &Opts,
 void AArch64TargetInfo::getTargetDefinesARMV83A(const LangOptions &Opts,
                                                 MacroBuilder &Builder) const {
   Builder.defineMacro("__ARM_FEATURE_COMPLEX", "1");
-  Builder.defineMacro("__ARM_FEATURE_JCVT", "1");
   Builder.defineMacro("__ARM_FEATURE_PAUTH", "1");
   // Also include the Armv8.2 defines
   getTargetDefinesARMV82A(Opts, Builder);
@@ -272,7 +248,6 @@ void AArch64TargetInfo::getTargetDefinesARMV84A(const LangOptions &Opts,
 void AArch64TargetInfo::getTargetDefinesARMV85A(const LangOptions &Opts,
                                                 MacroBuilder &Builder) const {
   Builder.defineMacro("__ARM_FEATURE_FRINT", "1");
-  Builder.defineMacro("__ARM_FEATURE_BTI", "1");
   // Also include the Armv8.4 defines
   getTargetDefinesARMV84A(Opts, Builder);
 }
@@ -472,7 +447,10 @@ void AArch64TargetInfo::getTargetDefines(const LangOptions &Opts,
   if ((FPU & NeonMode) && HasFullFP16)
     Builder.defineMacro("__ARM_FEATURE_FP16_VECTOR_ARITHMETIC", "1");
   if (HasFullFP16)
-   Builder.defineMacro("__ARM_FEATURE_FP16_SCALAR_ARITHMETIC", "1");
+    Builder.defineMacro("__ARM_FEATURE_FP16_SCALAR_ARITHMETIC", "1");
+
+  if (HasJSCVT)
+    Builder.defineMacro("__ARM_FEATURE_JCVT", "1");
 
   if (HasDotProd)
     Builder.defineMacro("__ARM_FEATURE_DOTPROD", "1");
@@ -530,6 +508,9 @@ void AArch64TargetInfo::getTargetDefines(const LangOptions &Opts,
     Builder.defineMacro("__ARM_FEATURE_PAC_DEFAULT", std::to_string(Value));
   }
 
+  if (HasBTI)
+    Builder.defineMacro("__ARM_FEATURE_BTI", "1");
+
   if (Opts.BranchTargetEnforcement)
     Builder.defineMacro("__ARM_FEATURE_BTI_DEFAULT", "1");
 
@@ -741,7 +722,7 @@ bool AArch64TargetInfo::handleTargetFeatures(std::vector<std::string> &Features,
 
     if (Feature == "+neon" || Feature == "+fp-armv8")
       FPU |= NeonMode;
-    if (Feature == "+jscvt") {
+    if (Feature == "+jsconv") {
       HasJSCVT = true;
       FPU |= NeonMode;
     }
@@ -860,7 +841,7 @@ bool AArch64TargetInfo::handleTargetFeatures(std::vector<std::string> &Features,
     }
     if (Feature == "+dit")
       HasDIT = true;
-    if (Feature == "+cccp")
+    if (Feature == "+ccpp")
       HasCCPP = true;
     if (Feature == "+ccdp") {
       HasCCPP = true;

clang/lib/Basic/Targets/AArch64.cpp

ostannard · 2024-01-25T11:38:56Z

clang/lib/Basic/Targets/AArch64.cpp

@@ -860,7 +841,7 @@ bool AArch64TargetInfo::handleTargetFeatures(std::vector<std::string> &Features,
    }
    if (Feature == "+dit")
      HasDIT = true;
-    if (Feature == "+cccp")
+    if (Feature == "+ccpp")


Again, does this need a test?

Agree, but not yet sure how it is better to test this

atrosinenko · 2024-01-25T18:29:37Z

I tried adding a few more lines to clang/test/Preprocessor/aarch64-target-features.c and turned out, clang accepts -march=armv8-a+jscvt but not -march=armv8-a+dpb. Brief debugging suggests this is because in the

  {"dpb", AArch64::AEK_NONE, {}, {}, FEAT_DPB, "+ccpp", 190},

line, Feature and NegFeature fields are empty.

I wonder if there is an overview of all the "feature-related" AArch64 stuff anywhere. As far as I understand, there are several kinds of identifiers:

feature names defined in the chapter "A2: Armv8-A Architecture Extensions" of the Arm Architecture Reference Manual (such as FEAT_JSCVT mentioned in "A2.5: The Armv8.3 architecture extension")
Function Multi-Versioning CPU features defined in enum CPUFeatures (such as FEAT_JSCVT)
AEK_* constants defined in enum ArchExtKind (such as AEK_JSCVT)
Names defined in the Extensions array in AArch64TargetParser.h
Feature / NegFeature defined in the same array
DependentFeatures defined in the same array

I guess, each kind of identifiers belongs to one of logical "namespaces". Here is what I see:

the names of FMV features looks identical to feature names defined in Arm ARM, but the sets of identifiers are different - the former is a strict subset of the latter at best. Obviously, there is a number of features hardly related to compiler development (such as various properties of hardware debug interfaces) and some other are not related to runtime function selection by FMV machinery (or at least the rules are not yet written in ACLE for them). It is just a bit misleading that Arm ARM defines FEAT_PAUTH and this feature is mentioned as AEK_PAUTH, "+pauth" literal, etc. but not as FEAT_PAUTH in the enumeration with constants named FEAT_* :)
after AEK_* constants were recently made not restricted by the bitsize of uint64_t anymore, these look like the best mirror of FEAT_* identifiers defined in Arm ARM (and there is an explicit naming guideline)
Name fields are used when parsing -march=...+feature arguments of Clang. These are kind of user-visible interface
Feature/NegFeature and DependentFeatures are used when parsing -target-features passed to clang -cc1 and are likely to contain string identifiers from the same namespace, according to seemingly the only in-tree user of DependentFeatures contents, but duplication like {"jscvt", AArch64::AEK_JSCVT, "+jsconv", "-jsconv", FEAT_JSCVT, "+fp-armv8,+neon,+jsconv", 210} (E.Feature additionally listed in E.DependentFeatures) looks redundant

atrosinenko · 2024-01-25T18:33:49Z

Meanwhile, which is current policy on enabling/disabling mandatory features via command line options?

atrosinenko requested review from asl, DavidSpickett, ostannard, momchil-velikov and sjoerdmeijer January 24, 2024 15:49

Drop setting NeonMode for Armv8-r case, similar to Armv8.3-a

59cf7d9

llvmbot added clang Clang issues not falling into any other category backend:AArch64 clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jan 25, 2024

ostannard reviewed Jan 25, 2024

View reviewed changes

Add a test for +nojscvt

49a922f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AArch64] Simplify Clang's description of architecture extensions #79311

[AArch64] Simplify Clang's description of architecture extensions #79311

atrosinenko commented Jan 24, 2024

llvmbot commented Jan 25, 2024 •

edited

ostannard Jan 25, 2024

atrosinenko Jan 25, 2024

atrosinenko commented Jan 25, 2024

atrosinenko commented Jan 25, 2024

[AArch64] Simplify Clang's description of architecture extensions #79311

Are you sure you want to change the base?

[AArch64] Simplify Clang's description of architecture extensions #79311

Conversation

atrosinenko commented Jan 24, 2024

llvmbot commented Jan 25, 2024 • edited

ostannard Jan 25, 2024

Choose a reason for hiding this comment

atrosinenko Jan 25, 2024

Choose a reason for hiding this comment

atrosinenko commented Jan 25, 2024

atrosinenko commented Jan 25, 2024

llvmbot commented Jan 25, 2024 •

edited