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

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls added a subscriber: olista01.
kristof.beyls 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 |
----------------
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"?


================
Comment at: lib/Support/Host.cpp:214-223
+  if (Implementer == "0x48") // HiSilicon Technologies, Inc.
+    // Look for the CPU part line.
+    for (unsigned I = 0, E = Lines.size(); I != E; ++I)
+      if (Lines[I].startswith("CPU part"))
+        // The CPU part is a 3 digit hexadecimal number with a 0x prefix. The
+        // values correspond to the "Part number" in the CP15/c0 register. The
+        // contents are specified in the various processor manuals.
----------------
There are unit tests for this functionality in unittests/Support/Host.cpp. Maybe you also want to add a test for this case?


================
Comment at: lib/Target/AArch64/AArch64.td:551
+                                  "HiSilicon TS-V110 processors", [
+                                  HasV8_2aOps,
+                                  FeatureCrypto,
----------------
What is needed to write "HasV8_4aOps" instead of "HasV8_2aOps" here?


================
Comment at: test/CodeGen/AArch64/arm64-neon-simd-ldst-one.ll:2
 ; RUN: llc < %s -verify-machineinstrs -mtriple=arm64-none-linux-gnu -mattr=+neon | FileCheck %s
+; RUN: llc < %s -verify-machineinstrs -mcpu=tsv110 -mtriple=arm64-none-linux-gnu -mattr=+neon | FileCheck %s
 
----------------
I don't think that for this test file it makes sense to add a run line for a specific CPU. If you don't have a strong reason why to add this line, please remove it.


================
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
----------------
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?


================
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 |
----------------
Should this be "armv8.4-a" instead of "armv8.2-a"?


Repository:
  rL LLVM

https://reviews.llvm.org/D53908