[AArch64] ARMV8-A archkind and target defines helper functions

[SjoerdMeijer]

Summary

This introduces helper functions that set target defines for different ARMV8-A architecture kinds. It fixes an issue that the v8.1 define ARM_FEATURE_QRDMX was not set for v8.2. These helper functions make things more “scalable” if we want to add ARMv8.3 at some point, and a cleanup has been done to hold the architecture kind in one variable (instead of one for each).

[rogfer01]

I agree with the spirit of the change but I think you want to use AArch64:: enumerators instead of those in ARM::

lib/Basic/Targets.cpp

6190 ↗

(On Diff #104159)

There is an AArch64::ArchKind enum in TargetParser.h, not sure if it would be useable here.

6332 ↗

(On Diff #104159)

I wonder if this is wrong, if so please fix it in a further change otherwise this one wouldn't be a NFCI.

6364 ↗

(On Diff #104159)

Would it make sense to set this to AArch64::AK_ARMV8A instead?

6376 ↗

(On Diff #104159)

I think you want to use llvm::AArch64::AK_ARMV8_1A instead here.

6378 ↗

(On Diff #104159)

Ditto with llvm::AArch64::AK_ARMV8_2A

[SjoerdMeijer]

Thanks! I am now using llvm::AArch64::ArchKind. And I agree that the check for setting __ARM_FEATURE_QRDMX is suspicious. I will address this separately.

[SjoerdMeijer]

Actually, I've decided to make the functional change here because I am touching and modifying this code again.

So this is now a functional change: ARM_FEATURE_QRDMX is now defined also for ARMv8.2. This is a define for some NEON additions to ARMv8.1, which should also be included to ARMv8.2.
I've created helper functions to include defines for the different architecture kinds, and a call to include the v8.2 defines also include the v8.1 ones. Regression tests have been added for this.

I have also modified the title/summary of this ticket.

[rogfer01]

This LGTM. Thanks for the refactoring and the fix.

cfe/trunk/lib/Basic/Targets.cpp

if (ArchKind == llvm::ARM::AK_INVALID)
       return false;
     setAtomic();
     CPU = Name;
     return true;
   }
 
   bool setFPMath(StringRef Name) override;
 
+  void getTargetDefinesARMV81A(const LangOptions &Opts,
+                               MacroBuilder &Builder) const {
+    Builder.defineMacro("__ARM_FEATURE_QRDMX", "1");
+  }
+
+  void getTargetDefinesARMV82A(const LangOptions &Opts,
+                               MacroBuilder &Builder) const {
+    // Also include the ARMv8.1-A defines
+    getTargetDefinesARMV81A(Opts, Builder);
+  }
+
   void getTargetDefines(const LangOptions &Opts,
                         MacroBuilder &Builder) const override {
     // Target identification.
     Builder.defineMacro("__arm");
     Builder.defineMacro("__arm__");
     // For bare-metal none-eabi.
     if (getTriple().getOS() == llvm::Triple::UnknownOS &&
         (getTriple().getEnvironment() == llvm::Triple::EABI ||
void getTargetDefines(const LangOptions &Opts,
 
     // ACLE 6.4.6 Q (saturation) flag
     if (DSP || SAT)
       Builder.defineMacro("__ARM_FEATURE_QBIT", "1");
 
     if (Opts.UnsafeFPMath)
       Builder.defineMacro("__ARM_FP_FAST", "1");
 
-    if (ArchKind == llvm::ARM::AK_ARMV8_1A)
-      Builder.defineMacro("__ARM_FEATURE_QRDMX", "1");
+    switch(ArchKind) {
+    default: break;
+    case llvm::ARM::AK_ARMV8_1A:
+      getTargetDefinesARMV81A(Opts, Builder);
+      break;
+    case llvm::ARM::AK_ARMV8_2A:
+      getTargetDefinesARMV82A(Opts, Builder);
+      break;
+    }
   }
 
   ArrayRef<Builtin::Info> getTargetBuiltins() const override {
     return llvm::makeArrayRef(BuiltinInfo,
                              clang::ARM::LastTSBuiltin-Builtin::FirstTSBuiltin);
   }
   bool isCLZForZeroUndef() const override { return false; }
   BuiltinVaListKind getBuiltinVaListKind() const override {
enum FPUModeEnum {
     FPUMode,
     NeonMode
   };
 
   unsigned FPU;
   unsigned CRC;
   unsigned Crypto;
   unsigned Unaligned;
-  unsigned V8_1A;
-  unsigned V8_2A;
   unsigned HasFullFP16;
+  llvm::AArch64::ArchKind ArchKind;
 
   static const Builtin::Info BuiltinInfo[];
 
   std::string ABI;
 
 public:
   AArch64TargetInfo(const llvm::Triple &Triple, const TargetOptions &Opts)
       : TargetInfo(Triple), ABI("aapcs") {
public:
   }
 
   bool setCPU(const std::string &Name) override {
     return Name == "generic" ||
            llvm::AArch64::parseCPUArch(Name) !=
            static_cast<unsigned>(llvm::AArch64::ArchKind::AK_INVALID);
   }
 
+  void getTargetDefinesARMV81A(const LangOptions &Opts,
+                        MacroBuilder &Builder) const {
+    Builder.defineMacro("__ARM_FEATURE_QRDMX", "1");
+  }
+
+  void getTargetDefinesARMV82A(const LangOptions &Opts,
+                        MacroBuilder &Builder) const {
+    // Also include the ARMv8.1 defines
+    getTargetDefinesARMV81A(Opts, Builder);
+
+    if (FPU == NeonMode && HasFullFP16)
+      Builder.defineMacro("__ARM_FEATURE_FP16_VECTOR_ARITHMETIC", "1");
+  }
+
   void getTargetDefines(const LangOptions &Opts,
                         MacroBuilder &Builder) const override {
     // Target identification.
     Builder.defineMacro("__aarch64__");
     // For bare-metal none-eabi.
     if (getTriple().getOS() == llvm::Triple::UnknownOS &&
         (getTriple().getEnvironment() == llvm::Triple::EABI ||
          getTriple().getEnvironment() == llvm::Triple::EABIHF))
if (CRC)
       Builder.defineMacro("__ARM_FEATURE_CRC32", "1");
 
     if (Crypto)
       Builder.defineMacro("__ARM_FEATURE_CRYPTO", "1");
 
     if (Unaligned)
       Builder.defineMacro("__ARM_FEATURE_UNALIGNED", "1");
 
-    if (V8_1A)
-      Builder.defineMacro("__ARM_FEATURE_QRDMX", "1");
-    if (V8_2A && FPU == NeonMode && HasFullFP16)
-      Builder.defineMacro("__ARM_FEATURE_FP16_VECTOR_ARITHMETIC", "1");
+    switch(ArchKind) {
+    default: break;
+    case llvm::AArch64::ArchKind::AK_ARMV8_1A:
+      getTargetDefinesARMV81A(Opts, Builder);
+      break;
+    case llvm::AArch64::ArchKind::AK_ARMV8_2A:
+      getTargetDefinesARMV82A(Opts, Builder);
+      break;
+    }
 
     // All of the __sync_(bool|val)_compare_and_swap_(1|2|4|8) builtins work.
     Builder.defineMacro("__GCC_HAVE_SYNC_COMPARE_AND_SWAP_1");
     Builder.defineMacro("__GCC_HAVE_SYNC_COMPARE_AND_SWAP_2");
     Builder.defineMacro("__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4");
     Builder.defineMacro("__GCC_HAVE_SYNC_COMPARE_AND_SWAP_8");
   }
 
public:
   }
 
   bool handleTargetFeatures(std::vector<std::string> &Features,
                             DiagnosticsEngine &Diags) override {
     FPU = FPUMode;
     CRC = 0;
     Crypto = 0;
     Unaligned = 1;
-    V8_1A = 0;
-    V8_2A = 0;
     HasFullFP16 = 0;
+    ArchKind = llvm::AArch64::ArchKind::AK_ARMV8A;
 
     for (const auto &Feature : Features) {
       if (Feature == "+neon")
         FPU = NeonMode;
       if (Feature == "+crc")
         CRC = 1;
       if (Feature == "+crypto")
         Crypto = 1;
       if (Feature == "+strict-align")
         Unaligned = 0;
       if (Feature == "+v8.1a")
-        V8_1A = 1;
+        ArchKind = llvm::AArch64::ArchKind::AK_ARMV8_1A;
       if (Feature == "+v8.2a")
-        V8_2A = 1;
+        ArchKind = llvm::AArch64::ArchKind::AK_ARMV8_2A;
       if (Feature == "+fullfp16")
         HasFullFP16 = 1;
     }
 
     setDataLayout();
 
     return true;
   }

cfe/trunk/test/Preprocessor/aarch64-target-features.c

 // RUN: %clang -target arm64-none-linux-gnu -fshort-enums -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-SHORTENUMS %s
 // CHECK-SHORTENUMS: __ARM_SIZEOF_MINIMAL_ENUM 1
 
 // RUN: %clang -target aarch64-none-linux-gnu -march=armv8-a+simd -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-NEON %s
 // RUN: %clang -target arm64-none-linux-gnu -march=armv8-a+simd -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-NEON %s
 // CHECK-NEON: __ARM_NEON 1
 // CHECK-NEON: __ARM_NEON_FP 0xE
 
-// RUN: %clang -target aarch64-none-eabi -march=armv8.1-a -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-V81A %s
-// CHECK-V81A: __ARM_FEATURE_QRDMX 1
+// RUN: %clang -target aarch64-none-eabi -march=armv8.1-a -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-QRDMX %s
+// RUN: %clang -target aarch64-none-eabi -march=armv8.2-a -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-QRDMX %s
+// CHECK-QRDMX: __ARM_FEATURE_QRDMX 1
 
 // RUN: %clang -target aarch64 -march=arm64 -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-ARCH-NOT-ACCEPT %s
 // RUN: %clang -target aarch64 -march=aarch64 -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-ARCH-NOT-ACCEPT %s
 // CHECK-ARCH-NOT-ACCEPT: error: the clang compiler does not support
 
 // RUN: %clang -target aarch64 -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-GENERIC %s
 // RUN: %clang -target aarch64 -march=armv8-a -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-GENERIC %s
 // CHECK-GENERIC: "-cc1"{{.*}} "-triple" "aarch64{{.*}}" "-target-feature" "+neon"

cfe/trunk/test/Preprocessor/arm-target-features.c

 // CHECK-V81A: #define __ARM_ARCH_PROFILE 'A'
 // CHECK-V81A: #define __ARM_FEATURE_QRDMX 1
 // CHECK-V81A: #define __ARM_FP 0xE
 
 // RUN: %clang -target armv8.2a-none-none-eabi -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-V82A %s
 // CHECK-V82A: #define __ARM_ARCH 8
 // CHECK-V82A: #define __ARM_ARCH_8_2A__ 1
 // CHECK-V82A: #define __ARM_ARCH_PROFILE 'A'
+// CHECK-V82A: #define __ARM_FEATURE_QRDMX 1
 // CHECK-V82A: #define __ARM_FP 0xE