[PATCH] D20089: Adding a TargetParser for AArch64

[jojo.ma via cfe-commits]

jojo added inline comments.

================
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,
----------------
rengolin wrote:
> Please, format this file to 80-columns.
I neglected this.Format it in the update.

================
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)
----------------
rengolin wrote:
> Same comment as the ARM def file, wrapping with "namespace AArch64 { }".
Looks like the namespace is not supported in the def file. I tried this in the early time.

================
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,
----------------
rengolin wrote:
> This is the "ARM" def file, maybe wrap with "namespace ARM { }"?
Same as "AArch64" def file.

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

================
Comment at: lib/Support/TargetParser.cpp:764
@@ +763,3 @@
+  Arch = getCanonicalArchName(Arch);
+  if (!Arch.startswith("v8"))
+    return ARM::AK_INVALID;
----------------
rengolin wrote:
> Maybe a better check here would be:
> 
>     if (checkArchVersion(Arch) < 8)
>         return ARM::AK_INVALID;
Good advice.Put the check in a method alone.

================
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");
----------------
rengolin wrote:
> 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.
Yes.Moving this to another method "getArchFeatures".


http://reviews.llvm.org/D20089