This is an archive of the discontinued LLVM Phabricator instance.

[PATCH] [ARM]: Armv8.6-a Matrix Mul Asm and Intrinsics Support
AbandonedPublic

Authored by LukeGeeson on Apr 6 2020, 4:55 AM.

Details

Summary

This patch upstreams support for the Armv8.6-a Matrix Multiplication
Extension. A summary of the features can be found here:

https://community.arm.com/developer/ip-products/processors/b/processors-ip-blog/posts/arm-architecture-developments-armv8-6-a

This patch includes:

  • Assembly support for AArch32, AArch64, NEON, and SVE
  • Intrinsics Support for AArch32, AArch64
  • Command line options to enable these features with +f32mm or f64mm

Note: these extensions are optional in the 8.6a architecture and so have
to be enabled by default

No IR types or C Types are needed for this extension.

This is part of a patch series, starting with BFloat16 support and
the other components in the armv8.6a extension (in previous patches
linked in phabricator)

Based on work by:

  • Luke Geeson
  • Oliver Stannard
  • Luke Cheeseman

Diff Detail

Event Timeline

LukeGeeson created this revision.Apr 6 2020, 4:55 AM

build failures related to linting, unit tests passing

DavidSpickett added inline comments.
clang/lib/Basic/Targets/AArch64.cpp
284

I don't see specific tests for this #define

clang/lib/Basic/Targets/AArch64.h
39

Why is this feature a number for AArch64, does >1 mean something?

LukeGeeson updated this revision to Diff 256006.Apr 8 2020, 6:38 AM
LukeGeeson marked 2 inline comments as done.

changed hasmatmul type to boolean in clang/lib/basic/target/aarch64.h and related files

clang/lib/Basic/Targets/AArch64.h
39

It seems this was an artifact from how this is handled in ARM.h where they do have special meaning. I'll modify this to be boolean in line with the others in AArch64.h

LukeGeeson updated this revision to Diff 256032.Apr 8 2020, 8:39 AM

added __ARM_FEATURE_MATMUL_INT8 check test

LukeGeeson marked 2 inline comments as done.Apr 8 2020, 8:40 AM
LukeGeeson added inline comments.
clang/lib/Basic/Targets/AArch64.cpp
284

test added in clang/test/CodeGen/aarch64-matmul.cpp

LukeGeeson edited the summary of this revision. (Show Details)Apr 8 2020, 8:44 AM
LukeGeeson added reviewers: t.p.northover, stuij.
LukeGeeson marked an inline comment as done.Apr 8 2020, 8:47 AM
fhahn added a comment.Apr 8 2020, 10:10 AM

This patch is quite big and I think it would be easier to review if it would be split up into distinct clang/llvm parts and maybe NEON/SVE parts on the LLVM side.

clang/lib/Driver/ToolChains/Arch/AArch64.cpp
71

Not sure if that is the right place to handle the implication. This function just decodes a given feature string. And shouldn't that already be handled in the backend by the implications in llvm/lib/Target/AArch64/AArch64.td?

This patch is quite big and I think it would be easier to review if it would be split up into distinct clang/llvm parts and maybe NEON/SVE parts on the LLVM side.

That's a good suggestion, I shouldn't have pushed my luck!

Luckily this patch doesn't introduce any new C types or IR types and so the frontend code is light, relatively speaking. That said, there is so little intrinsics work needed here that it might make sense to split the subsequent patches up to include intrinsics out-of-the-box as AArch64 asm+intrinsics, AArch32 asm+intrinsics, AArch64SVE asm + intrinsics etc... instead.

Does that sound ok?

fhahn added a comment.Apr 9 2020, 3:28 AM

This patch is quite big and I think it would be easier to review if it would be split up into distinct clang/llvm parts and maybe NEON/SVE parts on the LLVM side.

That's a good suggestion, I shouldn't have pushed my luck!

Luckily this patch doesn't introduce any new C types or IR types and so the frontend code is light, relatively speaking. That said, there is so little intrinsics work needed here that it might make sense to split the subsequent patches up to include intrinsics out-of-the-box as AArch64 asm+intrinsics, AArch32 asm+intrinsics, AArch64SVE asm + intrinsics etc... instead.

Does that sound ok?

Yeah that sounds good. In particular separating the clang/llvm changes would be helpful.

Hi folks,
Thank you for your help, I have split the patches up as follows:

  1. https://reviews.llvm.org/D77871 [AArch64] Armv8.6-a Matrix Mult Assembly + Intrinsics
  2. https://reviews.llvm.org/D77872 [AArch32] Armv8.6-a Matrix Mult Assembly + Intrinsics
  3. https://reviews.llvm.org/D77873 [AArch64] Armv8.6-A Mat Mul SVE Assembly
  4. https://reviews.llvm.org/D77874 [AArch32] Armv8.6a Matrix Mul Assembly
  5. https://reviews.llvm.org/D77875 [ARM] Armv8.6-a Matrix Mul cmd line support

In detail this covers the intrinsics (with minimum llvm backend needed to build and pass tests), SVE Assembly, (non-SVE) Assembly, and command-line support to enable the use of this extension. I mentioned above that there was so little intrinsics work needed and so the natural way to split up this patch was to include the llvm code needed to get each patch to build in the order above.

I'd appreciate any help you can give reviewing these!

LukeGeeson abandoned this revision.Apr 24 2020, 9:09 AM

Sub-issues all closed