[PATCH] D53908: [AArch64] Support HiSilicon's TSV110 processor

[Bryan Chan via Phabricator via llvm-commits]

bryanpkc marked 4 inline comments as done.
bryanpkc added inline comments.


================
Comment at: include/llvm/Support/AArch64TargetParser.def:120
                 (AArch64::AEK_CRC | AArch64::AEK_PROFILE))
+AARCH64_CPU_NAME("tsv110", ARMV8_2A, FK_CRYPTO_NEON_FP_ARMV8, false,
+                 (AArch64::AEK_PROFILE | AArch64::AEK_FP16 | AArch64::AEK_FP16FML |
----------------
kristof.beyls wrote:
> Looking at https://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg189436.html it seems tsv110 implements the Armv8.4-A architecture.
> If so, I think it'd be better if you could write "ARMV8_4A" instead of "ARMV8_2A" in the line above.
> What is needed to be able to write "ARMV8_4A"?
TSV110 does not implement RCPC support, which is a mandatory part of ARMv8.3-A and ARMv8.4-A. Therefore, strictly speaking, TSV110 only implements ARMv8.2-A plus a few features introduced in the later architecture levels.

I will take a better look at the GCC code and may submit a patch there as well.


================
Comment at: lib/Target/AArch64/AArch64.td:551
+                                  "HiSilicon TS-V110 processors", [
+                                  HasV8_2aOps,
+                                  FeatureCrypto,
----------------
kristof.beyls wrote:
> What is needed to write "HasV8_4aOps" instead of "HasV8_2aOps" here?
TSV110 would have to implement all features that are not explicitly described as optional in the ARMv8.3-A and ARMv8.4-A specifications. 


================
Comment at: test/CodeGen/AArch64/neon-dot-product.ll:2-4
+; RUN: llc -mtriple aarch64-none-linux-gnu -mcpu=cortex-a55 < %s | FileCheck %s
+; RUN: llc -mtriple aarch64-none-linux-gnu -mcpu=cortex-a75 < %s | FileCheck %s
+; RUN: llc -mtriple aarch64-none-linux-gnu -mcpu=tsv110 < %s | FileCheck %s
----------------
kristof.beyls wrote:
> I guess you're adding these lines to make sure that these cores are modelled as having the dot product instructions.
> Isn't there another test somewhere where you can test this more directly?
You are right. I have moved these tests to `unittests/Support/TargetParserTest.cpp`.


================
Comment at: unittests/Support/TargetParserTest.cpp:799
+  EXPECT_TRUE(testAArch64CPU(
+      "tsv110", "armv8.2-a", "crypto-neon-fp-armv8",
+      AArch64::AEK_CRC | AArch64::AEK_CRYPTO | AArch64::AEK_FP |
----------------
kristof.beyls wrote:
> Should this be "armv8.4-a" instead of "armv8.2-a"?
No. See above.


Repository:
  rL LLVM

https://reviews.llvm.org/D53908