[clang][aarch64] Generate preprocessor macros for -march=armv8.6a+sve.

[fpetrogalli]

Summary

The following preprocessor macros are implied when -march=armv8.6a+sve:

__ARM_FEATURE_SVE 1
__ARM_FEATURE_SVE_BF16 1
__ARM_FEATURE_SVE_MATMUL_FP32 1
__ARM_FEATURE_SVE_MATMUL_INT8 1

[sdesmalen]

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

118

Looking at what Clang emits for e.g. -march=armv8.5-a, it just adds a target-feature +v8.5a. The definitions in llvm/lib/Target/AArch64/AArch64.td. suggests that LLVM is already able to infer all supported features from that. e.g.

def HasV8_4aOps : SubtargetFeature<
   :
   :

def HasV8_5aOps : SubtargetFeature<
  "v8.5a", "HasV8_5aOps", "true", "Support ARM v8.5a instructions",
  [HasV8_4aOps, FeatureAltFPCmp, FeatureFRInt3264, FeatureSpecRestrict,
   FeatureSSBS, FeatureSB, FeaturePredRes, FeatureCacheDeepPersist,
   FeatureBranchTargetId]>;

def HasV8_6aOps : SubtargetFeature<
  "v8.6a", "HasV8_6aOps", "true", "Support ARM v8.6a instructions",

  [HasV8_5aOps, FeatureAMVS, FeatureBF16, FeatureFineGrainedTraps,
   FeatureEnhancedCounterVirtualization, FeatureMatMulInt8]>;

So I don't think you necessarily have to decompose the architecture version into target-features in the Clang driver as well. For Clang it matters that the right set of feature macros are defined so that the ACLE header file exposes the correct set of functions for the given architecture version. At least for the SVE ACLE that is just a small handful of features.

[SjoerdMeijer]

I haven't looked into details of these arch extensions yet, will do that tomorrow, but I don't think there's any disagreement, i.e., these options and their behaviour are synchronised with the GCC community. Thus, it's better if you remove this wording from both description and comments in the source code. If there is divergence in behaviour, then that is probably be an oversight somewhere. I am also pretty sure that this 8.5 stuff is implemented, not sure if that is upstreamed. And also, I think we need to be more specific than "Default target features implied by -march on AArch64", because this is a big topic, which is not addressed in this patch. This patch is only about a few architecture extension. I am also for example surprised to see bfloat there, as I saw quite some activity in this area.

[fpetrogalli]

In D83079#2129371, @SjoerdMeijer wrote:

I haven't looked into details of these arch extensions yet, will do that tomorrow, but I don't think there's any disagreement, i.e., these options and their behaviour are synchronised with the GCC community. Thus, it's better if you remove this wording from both description and comments in the source code. If there is divergence in behaviour, then that is probably be an oversight somewhere. I am also pretty sure that this 8.5 stuff is implemented, not sure if that is upstreamed. And also, I think we need to be more specific than "Default target features implied by -march on AArch64", because this is a big topic, which is not addressed in this patch. This patch is only about a few architecture extension. I am also for example surprised to see bfloat there, as I saw quite some activity in this area.

I @SjoerdMeijer - I think I misinterpreted the way the target feature are generated, and which part of the codebase handles that. I wasn't aware about the common code pointed out by @sdesmalen that was translating architecture version into target feature - I thought everything needed to be translated by the driver into a list of -target-feature, that is why I thought there was discrepancy. If there is any, this patch is probably not the right way to fix it. The best thing to do is abandon this patch.

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

118

Thank you for explaining this @sdesmalen, I thought that the features needed to get explicitly generated by clang.
I will create a separate patch that adds only the feature macros needed for the SVE ACLEs.

[fpetrogalli]

Update the patch to limit its scope to generate the feature macros for -march=armv8.6a+sve.

[sdesmalen]

LGTM

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

415

nit: unrelated change.

[fpetrogalli]

Removed the unrelated change of the empty line.

[SjoerdMeijer]

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

118

Would it be more consistent to move this....

273

...to somewhere here where implied target features are handled....

373

For example, to here.

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

115

Can you add a run line for v8.5 if there isn't already one, and add CHECK-NOTs for these macros.

[fpetrogalli]

Addressed code review, moving the code and adding more testing, including the v8.5 one.

[sdesmalen]

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

375

Is this correct and/or necessary? I would expect LLVM to just handle features in the order they're passed, and the architecture version is always processed first, e.g. -march=armv8.6-a+noi8mm will always first process armv8.6a before processing features like noi8mm.

[fpetrogalli]

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

375

I was expecting that too, but in in this place the +i8mm is added after whatever the user have passed to -march, which means that without this extra check the user input -mattr=armv8.6a+sve+noimm8 becomes broken because we are adding -target-feature=+i8mm after -i8mm. This behavior is guarded by a regression tests that starts failing if I don't use these extra checks. This was not needed in the original place were I added the functionality because the +i8mm was being added right after +v8.6a and before splitting up the +sve+noi8mm, so that the user input was the one (un)setting the feature.

[sdesmalen]

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

375

As you said, we end up with a Feature list as follows:

parsing(-march=armv8.6-a+noi8mm)
=> Features = [ v8.6a ]

parsing(+noi8mm)
=> Features = [ v8.6a, -i8mm ]

Then going through the feature list again:
=> Features = [ v8.6a, -i8mm, +i8mm ]
                ^^^^^          ^^^^
                  \_____________/
                     adds +i8mm

To fix that, you can insert these features into the list straight after "+v8.6a", instead of appending at the end of the Features list. Either that, or calling llvm::AArch64::getDefaultExtensions() + llvm::AArch64::getExtensionFeatures() in getAArch64ArchFeaturesFromMarch like is done in DecodeAArch64Mcpu, which should do all this for free. That seems like a more invasive change though that you shouldn't try to do in this patch.

[fpetrogalli]

@sdesmalen, I have followed your suggestion to use insert instead of push_back!

Code is much nicer now, thanks!

[sdesmalen]

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

376

Both +i8mm and +bf16 should be added at iterator V8_6Pos, because I believe that std::next(V8_6Pos) inserts it after the item that follows v8.6a, rather than after v8.6a directly.

[sdesmalen]

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

376

Never mind the above comment, you pointed out offline that std::next is correct because insert inserts the value before V8_6Pos, my bad!

Looking at the documentation for std::vector::insert, it seems you can write:

if (V8_6Pos != std::end(Features))
  Features.insert(std::next(V8_6Pos), {"+bf16", "+i8mm"});

which avoids having to update V8_6Pos.

[fpetrogalli]

Update the insert invocation to use initializer list instead of calling it twice.

[sdesmalen]

LGTM again!

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

163

nit: I don't think you really need to test all permutations, one positive test (for -march=armv8.6-a+sve) and this negative test should be sufficient?

[fpetrogalli]

Removed extra tests that are not needed.

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

 static bool
 getAArch64ArchFeaturesFromMarch(const Driver &D, StringRef March,
                                 const ArgList &Args,
                                 std::vector<StringRef> &Features) {
   std::string MarchLowerCase = March.lower();
   std::pair<StringRef, StringRef> Split = StringRef(MarchLowerCase).split("+");
 
   llvm::AArch64::ArchKind ArchKind = llvm::AArch64::parseArch(Split.first);
-  if (ArchKind == llvm::AArch64::ArchKind::INVALID ||
-      !llvm::AArch64::getArchFeatures(ArchKind, Features) ||
-      (Split.second.size() &&
-       !DecodeAArch64Features(D, Split.second, Features, ArchKind)))
+
+  if (!llvm::AArch64::getArchFeatures(ArchKind, Features))
+    return false;
+
+  if (ArchKind == llvm::AArch64::ArchKind::ARMV8_6A) {
+    Features.push_back("+i8mm");
+    Features.push_back("+bf16");
+  }
+
+  if (!DecodeAArch64Features(D, Split.second, Features, ArchKind))
     return false;
 
   return true;
 }
 
 static bool
 getAArch64ArchFeaturesFromMcpu(const Driver &D, StringRef Mcpu,
                                const ArgList &Args,
else
       Features.push_back("-crc");
   }
 
   // Handle (arch-dependent) fp16fml/fullfp16 relationship.
   // FIXME: this fp16fml option handling will be reimplemented after the
   // TargetParser rewrite.
   const auto ItRNoFullFP16 = std::find(Features.rbegin(), Features.rend(), "-fullfp16");
   const auto ItRFP16FML = std::find(Features.rbegin(), Features.rend(), "+fp16fml");
   if (llvm::is_contained(Features, "+v8.4a")) {
     const auto ItRFullFP16  = std::find(Features.rbegin(), Features.rend(), "+fullfp16");
     if (ItRFullFP16 < ItRNoFullFP16 && ItRFullFP16 < ItRFP16FML) {
       // Only entangled feature that can be to the right of this +fullfp16 is -fp16fml.
       // Only append the +fp16fml if there is no -fp16fml after the +fullfp16.
       if (std::find(Features.rbegin(), ItRFullFP16, "-fp16fml") == ItRFullFP16)
         Features.push_back("+fp16fml");
     }
     else
if (HasCrypto && !NoCrypto) {
       if (!HasAES)
         Features.push_back("-aes");
       if (HasV82a || HasV83a || HasV84a) {
         Features.push_back("-sm4");
         Features.push_back("-sha3");
       }
     }
   }
 
   if (Arg *A = Args.getLastArg(options::OPT_mno_unaligned_access,
                                options::OPT_munaligned_access))
     if (A->getOption().matches(options::OPT_mno_unaligned_access))
       Features.push_back("+strict-align");
 
   if (Args.hasArg(options::OPT_ffixed_x1))
     Features.push_back("+reserve-x1");
 
   if (Args.hasArg(options::OPT_ffixed_x2))
     Features.push_back("+reserve-x2");
 

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

 // RUN: %clang -target aarch64-none-linux-gnu -march=armv8-a+sve+f32mm -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-SVE-F32MM %s
 // CHECK-SVE-F32MM: __ARM_FEATURE_SVE 1
 // CHECK-SVE-F32MM: __ARM_FEATURE_SVE_MATMUL_FP32 1
 
 // RUN: %clang -target aarch64-none-linux-gnu -march=armv8-a+sve+f64mm -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-SVE-F64MM %s
 // CHECK-SVE-F64MM: __ARM_FEATURE_SVE 1
 // CHECK-SVE-F64MM: __ARM_FEATURE_SVE_MATMUL_FP64 1
 
+// RUN: %clang -target aarch64-none-linux-gnu -march=armv8.6-a+sve -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-SVE-8_6 %s
+// CHECK-SVE-8_6: __ARM_FEATURE_SVE 1
+// CHECK-SVE-8_6: __ARM_FEATURE_SVE_BF16 1
+// CHECK-SVE-8_6: __ARM_FEATURE_SVE_MATMUL_FP32 1
+// CHECK-SVE-8_6: __ARM_FEATURE_SVE_MATMUL_INT8 1
+
 // The following tests may need to be revised in the future since
 // SVE2 is currently still part of Future Architecture Technologies
 // (https://developer.arm.com/docs/ddi0602/latest)
 //
 // RUN: %clang -target aarch64-none-linux-gnu -march=armv8-a+sve2 -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-SVE2 %s
 // CHECK-SVE2: __ARM_FEATURE_SVE2 1
 // RUN: %clang -target aarch64-none-linux-gnu -march=armv8-a+sve2-aes -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-SVE2AES %s
 // CHECK-SVE2AES: __ARM_FEATURE_SVE2_AES 1

 // 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-none-linux-gnueabi -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-none-linux-gnueabi -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-none-linux-gnueabi -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-none-linux-gnueabi -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_FP16FML 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-none-linux-gnueabi -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-none-linux-gnueabi -march=armv8.2-a+nofp16 -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-FULLFP16-NOFML-VECTOR-SCALAR %s

 // CHECK-BTI-OFF-NOT: __ARM_FEATURE_BTI_DEFAULT
 // CHECK-BTI:         #define __ARM_FEATURE_BTI_DEFAULT 1
 
 // ================== Check BFloat16 Extensions.
 // RUN: %clang -target aarch64-arm-none-eabi -march=armv8.6-a+bf16 -x c -E -dM %s -o - 2>&1 | FileCheck -check-prefix=CHECK-BFLOAT %s
 // CHECK-BFLOAT: __ARM_BF16_FORMAT_ALTERNATIVE 1
 // CHECK-BFLOAT: __ARM_FEATURE_BF16 1
 // CHECK-BFLOAT: __ARM_FEATURE_BF16_VECTOR_ARITHMETIC 1