This is an archive of the discontinued LLVM Phabricator instance.

add support for Cavium ThunderX ARM64 processors
ClosedPublic

Authored by steleman on Jan 18 2017, 10:05 PM.

Details

Summary

This set of patches adds support for Cavium ThunderX ARM64 processors:

  • ThunderX
  • ThunderX T81
  • ThunderX T83
  • ThunderX T88

Diff Detail

Repository
rL LLVM

Event Timeline

steleman created this revision.Jan 18 2017, 10:05 PM
steleman updated this revision to Diff 84933.Jan 18 2017, 10:06 PM
steleman updated this revision to Diff 84934.
steleman updated this revision to Diff 84935.
steleman updated this revision to Diff 84936.Jan 18 2017, 10:08 PM
steleman updated this revision to Diff 84937.Jan 18 2017, 10:12 PM

Support for Cavium ThunderX T8X ARM64 processors.

rengolin added a subscriber: llvm-commits.

Few inline comments. I'm adding a few people to review plus copying llvm-commits.

Next time, remember that if you don't do that, people don't see it. :)

cheers,
--renato

lib/Support/TargetParser.cpp
451 ↗(On Diff #84937)

Please, add unit test for this and the new cores.

lib/Target/AArch64/AArch64.td
308 ↗(On Diff #84937)

Isn't LSE 8.1? Do X and XT88 only implement 8.0 + LSE?

Hi:
Some comments specifically on the scheduler. Overall the sched-model looks good, although I don't have the detailed latency and micro-architecture information of ThunderX.

A few points you may want to consider :

  1. You could remove the lines with 'let ResourceCycles = [1];' as that's by-default. That would help you reduce size of descriptions in a number of places.
  1. Same for 'let Latency = 1;'
  1. I don't know the micro-arch of ThunderX, but is there a typo for the latency of THXT8XWriteVST3 as it exceeds corresponding resource-cycle for the same sched-class. You may want to just double-check that.
  1. ReadAdvance seems to exceed write latency in some cases, and so you may want to double check that - e.g. ReadI (2) verus WriteI (1).

Best Regards
Javed

Hi:
Some comments specifically on the scheduler. Overall the sched-model looks good, although I don't have the detailed latency and micro-architecture information of ThunderX.

A few points you may want to consider :

  1. You could remove the lines with 'let ResourceCycles = [1];' as that's by-default. That would help you reduce size of descriptions in a number of places.
  1. Same for 'let Latency = 1;'
  1. I don't know the micro-arch of ThunderX, but is there a typo for the latency of THXT8XWriteVST3 as it exceeds corresponding resource-cycle for the same sched-class. You may want to just double-check that.
  1. ReadAdvance seems to exceed write latency in some cases, and so you may want to double check that - e.g. ReadI (2) verus WriteI (1).

Best Regards
Javed

Thank you very much for the comments - Renato and Javed.

I will add test cases, review the latencies and re-submit with corrections.

MatzeB added a subscriber: MatzeB.Jan 19 2017, 10:42 AM
MatzeB added inline comments.
lib/Target/AArch64/AArch64Subtarget.cpp
90 ↗(On Diff #84937)

HasLSE and HasV8_1aOps are already modeled as target features and should not set here. This switch exists because we cannot model integer (or any non-boolean) values with target features today. The rest should go to AArch64.td where you already added it anyway it seems.

jgreenhalgh edited edge metadata.Jan 20 2017, 4:11 AM

The feature bits set match my (limited) understanding of the various ThunderX variants, and their implementation in GCC. From that perspective this patch looks good to me.

I don't have any opinion on the pipeline model (I don't have access to a datasheet to check against), nor on how idiomatic this patch would be for LLVM (I'm not familiar with the code base).

steleman updated this revision to Diff 86906.Feb 2 2017, 3:30 PM

Fixes and corrections as per review comments.
Added test case for ARMV8.1-A and LSE (test/MC/AArch64/armv8.1a-lse.s).

This latest changeset is based on ToT from 02/02/2017.

steleman updated this revision to Diff 86935.Feb 2 2017, 8:45 PM

Hi, can you also please add the relevant tests to unittests/Support/TargetParserTest.cpp? It should be pretty obvious from the context. Looks good otherwise.

Hi, can you also please add the relevant tests to unittests/Support/TargetParserTest.cpp? It should be pretty obvious from the context.

Yes I certainly can. Please stay tuned.

steleman updated this revision to Diff 87865.Feb 9 2017, 1:15 PM
  • Added tests for ARMV8.1-A LSE Extensions.
  • Added tests in unittests/Support/TargetParserTest.cpp
rengolin accepted this revision.Feb 9 2017, 1:32 PM

Thanks! LGTM.

This revision is now accepted and ready to land.Feb 9 2017, 1:32 PM
MatzeB added inline comments.Feb 9 2017, 5:35 PM
lib/Target/AArch64/AArch64Subtarget.h
48–52 ↗(On Diff #87865)

Please sort this alphabetically!

steleman updated this revision to Diff 88212.Feb 13 2017, 9:47 AM

Sorted the ThunderX CPU names alphabetically in AArch64Subtarget.h.

MatzeB accepted this revision.Feb 13 2017, 10:16 AM

Sorted the ThunderX CPU names alphabetically in AArch64Subtarget.h.

Thanks, still good to go.

This revision was automatically updated to reflect the committed changes.