This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Correct v9.x-a Features
ClosedPublic

Authored by lenary on Sep 26 2022, 8:21 AM.

Details

Summary

A change to D109517 during review stated it was disabling the crypto
extensions by default in armv9a, but it also ended up removing two other
non-crypto features: i8mm and bf16. This error was also made in D116158.

This patch re-adds those two extensions to the feature bitmaps for the
affected armv9a versions in the target parser.

Diff Detail

Event Timeline

lenary created this revision.Sep 26 2022, 8:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 26 2022, 8:21 AM
lenary requested review of this revision.Sep 26 2022, 8:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 26 2022, 8:21 AM
lenary updated this revision to Diff 462916.Sep 26 2022, 8:26 AM

Tagging two reviewers who did the original v9a patches for these versions of v9a.

tmatheson accepted this revision.Sep 26 2022, 8:43 AM
This revision is now accepted and ready to land.Sep 26 2022, 8:43 AM

Do you know if this code is necessary: https://github.com/llvm/llvm-project/blob/53c3664f674cc2f33c8214f493c2c17ce9b4d466/clang/lib/Driver/ToolChains/Arch/AArch64.cpp#L483 ? It is related, and I noticed it recently in other reviews.

Do you know if this code is necessary: https://github.com/llvm/llvm-project/blob/53c3664f674cc2f33c8214f493c2c17ce9b4d466/clang/lib/Driver/ToolChains/Arch/AArch64.cpp#L483 ? It is related, and I noticed it recently in other reviews.

That code may be why no test changes were needed. All the target parsing code is so cursed. I think the TargetParser change will affect other non-clang LLVM tools, whereas that code is clang-only. I'm going to do another check or two, to see how load-bearing the clang code is or whether i can remove it now.

The clang code is definitely load-bearing!

I'm juggling a few too many things today to work out exactly why - I think the reality is that these feature bitmaps are only used in AArch64 when you use -mcpu= with a cpu of that version, and are ignored if you use -march=. This is not at all obvious! I think quite a lot of us are fed up with AArch64TargetParser.def because of all these confusing corners.

OK - thanks for checking. The changes here look like an improvement, even if we can't remove that other code quite yet. I'm hoping that as target attributes improve we can remove a lot of the code that is currently in the Driver.

This revision was landed with ongoing or failed builds.Sep 28 2022, 6:53 AM
This revision was automatically updated to reflect the committed changes.