[PATCH] D48625: [ARM][AArch64] Armv8.4-A enablement

[Oliver Stannard via Phabricator via llvm-commits]

olista01 added inline comments.


================
Comment at: include/llvm/Support/ARMTargetParser.def:137
 ARM_ARCH_EXT_NAME("crypto",   ARM::AEK_CRYPTO,   "+crypto","-crypto")
+ARM_ARCH_EXT_NAME("sm4",      ARM::AEK_SM4,      "+sm4",   "-sm4")
+ARM_ARCH_EXT_NAME("sha3",     ARM::AEK_SHA3,     "+sha3",  "-sha3")
----------------
Why are these needed, if the sha3 and sm4 instructions are AArch64-only?


================
Comment at: include/llvm/Support/TargetParser.h:89
   AEK_DOTPROD =     1 << 14,
+  AEK_SM4     =     1 << 15,
+  AEK_SHA3    =     1 << 16,
----------------
Same here, are these needed?


================
Comment at: lib/Target/AArch64/AArch64.td:35
+    "sha3", "HasSHA3", "true",
+    "Enable SHA512 and SHA3 support", [FeatureNEON]>;
+
----------------
I think SHA3 should also depend on SHA2. From the architecture manual:
> Implementation of ARMv8.2-SHA requires implementation of the ARMv8.0 Cryptographic Extension SHA-1 and SHA256 functionality.


================
Comment at: lib/Target/AArch64/AArch64.td:49
+// Therefore, we rely on Clang, the user interacing tool, to pass on the
+// appropiate crypto options. But here in the backend, crypto has very little
+// meaning anymore. We kept the Crypto defintion here for backward
----------------
Typo: "appropriate".


https://reviews.llvm.org/D48625