[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

464–473

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.

466–474

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

[tyb0807]

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

[tyb0807]

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

464–473

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

466–474

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.

[tmatheson]

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

263

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

263

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

471–483

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

 //===--- AArch64.cpp - Implement AArch64 target feature support -----------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //

 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

 //===--- AArch64.cpp - AArch64 (not ARM) Helpers for Tools ------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
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) {
         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 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));
+                                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");
 
   if (Args.hasArg(options::OPT_ffixed_x2))
     Features.push_back("+reserve-x2");
 

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