This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Support HiSilicon's TSV110 processor
ClosedPublic

Authored by bryanpkc on Oct 30 2018, 3:34 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

bryanpkc created this revision.Oct 30 2018, 3:34 PM
kristof.beyls added inline comments.
include/llvm/Support/AArch64TargetParser.def
120 ↗(On Diff #171827)

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

lib/Support/Host.cpp
214–223 ↗(On Diff #171827)

There are unit tests for this functionality in unittests/Support/Host.cpp. Maybe you also want to add a test for this case?

lib/Target/AArch64/AArch64.td
551 ↗(On Diff #171827)

What is needed to write "HasV8_4aOps" instead of "HasV8_2aOps" here?

test/CodeGen/AArch64/arm64-neon-simd-ldst-one.ll
2 ↗(On Diff #171827)

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.

test/CodeGen/AArch64/neon-dot-product.ll
2–4 ↗(On Diff #171827)

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?

unittests/Support/TargetParserTest.cpp
799 ↗(On Diff #171827)

Should this be "armv8.4-a" instead of "armv8.2-a"?

bryanpkc marked 4 inline comments as done.Nov 1 2018, 2:26 PM
bryanpkc added inline comments.
include/llvm/Support/AArch64TargetParser.def
120 ↗(On Diff #171827)

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.

lib/Target/AArch64/AArch64.td
551 ↗(On Diff #171827)

TSV110 would have to implement all features that are not explicitly described as optional in the ARMv8.3-A and ARMv8.4-A specifications.

test/CodeGen/AArch64/neon-dot-product.ll
2–4 ↗(On Diff #171827)

You are right. I have moved these tests to unittests/Support/TargetParserTest.cpp.

unittests/Support/TargetParserTest.cpp
799 ↗(On Diff #171827)

No. See above.

bryanpkc updated this revision to Diff 172235.Nov 1 2018, 2:28 PM
bryanpkc marked 2 inline comments as done.

Added a unit test for lib/Support/Host.cpp and removed some redundant tests.

Pinging reviewers....

kristof.beyls accepted this revision.Nov 9 2018, 2:10 AM

This patch looks fine to me, assuming the TSV110 core implements Armv8.2 including the optional extensions ARMv8.2-FP16, ARMv8.2-DotProd, ARMv8.2-FHM and SPE.

This revision is now accepted and ready to land.Nov 9 2018, 2:10 AM

Thanks Kristof!

This revision was automatically updated to reflect the committed changes.