This is an archive of the discontinued LLVM Phabricator instance.

[AArch64]Enabling Cortex-A510 Support
ClosedPublic

Authored by mubashar_ on Sep 15 2021, 6:30 AM.

Details

Summary

This patch enables support for Cortex-A510 CPUs.

Diff Detail

Event Timeline

mubashar_ created this revision.Sep 15 2021, 6:30 AM
mubashar_ requested review of this revision.Sep 15 2021, 6:30 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 15 2021, 6:30 AM
MarkMurrayARM accepted this revision.Sep 15 2021, 6:47 AM

As I did the downstream work for this, I'm happy with it to go in in this form.

This revision is now accepted and ready to land.Sep 15 2021, 6:47 AM
dmgreen requested changes to this revision.Sep 15 2021, 6:50 AM
dmgreen added a subscriber: dmgreen.

As I did the downstream work for this, I'm happy with it to go in in this form.

This doesn't seem.. wise. Please make sure the reviews you do are at a sufficient quality, and it is probably best not to review patches you write yourself.

llvm/include/llvm/Support/AArch64TargetParser.def
201

Why is this 8.3? The TRM (https://developer.arm.com/documentation/101604/0003/The-Cortex-A510--core) describes it as implementing the 9.0-A architecture.

llvm/lib/Target/AArch64/AArch64.td
1083

This should be in some sort of order, next to the Cortex-A55. It should probably be called ProcA510 for consistency too.

1223

Ordering. Please use the CortexA55Model. This is not an out of order core like the A57.

llvm/lib/Target/AArch64/AArch64Subtarget.cpp
196 ↗(On Diff #372691)

This can be the same case block as the A53 and A55.

This revision now requires changes to proceed.Sep 15 2021, 6:50 AM
mubashar_ updated this revision to Diff 372698.Sep 15 2021, 6:50 AM

Updated release notes to solve a merge conflict.

As I did the downstream work for this, I'm happy with it to go in in this form.

This doesn't seem.. wise. Please make sure the reviews you do are at a sufficient quality, and it is probably best not to review patches you write yourself.

Very good point!

I reduce this to the claim that it has been stable downstream for a couple of weeks now.

mubashar_ set the repository for this revision to rG LLVM Github Monorepo.
mubashar_ updated this revision to Diff 378216.Oct 8 2021, 8:00 AM
mubashar_ marked 3 inline comments as done.
xgupta added a subscriber: xgupta.Oct 8 2021, 8:25 AM

Where are the other patches? Or diff is not uploaded correctly.

@xgupta those would be under the history tab.

xgupta added a comment.Oct 8 2021, 8:41 AM

Yeah, those changes are in history, But we need them in the present to commit :)

The patch is incorrectly updated, did you followed https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-command-line?

@xgupta I followed the request review via web interface section of that page. Let me look into it.

mubashar_ removed rG LLVM Github Monorepo as the repository for this revision.Oct 8 2021, 8:47 AM
mubashar_ set the repository for this revision to rG LLVM Github Monorepo.
mubashar_ updated this revision to Diff 378645.Oct 11 2021, 6:29 AM

Added files that were missing from previous diff.

It misses llvm/test/CodeGen/AArch64 and llvm/test/MC/AArch64 testcases changes, see for example https://reviews.llvm.org/D36667 (Cortex-A55 support).

It misses llvm/test/CodeGen/AArch64 and llvm/test/MC/AArch64 testcases changes, see for example https://reviews.llvm.org/D36667 (Cortex-A55 support).

I'm not convinced this is required - those tests are to ensure that the extensions (rcpc, dotprod) can be enabled either from a -march, or from a -mcpu=<cpu> with a cpu that contains that extension -- they are not testing that the extension is enabled from *all* cpus that contain said extension.

oh thanks for the clarification, I am not aware of it.

Thanks for the updates.

It misses llvm/test/CodeGen/AArch64 and llvm/test/MC/AArch64 testcases changes, see for example https://reviews.llvm.org/D36667 (Cortex-A55 support).

I'm not convinced this is required - those tests are to ensure that the extensions (rcpc, dotprod) can be enabled either from a -march, or from a -mcpu=<cpu> with a cpu that contains that extension -- they are not testing that the extension is enabled from *all* cpus that contain said extension.

Yeah I agree. It would be useful to add some tests for -mcpu=cortex-a510+crypto and -mcpu=cortex-a510+nocrypto, to make sure they do what is expected.

clang/docs/ReleaseNotes.rst
86 ↗(On Diff #378645)

Can you add an Arm section below and move this to it.

mubashar_ updated this revision to Diff 379321.Oct 13 2021, 3:05 AM

Added crypto and non-crypto related tests for mcpu in aarch64-cpus.c

Added crypto and non-crypto related tests for mcpu in aarch64-cpus.c

Thanks, but did this change miss being updated in the review?

clang/docs/ReleaseNotes.rst
86 ↗(On Diff #378645)

This still needs doing. Also please add a line to the arm section saying "Added support for the Armv9-A, Armv9.1-A and Armv9.2-A architectures."

It looks like there are some llvm release notes in llvm/docs/ReleaseNotes.rst too, that could have a line added.

llvm/include/llvm/Support/AArch64TargetParser.def
149

AEK_PAUTH is twice here.

mubashar_ updated this revision to Diff 379713.Oct 14 2021, 7:33 AM
mubashar_ marked 2 inline comments as done.

Added crypto and no-crypto tests for -mcpu and added new section for Arm and AArch64 support in release notes.

mubashar_ updated this revision to Diff 379717.Oct 14 2021, 7:54 AM
mubashar_ marked an inline comment as done.

Corrected PAUTH duplication mistake.

mubashar_ marked an inline comment as done.Oct 14 2021, 7:55 AM
mubashar_ added inline comments.
clang/docs/ReleaseNotes.rst
86 ↗(On Diff #378645)

I didn't add support for these architectures. This may already be in its own commit.

dmgreen accepted this revision.Oct 14 2021, 7:56 AM

Thanks for the changes. Nice working pulling this into shape.

LGTM

llvm/include/llvm/Support/AArch64TargetParser.def
149

Can you make sure you remove the second PAUTH here before committing.

This revision is now accepted and ready to land.Oct 14 2021, 7:56 AM
mubashar_ marked an inline comment as done.Oct 14 2021, 8:01 AM
lenary added inline comments.Oct 15 2021, 5:34 AM
clang/docs/ReleaseNotes.rst
86 ↗(On Diff #378645)

I'll do armv9 in a separate NFC commit

This revision was landed with ongoing or failed builds.Oct 15 2021, 6:33 AM
This revision was automatically updated to reflect the committed changes.

This seems to be breaking the macOS bot, can you take a look? https://green.lab.llvm.org/green/job/clang-stage1-RA/24735/

This seems to be breaking the macOS bot, can you take a look? https://green.lab.llvm.org/green/job/clang-stage1-RA/24735/

Will do.