This is an archive of the discontinued LLVM Phabricator instance.

[PATCH] [ARM] ARMv8.6-a command-line + BFloat16 Asm Support
ClosedPublic

Authored by stuij on Mar 12 2020, 6:41 AM.

Details

Summary

This patch introduces command-line support for the Armv8.6-a architecture and assembly support for BFloat16. Details can be found
https://community.arm.com/developer/ip-products/processors/b/processors-ip-blog/posts/arm-architecture-developments-armv8-6-a

in addition to the GCC patch for the 8..6-a CLI:
https://gcc.gnu.org/legacy-ml/gcc-patches/2019-11/msg02647.html

In detail this patch

  • march options for armv8.6-a
  • BFloat16 assembly

This is part of a patch series, starting with command-line and Bfloat16
assembly support. The subsequent patches will upstream intrinsics
support for BFloat16, followed by Matrix Multiplication and the
remaining Virtualization features of the armv8.6-a architecture.

Based on work by:

  • labrinea
  • MarkMurrayARM
  • Luke Cheeseman
  • Javed Asbar
  • Mikhail Maltsev
  • Luke Geeson

Diff Detail

Event Timeline

LukeGeeson created this revision.Mar 12 2020, 6:41 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 12 2020, 6:41 AM
LukeGeeson edited the summary of this revision. (Show Details)Mar 12 2020, 6:42 AM
LukeGeeson edited the summary of this revision. (Show Details)Mar 12 2020, 6:59 AM
LukeGeeson edited the summary of this revision. (Show Details)Mar 12 2020, 7:38 AM
jfb added a reviewer: jfb.Mar 12 2020, 11:22 AM
stuij added a subscriber: stuij.Mar 16 2020, 10:30 AM
stuij commandeered this revision.Mar 16 2020, 10:32 AM
stuij added a reviewer: LukeGeeson.

Commandeered because Luke is on vacation.

stuij updated this revision to Diff 250630.Mar 16 2020, 2:17 PM
stuij edited the summary of this revision. (Show Details)

Updating D76062: [PATCH] [ARM] ARMv8.6-a command-line + BFloat16 Asm Support

follow changes in patch: [TableGen] Support combining AssemblerPredicates with ORs

stuij edited the summary of this revision. (Show Details)Mar 16 2020, 2:21 PM
stuij added a comment.Mar 18 2020, 3:16 AM

The failing test is not related to this ticket. It is caused by https://reviews.llvm.org/D70720/new/#1925184, Oliver Stannard is looking into it.

SjoerdMeijer added inline comments.Mar 18 2020, 3:54 AM
clang/lib/Basic/Targets/AArch64.cpp
189

Can you be more specific, what are we missing here?

Hmm, now I see the same above: "FIXME: Armv8.5 makes some extensions mandatory. Handle them here."
While you're at it, can you also change that?

llvm/include/llvm/Support/AArch64TargetParser.def
52

just double checking (because I can't remember): BF16 is a mandatory extension?

llvm/lib/Target/ARM/ARM.td
532

it's implied here, so looks mandatory.

llvm/lib/Target/ARM/ARMSubtarget.h
260

nit: BFloat16 floating point -> BFloat16 floating point operations

stuij updated this revision to Diff 251336.Mar 19 2020, 4:05 AM
stuij marked 3 inline comments as done.

addressing Sjoerd's comments on mandatory defines and rewording

stuij marked an inline comment as done.Mar 19 2020, 4:06 AM
stuij added inline comments.
clang/lib/Basic/Targets/AArch64.cpp
189

Those comments were somewhat of a drive-by nature, made by a GCC contributor. I went through the v8.x extensions and tried to match them with both GCC and LLVM feature defines. Neither LLVM or GCC has defines for all extensions, and after talking to the contributer (Kyrylo Tkachov), it turns out he had at most parity with GCC in mind.

As an aside GCC seems to have about 14 feature defines that are not present in LLVM, and one feature is spelled ARM_FEATURE_FP16_FML in GCC and ARM_FEATURE_FP16FML in LLVM.

After surveying the list, I found a few defines that could be placed here. I've added them as inline comments. I think they deserve their own patch, as this involves multiple revisions and you'd like to make sure that it's all handled sensibly. I believe we have plans to overhaul this general area.

I've removed the fixme's in revisions that didn't actually need them.

llvm/include/llvm/Support/AArch64TargetParser.def
52

for 8.2 it isn't, for 8.6 it is

Besides the irrelevant formatting nits, one minor question about the clang test.

clang/test/Driver/aarch64-cpus.c
622

Do we need 2 additional tests here?

  • one for v8.6,
  • and another with SVE?
llvm/lib/Target/AArch64/AArch64InstrFormats.td
7806

nit: indentation is a bit off here

7808

here too

7812

and here

7820

and this can be on the same line as above?

7867

and perhaps this one. But looks intentional, perhaps it's fine then, I don't know.

llvm/lib/Target/ARM/ARMInstrNEON.td
8936

on the same line as above?

8937

no newline?

stuij updated this revision to Diff 251916.Mar 22 2020, 2:21 PM
stuij marked 9 inline comments as done.

reindenting a few lines

clang/test/Driver/aarch64-cpus.c
622

I think we're ok here.

  • for v8.6 the driver doesn't pass bfloat as an argument
  • SVE: from a cmdline perspective, there's no special interaction between bfloat and SVE. either can be active without the other.
llvm/lib/Target/AArch64/AArch64InstrFormats.td
7867

Yes, the square brackets will be filled in in a next patch. I'll just leave them as is.

stuij updated this revision to Diff 252443.Mar 24 2020, 3:21 PM

adding sve and bf16+sve driver tests

stuij added a comment.Mar 24 2020, 3:23 PM

after back-and-forth with Sjoerd, added sve and bf16+sve driver tests

SjoerdMeijer accepted this revision.Mar 25 2020, 1:38 AM

Thanks, LGTM

This revision is now accepted and ready to land.Mar 25 2020, 1:38 AM
This revision was automatically updated to reflect the committed changes.

apologies please ignore adding here, added reviewers to the wrong diff

ab added a subscriber: ab.Sep 21 2022, 10:53 AM
ab added inline comments.
llvm/include/llvm/Support/AArch64TargetParser.def
52

Belated question: what's the rationale for replacing AEK_CRYPTO with SM4+SHA3+SHA2(+AES)? I'm not aware of the required crypto bits changing, but maybe I missed something. There's a related question around how we should deal with crypto here in the first place (remove FK, remove the crypto exts?), but that seems orthogonal to v8.6a+ vs. v8.5a- implying different crypto extensions.

For context, this comes up in D134351 where specifying V8_6A would enable SM4, which we don't support.

Herald added a project: Restricted Project. · View Herald TranscriptSep 21 2022, 10:53 AM
Herald added a subscriber: StephenFan. · View Herald Transcript
stuij added inline comments.Jan 20 2023, 3:34 AM
llvm/include/llvm/Support/AArch64TargetParser.def
52

Belated (initial) answer: yes unfortunately our crypto story is a bit of a mess (mostly related to it mandatory yes or no). we have a catch-all ticket on our backlog to deal with crypto inconsistencies, which will hopefully be picked up soon.

I've added your point, and so again hopefully soon we can provide clarity and smooth things out.