[PATCH] D20089: Adding a TargetParser for AArch64

[Renato Golin via cfe-commits]

rengolin added a comment.

Hi Jojo,

This is looking better, thanks!

While I agree with Bradley that the repetition is not pretty, but I think it will expose all issues to make a class design simple and straightforward, once we get all the sharp edges out. But we need to know what are the difficulties on Clang, llc and the back-ends, and make sure that the architectural part of the change is correct, before we that.

A class based design *will* change how every TargetParser method is being called now, and will touch a large number of files with mechanical changes, including in the ARM side, unrelated to the addition of AArch64.

After my failed attempt at getting a class design across, I'd rather introduce functionality first, then design better, than the other way round. But I think they should be separated commits based on their intents and merits alone.

cheers,
--renato


================
Comment at: include/llvm/Support/AArch64TargetParser.def:20
@@ +19,3 @@
+AARCH64_ARCH("armv8-a", AK_ARMV8A, "8-A", "v8", ARMBuildAttrs::CPUArch::v8_A,
+          FK_CRYPTO_NEON_FP_ARMV8, (AArch64::AEK_CRC | AArch64::AEK_CRYPTO | AArch64::AEK_FP | AArch64::AEK_SIMD | AArch64::AEK_FP16 | AArch64::AEK_PROFILE))
+AARCH64_ARCH("armv8.1-a", AK_ARMV8_1A, "8.1-A", "v8.1a", ARMBuildAttrs::CPUArch::v8_A,
----------------
Please, format this file to 80-columns.

================
Comment at: include/llvm/Support/AArch64TargetParser.def:31
@@ +30,3 @@
+// FIXME: This would be nicer were it tablegen
+AARCH64_ARCH_EXT_NAME("invalid",  AArch64::AEK_INVALID,  nullptr,  nullptr)
+AARCH64_ARCH_EXT_NAME("none",     AArch64::AEK_NONE,     nullptr,  nullptr)
----------------
Same comment as the ARM def file, wrapping with "namespace AArch64 { }".

================
Comment at: include/llvm/Support/ARMTargetParser.def:48
@@ -47,3 +47,3 @@
 ARM_ARCH("invalid", AK_INVALID, nullptr, nullptr,
-          ARMBuildAttrs::CPUArch::Pre_v4, FK_NONE, AEK_NONE)
+          ARMBuildAttrs::CPUArch::Pre_v4, FK_NONE, ARM::AEK_NONE)
 ARM_ARCH("armv2", AK_ARMV2, "2", "v2", ARMBuildAttrs::CPUArch::Pre_v4,
----------------
This is the "ARM" def file, maybe wrap with "namespace ARM { }"?

================
Comment at: include/llvm/Support/TargetParser.h:144
@@ +143,3 @@
+
+namespace AArch64 {
+
----------------
Please, add a FIXME line here, saying this really should be made into a class, to use ARM's methods from inheritance.

================
Comment at: lib/Support/TargetParser.cpp:764
@@ +763,3 @@
+  Arch = getCanonicalArchName(Arch);
+  if (!Arch.startswith("v8"))
+    return ARM::AK_INVALID;
----------------
Maybe a better check here would be:

    if (checkArchVersion(Arch) < 8)
        return ARM::AK_INVALID;

================
Comment at: lib/Support/TargetParser.cpp:770
@@ +769,3 @@
+    if (A.getName().endswith(Syn)) {
+      if (A.ID == ARM::AK_ARMV8_1A)
+        Features.push_back("+v8.1a");
----------------
I don't like mixing Features with parseArch. Maybe we could do two different methods and get callers to use both when needed, just like with ARM.

parseArch needs to be dead simple.


http://reviews.llvm.org/D20089