[AArch64] Default HBC/MOPS features in clang

[tyb0807]

Summary

This implements minimum support in clang for default HBC/MOPS features
on v8.8-a/v9.3-a or later architectures.

[nickdesaulniers]

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

463–482

Can we reuse ItBegin and ItEnd here rather than Features.begin() and Features.end()?

465–474

Is it possible to re-use some of the many calls to std::find (L395-403)? Seems like we re-scan the feature list A LOT.

[tyb0807]

Cache architecture feature to avoid scanning the feature list over and over again.

[tyb0807]

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

463–482

This is not relevant anymore, but for the initial code, I guess we can't reuse ItBegin/End because Features has been changed since then (a lot of push_back). The reason why we need to keep these default features (+hbc, +mops, ...) right after the architecture feature is that we want to be able to disable the default features X if +noX option is given in the command line, knowing that +noX features will come after the architecture feature in the Features list.

465–474

I cached the architecture feature when it is defined to avoid these calls to std::find.

[tmatheson]

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

265

Looks like you could do AF = Features.back() below all these if/else conditions (where it is first used) based on the value of success. Maybe there is no need to pass it into getAArch64MicroArchFeaturesFromMcpu and the other functions?

Adding an extra output parameter to each of these functions (which duplicates an existing output parameter) seems unnecessary and makes the interfaces more complicated.

[tyb0807]

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

265

we can't do AF = Features.back() below all the if/else conditions because getAArch64MicroArchFeaturesFromM* adds (usually) the architecture feature first, followed by a bunch of other features, so when it returns, Features.back() is not the architecture feature anymore.

I see your point about complicating the interfaces, maybe @nickdesaulniers can help us here?

[tyb0807]

Taking into account remarks from @tmatheson, I'm reverting my latest changes
consisting in caching architecture feature into a variable, which makes the
getAArch64ArchFeaturesFrom* interfaces more complicated.

I'd propose we move forward with this patch, and the performance issue raised by
@nickdesaulniers will be addressed properly in a separate one.

[tmatheson]

LGTM, please give @nickdesaulniers some time to respond though. I do agree that iterating over the features repeatedly is less than ideal, but also that this patch is probably not the place to try to fix it.

[nickdesaulniers]

In D120111#3351001, @tmatheson wrote:

LGTM, please give @nickdesaulniers some time to respond though. I do agree that iterating over the features repeatedly is less than ideal, but also that this patch is probably not the place to try to fix it.

I'm not exactly thrilled about continuing to kick the can down the road on this; consider making a child revision that fixes that first, then landing this patch. Otherwise that never gets cleaned up.

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

468–480

Is Pos ever read later, or simply compared to std::end(Features)? If not, consider simplifying removing Pos outright and simply calling std::find_first_of in an if predicate.

clang/lib/Basic/Targets/AArch64.cpp

 void AArch64TargetInfo::getTargetDefinesARMV87A(const LangOptions &Opts,
                                                 MacroBuilder &Builder) const {
   // Also include the Armv8.6 defines
   getTargetDefinesARMV86A(Opts, Builder);
 }
 
 void AArch64TargetInfo::getTargetDefinesARMV88A(const LangOptions &Opts,
                                                 MacroBuilder &Builder) const {
-  // FIXME: this does not handle the case where MOPS is disabled using +nomops
-  Builder.defineMacro("__ARM_FEATURE_MOPS", "1");
   // Also include the Armv8.7 defines
   getTargetDefinesARMV87A(Opts, Builder);
 }
 
 void AArch64TargetInfo::getTargetDefinesARMV9A(const LangOptions &Opts,
                                                MacroBuilder &Builder) const {
   // Armv9-A maps to Armv8.5-A
   getTargetDefinesARMV85A(Opts, Builder);

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

if (success && (A = Args.getLastArg(clang::driver::options::OPT_mtune_EQ)))
     success =
         getAArch64MicroArchFeaturesFromMtune(D, A->getValue(), Args, Features);
   else if (success && (A = Args.getLastArg(options::OPT_mcpu_EQ)))
     success =
         getAArch64MicroArchFeaturesFromMcpu(D, A->getValue(), Args, Features);
   else if (success &&
            (Args.hasArg(options::OPT_arch) || isCPUDeterminedByTriple(Triple)))
     success = getAArch64MicroArchFeaturesFromMcpu(
         D, getAArch64TargetCPU(Args, Triple, A), Args, Features);
 
   if (!success) {
     auto Diag = D.Diag(diag::err_drv_clang_unsupported);
     // If "-Wa,-march=" is used, 'WaMArch' will contain the argument's value,
     // while 'A' is uninitialized. Only dereference 'A' in the other case.
     if (!WaMArch.empty())
       Diag << "-march=" + WaMArch.str();
     else
if (HasCrypto && !NoCrypto) {
       if (!HasAES)
         Features.push_back("-aes");
       if (HasV82a || HasV83a || HasV84a) {
         Features.push_back("-sm4");
         Features.push_back("-sha3");
       }
     }
   }
 
-  const char *Archs[] = {"+v8.6a", "+v8.7a", "+v8.8a",
+  // FIXME: these insertions should ideally be automated using default
+  // extensions support from the backend target parser.
+  const char *v8691OrLater[] = {"+v8.6a", "+v8.7a", "+v8.8a",
                                 "+v9.1a", "+v9.2a", "+v9.3a"};
-  auto Pos = std::find_first_of(Features.begin(), Features.end(),
-                                std::begin(Archs), std::end(Archs));
+  auto Pos =
+      std::find_first_of(Features.begin(), Features.end(),
+                         std::begin(v8691OrLater), std::end(v8691OrLater));
   if (Pos != std::end(Features))
     Pos = Features.insert(std::next(Pos), {"+i8mm", "+bf16"});
 
+  // For Armv8.8-a/Armv9.3-a or later, FEAT_HBC and FEAT_MOPS are enabled by
+  // default.
+  const char *v8893OrLater[] = {"+v8.8a", "+v9.3a"};
+  Pos = std::find_first_of(Features.begin(), Features.end(),
+                           std::begin(v8893OrLater), std::end(v8893OrLater));
+  if (Pos != std::end(Features))
+    Pos = Features.insert(std::next(Pos), {"+hbc", "+mops"});
+
   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");
   } else if (Triple.isOSOpenBSD())
     Features.push_back("+strict-align");
 
   if (Args.hasArg(options::OPT_ffixed_x1))
     Features.push_back("+reserve-x1");

clang/test/Driver/aarch64-hbc.c

 // Test that target feature hbc is implemented and available correctly
-// RUN: %clang -### -target aarch64-none-none-eabi -march=armv8.8-a+hbc %s 2>&1 | FileCheck %s
-// CHECK: "-target-feature" "+hbc"
-
+// RUN: %clang -### -target aarch64-none-none-eabi -march=armv8.7-a+hbc   %s 2>&1 | FileCheck %s
+// RUN: %clang -### -target aarch64-none-none-eabi -march=armv8.8-a       %s 2>&1 | FileCheck %s
+// RUN: %clang -### -target aarch64-none-none-eabi -march=armv8.8-a+hbc   %s 2>&1 | FileCheck %s
 // RUN: %clang -### -target aarch64-none-none-eabi -march=armv8.8-a+nohbc %s 2>&1 | FileCheck %s --check-prefix=NO_HBC
+// RUN: %clang -### -target aarch64-none-none-eabi -march=armv9.2-a+hbc   %s 2>&1 | FileCheck %s
+// RUN: %clang -### -target aarch64-none-none-eabi -march=armv9.3-a       %s 2>&1 | FileCheck %s
+// RUN: %clang -### -target aarch64-none-none-eabi -march=armv9.3-a+hbc   %s 2>&1 | FileCheck %s
+// RUN: %clang -### -target aarch64-none-none-eabi -march=armv9.3-a+nohbc %s 2>&1 | FileCheck %s --check-prefix=NO_HBC
+
+// CHECK: "-target-feature" "+hbc"
 // NO_HBC: "-target-feature" "-hbc"

clang/test/Driver/aarch64-mops.c

 // Test that target feature mops is implemented and available correctly
-// RUN: %clang -### -target aarch64-none-none-eabi -march=armv8.8-a+mops %s 2>&1 | FileCheck %s
-// CHECK: "-target-feature" "+mops"
-
+// RUN: %clang -### -target aarch64-none-none-eabi -march=armv8.7-a+mops   %s 2>&1 | FileCheck %s
+// RUN: %clang -### -target aarch64-none-none-eabi -march=armv8.8-a        %s 2>&1 | FileCheck %s
+// RUN: %clang -### -target aarch64-none-none-eabi -march=armv8.8-a+mops   %s 2>&1 | FileCheck %s
 // RUN: %clang -### -target aarch64-none-none-eabi -march=armv8.8-a+nomops %s 2>&1 | FileCheck %s --check-prefix=NO_MOPS
+// RUN: %clang -### -target aarch64-none-none-eabi -march=armv9.2-a+mops   %s 2>&1 | FileCheck %s
+// RUN: %clang -### -target aarch64-none-none-eabi -march=armv9.3-a        %s 2>&1 | FileCheck %s
+// RUN: %clang -### -target aarch64-none-none-eabi -march=armv9.3-a+mops   %s 2>&1 | FileCheck %s
+// RUN: %clang -### -target aarch64-none-none-eabi -march=armv9.3-a+nomops %s 2>&1 | FileCheck %s --check-prefix=NO_MOPS
+
+// CHECK: "-target-feature" "+mops"
 // NO_MOPS: "-target-feature" "-mops"

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

 // ================== Check Large System Extensions (LSE)
 // RUN: %clang -target aarch64-none-linux-gnu -march=armv8-a+lse -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-LSE %s
 // RUN: %clang -target arm64-none-linux-gnu -march=armv8-a+lse -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-LSE %s
 // RUN: %clang -target aarch64-none-linux-gnu -march=armv8.1-a -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-LSE %s
 // RUN: %clang -target arm64-none-linux-gnu -march=armv8.1-a -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-LSE %s
 // CHECK-LSE: __ARM_FEATURE_ATOMICS 1
 
 // ================== Check Armv8.8-A/Armv9.3-A memcpy and memset acceleration instructions (MOPS)
-// RUN: %clang -target aarch64-arm-none-eabi -march=armv8.7-a      -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-NOMOPS %s
-// RUN: %clang -target aarch64-arm-none-eabi -march=armv8.7-a+mops -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-MOPS   %s
-// RUN: %clang -target aarch64-arm-none-eabi -march=armv8.8-a      -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-MOPS   %s
-// RUN: %clang -target aarch64-arm-none-eabi -march=armv8.8-a+mops -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-MOPS   %s
-// RUN: %clang -target aarch64-arm-none-eabi -march=armv9.2-a      -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-NOMOPS %s
-// RUN: %clang -target aarch64-arm-none-eabi -march=armv9.2-a+mops -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-MOPS   %s
-// RUN: %clang -target aarch64-arm-none-eabi -march=armv9.3-a      -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-MOPS   %s
-// RUN: %clang -target aarch64-arm-none-eabi -march=armv9.3-a+mops -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-MOPS   %s
+// RUN: %clang -target aarch64-arm-none-eabi -march=armv8.7-a             -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-NOMOPS %s
+// RUN: %clang -target aarch64-arm-none-eabi -march=armv8.7-a+mops        -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-MOPS   %s
+// RUN: %clang -target aarch64-arm-none-eabi -march=armv8.8-a             -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-MOPS   %s
+// RUN: %clang -target aarch64-arm-none-eabi -march=armv8.8-a+nomops      -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-NOMOPS %s
+// RUN: %clang -target aarch64-arm-none-eabi -march=armv8.8-a+nomops+mops -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-MOPS   %s
+// RUN: %clang -target aarch64-arm-none-eabi -march=armv8.8-a+mops        -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-MOPS   %s
+// RUN: %clang -target aarch64-arm-none-eabi -march=armv8.8-a+mops+nomops -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-NOMOPS %s
+// RUN: %clang -target aarch64-arm-none-eabi -march=armv9.2-a             -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-NOMOPS %s
+// RUN: %clang -target aarch64-arm-none-eabi -march=armv9.2-a+mops        -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-MOPS   %s
+// RUN: %clang -target aarch64-arm-none-eabi -march=armv9.3-a             -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-MOPS   %s
+// RUN: %clang -target aarch64-arm-none-eabi -march=armv9.3-a+nomops      -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-NOMOPS %s
+// RUN: %clang -target aarch64-arm-none-eabi -march=armv9.3-a+mops        -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-MOPS   %s
 // CHECK-MOPS: __ARM_FEATURE_MOPS 1
 // CHECK-NOMOPS-NOT: __ARM_FEATURE_MOPS 1
 
 // ================== Check default macros for Armv8.1-A and later
 // RUN: %clang -target aarch64-arm-none-eabi -march=armv8.1-a -x c -E -dM %s -o - | FileCheck --check-prefixes=CHECK-V81-OR-LATER,CHECK-BEFORE-V83,CHECK-BEFORE-V85     %s
 // RUN: %clang -target aarch64-arm-none-eabi -march=armv8.2-a -x c -E -dM %s -o - | FileCheck --check-prefixes=CHECK-V81-OR-LATER,CHECK-BEFORE-V83,CHECK-BEFORE-V85     %s
 // RUN: %clang -target aarch64-arm-none-eabi -march=armv8.3-a -x c -E -dM %s -o - | FileCheck --check-prefixes=CHECK-V81-OR-LATER,CHECK-V83-OR-LATER,CHECK-BEFORE-V85   %s
 // RUN: %clang -target aarch64-arm-none-eabi -march=armv8.4-a -x c -E -dM %s -o - | FileCheck --check-prefixes=CHECK-V81-OR-LATER,CHECK-V83-OR-LATER,CHECK-BEFORE-V85   %s