This is the first checkin to support Marvell ThunderX3T110.
Initial definition of the micro-ops of the instructions in ThunderX3T110 is included.
Paths
| Differential D78129
Add Marvell ThunderX3T110 support ClosedPublic Authored by wxz2020 on Apr 14 2020, 10:08 AM.
Details Summary This is the first checkin to support Marvell ThunderX3T110. Initial definition of the micro-ops of the instructions in ThunderX3T110 is included.
Diff Detail
Event TimelineComment Actions The newly added two files were not accepted by the pre-merge check. Re worked on the diff file and uploaded it now. ktkachov added inline comments.
wxz2020 added inline comments. Comment Actions Remove non-official target options. We now only support -mcpu=thunderx3t110 Typo fix. chill added inline comments.
DavidSpickett added inline comments.
SjoerdMeijer added inline comments.
wxz2020 added inline comments.
Comment Actions In email Wei asked for help about he following error message: error message from tblgen Included from /home/wei/project/tx3/llvm-project/llvm/lib/Target/AArch64/AArch64.td:439: /home/wei/project/tx3/llvm-project/llvm/lib/Target/AArch64/AArch64InstrInfo.td:961:5: error: 'CycloneModel' lacks information for 'AUTDZA' def DZA : SignAuthZero<prefix_z, 0b10, !strconcat(asm, "dza")>; ^ In the end: Incomplete schedule models found. · Consider setting 'CompleteModel = 0' while developing new models. · Pseudo instructions can be marked with 'hasNoSchedulingInfo = 1'. · Instructions should usually have Sched<[...]> as a superclass, you may temporarily use an empty list. · Instructions related to unsupported features can be excluded with list<Predicate> UnsupportedFeatures = [HasA,..,HasY]; in the processor model. error: Incomplete schedule model And the comment from the person on the email was to define the instruction. The instruction is defined, as evidenced when table gen is run to produce the record-list. The error message can be suppressed by defining CompleteModele = 0, but that isn't correct, as there are models for those instructions. However, I now note that at least In the output that Wei captured, it isn't complaining about the ThunderX3T110 model, but about Cyclone. Comment Actions
Hi Joel, It is okay'ish to set CompleteModel = 0 if you're not interested in describing all instructions. The other way is of course to add the missing instruction to the model. But first things first. @chill commented earlier about predicates. I also don't think you need to add HasV8_3a for the reason Momchill described, and there is still one left AArch64InstrInfo.td. And while I am still looking into this, let me ask the question what I am wondering: why do you need to change the other scheduling models? These instructions are always supported, as a NOP, or otherwise. Comment Actions
We are trying to describe all instructions. The issue Wei is having is that the pointer auth instructions are being called out in the error message, but their timing information is defined—see the very end of AArch64SchedThunderX3T110.td for AUTDZA.
The changes to UnsupportedFeatures that was in every model has been taken out. I'm not sure why UnsupportedFeatures exists at all, but it isn't just AArch64 that has this. Should the latest version be put up even if it fails on table-gen? That would make things easier to review. Comment Actions I tried to set CompleteModel = 0 on line 25 in file llvm/lib/Target/AArch64/AArch64SchedThunderX3T110.td The same error is still there.
Comment Actions Since debugging on phabricator is a bit difficult, I took the patch and had a little play. Now I actually remember seeing this before. I think the way this works is that when you describe new instructions (PA in this case) that other models don't have, they start complaining about incomplete models when they have set let CompleteModel = 1 I got things compiling with these changes:
I have never really looked into these instructions, and quick grep doesn't show any descriptions for some. For example, AUTDA doesn't exist. The others look aliases, perhaps they need a different treatment, or a regexp, I don't know, but perhaps you can take it from here. Comment Actions I can confirm your findings, basically as long as we comment out the new instructions, it will be OK to build. Agree with your changes 1), 2) and 3).
Thanks a lot for the help, Comment Actions I don't think it makes sense to combine two unrelated things SVE and PA support into a combined thing. Since we already have UnsupportedFeatures in every sub-target .td file, I think it would be better to instead have: def PAUnsupported : AArch64Unsupported { let F = [HasPA]; } and modify each .td file to have list<Predicate> UnsupportedFeatures = !listconcat(SVEUnsupported.F, PAUnsupported.F); Comment Actions
Agree. Methinks that this is easier to read. Comment Actions Agreed about the UnsupportedFeatures list. But I just wanted to unblock this work, create a first version that compiles, so you can pick it up and clean things further up. Comment Actions I put every people's feedbacks into the code and upload them here. I think we can use it as the start point for TX3, and add more when we move on. Thank you all for great help, Comment Actions Let's separate out HasPA from SVEFeatures now while we are at it, probably it's more work to do this as a follow up, and after that this looks good to me. Bonus points for adding some llvm-mca tests, see the llvm-project/llvm/test/tools/llvm-mca/ directory for some inspiration how this latencies can be checked, but that seems like something for follow-up.
Comment Actions I think I made all the changes mentioned in the feedbacks. Please take sometime to review the current version. We are eager to get the first thunderx3t110 checked in. There are a few things we plan to do after the 1st version:
Let me know if there is anything missing. Thanks, Comment Actions
Can you add what Joel suggested? I don't see the point of doing this as a follow up. This very simple thing is done half right here, so is best fixed here as there is no reason to leave this for another time; following up on this and talking about this is more work than just doing it. Comment Actions
Comment Actions This looks good now, but sorry, one more request: I've just noticed a Clang driver test is missing. Can you add a test for this to clang/test/Driver/aarch64-cpus.c? And related to this, the relevant tests to llvm/unittests/Support/TargetParserTest.cpp? Comment Actions
So far all the existing issues are fixed according to review feedbacks Thanks Comment Actions Correct the wrong version of llvm/unittests/Support/TargetParserTest.cpp which causes the test failure. This revision is now accepted and ready to land.May 13 2020, 12:39 AM Closed by commit rG382d3a85e2a9: [AARch64] Add Marvell ThunderX3T110 support (authored by wxz2020, committed by joelkevinjones). · Explain WhyMay 13 2020, 5:29 PM This revision was automatically updated to reflect the committed changes. Comment Actions I have a question here. Our customers need the TX3 support urgently, they asked me to put this TX3 support to LLVM 10.0.1 and if there is 9.0.2 release planned, they also need it there. How can I do this? To put this checkin to LLVM 10.0.1 branch and 9.0.2 branch? Is it possible? What steps I should follow? Thanks, Comment Actions Best to ask the release manager. Today Tom Stellard posted a message to the llvm dev list with subject: [llvm-dev] LLVM 10.0.1-rc1 release update So best to reply to this(*) asap with your query if it's possible to get this onto the branch. (*) http://lists.llvm.org/pipermail/llvm-dev/2020-May/141670.html
Revision Contents
Diff 263896 clang/test/Driver/aarch64-cpus.c
llvm/include/llvm/Support/AArch64TargetParser.def
llvm/lib/Target/AArch64/AArch64.td
llvm/lib/Target/AArch64/AArch64SchedA53.td
llvm/lib/Target/AArch64/AArch64SchedA57.td
llvm/lib/Target/AArch64/AArch64SchedCyclone.td
llvm/lib/Target/AArch64/AArch64SchedExynosM3.td
llvm/lib/Target/AArch64/AArch64SchedExynosM4.td
llvm/lib/Target/AArch64/AArch64SchedExynosM5.td
llvm/lib/Target/AArch64/AArch64SchedFalkor.td
llvm/lib/Target/AArch64/AArch64SchedKryo.td
llvm/lib/Target/AArch64/AArch64SchedThunderX.td
llvm/lib/Target/AArch64/AArch64SchedThunderX2T99.td
llvm/lib/Target/AArch64/AArch64SchedThunderX3T110.td
llvm/lib/Target/AArch64/AArch64Subtarget.h
llvm/lib/Target/AArch64/AArch64Subtarget.cpp
llvm/test/CodeGen/AArch64/aarch64-combine-fmul-fsub.mir
llvm/test/CodeGen/AArch64/cpus.ll
llvm/test/CodeGen/AArch64/machine-combiner-madd.ll
llvm/test/CodeGen/AArch64/preferred-function-alignment.ll
llvm/test/CodeGen/AArch64/remat.ll
llvm/unittests/Support/TargetParserTest.cpp
|
Add a tests for this in llvm/unittests/Support/TargetParserTest.cpp (there are existing ones for "thunderx2t99").
Might need to update "NumAArch64CPUArchs" too.