This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][ARM] Enablement of Cortex-A710 Support
ClosedPublic

Authored by mubashar_ on Nov 5 2021, 3:14 AM.

Details

Reviewers
dmgreen
lenary

Diff Detail

Event Timeline

mubashar_ created this revision.Nov 5 2021, 3:14 AM
mubashar_ requested review of this revision.Nov 5 2021, 3:14 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 5 2021, 3:14 AM
dmgreen added inline comments.Nov 5 2021, 6:33 AM
llvm/include/llvm/Support/AArch64TargetParser.def
159–160

Natural order would be better I think, where the new A710 is added after the A78.

FlagM should already be included as a part of 8.4, so isn't needed here.
Should BFloat16 be added?

llvm/lib/Target/AArch64/AArch64.td
648

Order below, and you can add FeatureCmpBccFusion according to the optimization guide.

llvm/lib/Target/AArch64/AArch64Subtarget.cpp
105

Add here instead, to keep it with more similar cores.

llvm/lib/Target/ARM/ARM.td
1334

Ordering here and elsewhere, and the formatting is a little off.

lenary added inline comments.Nov 5 2021, 8:38 AM
llvm/include/llvm/Support/AArch64TargetParser.def
159–160

FlagM should already be included as a part of 8.4, so isn't needed here.

FlagM is not part of the AARCH64_ARCH("armv9-a", ARMV9A ...) definition, nor part of the equivalents for armv8.4a and armv8.5a, so it was added explicitly here.

Should BFloat16 be added?

Yes

lenary added a comment.Nov 5 2021, 8:52 AM

I'm happy with these changes, and with the comments from @dmgreen - as I'm away next week, don't wait for my approval to land this if David has given his approval.

dmgreen added inline comments.Nov 7 2021, 8:48 AM
llvm/include/llvm/Support/AArch64TargetParser.def
159–160

FeatureFlagM is included in HasV8_4aOps. So should already be included (much like something like dotprod).

Why AEK_FLAGM isn't part of ARMV8_4A I don't know. And why we have two maps for the same set of information...

mubashar_ updated this revision to Diff 385463.Nov 8 2021, 5:23 AM
mubashar_ marked 6 inline comments as done.

Patch addresses comments.

dmgreen added inline comments.Nov 10 2021, 1:37 AM
llvm/include/llvm/Support/AArch64TargetParser.def
159–160

I think that AEK_FLAGM should be included in architecture that is 8.4+. Then it shouldn't be needed here.
Perhaps lets do that as a separate commit though.

177

I would go after this, it being a a78-like cpu it's probably worth keeping the two together.

llvm/include/llvm/Support/ARMTargetParser.def
312

Ditto and same in other places.

llvm/lib/Target/AArch64/AArch64Subtarget.cpp
87

Make sure you keep this in. It looks like it might have been removed by accident.

llvm/unittests/Support/TargetParserTest.cpp
278

Ordering.

mubashar_ updated this revision to Diff 386165.Nov 10 2021, 7:47 AM
mubashar_ marked 5 inline comments as done.

Placement of Cortex-A710 items changed to order specified by previous comments.

dmgreen accepted this revision.Nov 11 2021, 6:41 AM

Thanks. LGTM

This revision is now accepted and ready to land.Nov 11 2021, 6:41 AM