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

[Luke Geeson via Phabricator via cfe-commits]

LukeGeeson abandoned this revision.
LukeGeeson added a comment.

Sub-issues all closed


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77540/new/

https://reviews.llvm.org/D77540



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Luke Geeson via Phabricator via cfe-commits]

LukeGeeson added a comment.

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!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77540/new/

https://reviews.llvm.org/D77540



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Florian Hahn via Phabricator via cfe-commits]

fhahn added a comment.

In D77540#1970034 , @LukeGeeson wrote:

> In D77540#1969873 , @fhahn wrote:
>
> > 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77540/new/

https://reviews.llvm.org/D77540



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Luke Geeson via Phabricator via cfe-commits]

LukeGeeson added a comment.

In D77540#1969873 , @fhahn wrote:

> 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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77540/new/

https://reviews.llvm.org/D77540



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Florian Hahn via Phabricator via cfe-commits]

fhahn added a comment.

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.




Comment at: clang/lib/Driver/ToolChains/Arch/AArch64.cpp:71
+
+// +sve implies +f32mm if the base architecture is v8.6A
+if ((ArchKind == llvm::AArch64::ArchKind::ARMV8_6A) && Feature == "sve")

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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77540/new/

https://reviews.llvm.org/D77540



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Luke Geeson via Phabricator via cfe-commits]

LukeGeeson marked 2 inline comments as done.
LukeGeeson added inline comments.



Comment at: clang/lib/Basic/Targets/AArch64.cpp:284
+  if (HasMatMul)
+Builder.defineMacro("__ARM_FEATURE_MATMUL_INT8", "1");
+

DavidSpickett wrote:
> I don't see specific tests for this #define
test added in clang/test/CodeGen/aarch64-matmul.cpp


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77540/new/

https://reviews.llvm.org/D77540



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Luke Geeson via Phabricator via cfe-commits]

LukeGeeson added inline comments.



Comment at: clang/lib/Basic/Targets/AArch64.h:39
   bool HasTME;
+  unsigned HasMatMul;
 

DavidSpickett wrote:
> Why is this feature a number for AArch64, does >1 mean something?
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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77540/new/

https://reviews.llvm.org/D77540



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added inline comments.



Comment at: clang/lib/Basic/Targets/AArch64.cpp:284
+  if (HasMatMul)
+Builder.defineMacro("__ARM_FEATURE_MATMUL_INT8", "1");
+

I don't see specific tests for this #define



Comment at: clang/lib/Basic/Targets/AArch64.h:39
   bool HasTME;
+  unsigned HasMatMul;
 

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77540/new/

https://reviews.llvm.org/D77540



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Luke Geeson via Phabricator via cfe-commits]

LukeGeeson added a comment.

build failures related to linting, unit tests passing


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77540/new/

https://reviews.llvm.org/D77540



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits