This is an archive of the discontinued LLVM Phabricator instance.

[Clang][AArch64][SME] Add vector read/write (mova) intrinsics
ClosedPublic

Authored by bryanpkc on Jun 27 2022, 7:15 AM.

Details

Summary

This patch adds support for the following SME ACLE intrinsics (as defined
in https://arm-software.github.io/acle/main/acle.html):

  • svread_hor_za8[_s8]_m // also for u8
  • svread_hor_za16[_s16]_m // also for u16, f16, bf16
  • svread_hor_za32[_s32]_m // also for u32, f32
  • svread_hor_za64[_s64]_m // also for u64, f64
  • svread_hor_za128[_s8]_m // also for s16, s32, s64, u8, u16, u32, u64, bf16, f16, f32, f64
  • svread_ver_za8[_s8]_m // also for u8
  • svread_ver_za16[_s16]_m // also for u16, f16, bf16
  • svread_ver_za32[_s32]_m // also for u32, f32
  • svread_ver_za64[_s64]_m // also for u64, f64
  • svread_ver_za128[_s8]_m // also for s16, s32, s64, u8, u16, u32, u64, bf16, f16, f32, f64
  • svwrite_hor_za8[_s8]_m // also for u8
  • svwrite_hor_za16[_s16]_m // also for u16, f16, bf16
  • svwrite_hor_za32[_s32]_m // also for u32, f32
  • svwrite_hor_za64[_s64]_m // also for u64, f64
  • svwrite_hor_za128[_s8]_m // also for s16, s32, s64, u8, u16, u32, u64, bf16, f16, f32, f64
  • svwrite_ver_za8[_s8]_m // also for u8
  • svwrite_ver_za16[_s16]_m // also for u16, f16, bf16
  • svwrite_ver_za32[_s32]_m // also for u32, f32
  • svwrite_ver_za64[_s64]_m // also for u64, f64
  • svwrite_ver_za128[_s8]_m // also for s16, s32, s64, u8, u16, u32, u64, bf16, f16, f32, f64

Co-authored-by: Sagar Kulkarni <sagar.kulkarni1@huawei.com>

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJun 27 2022, 7:15 AM
sagarkulkarni19 requested review of this revision.Jun 27 2022, 7:15 AM
  • Decorate intrinsics with SME attributes.
  • Update testcases by adding "arm_streaming" attribute to the callee.
bryanpkc commandeered this revision.Jan 8 2023, 6:36 PM
bryanpkc added a reviewer: sagarkulkarni19.
bryanpkc updated this revision to Diff 487263.Jan 8 2023, 7:29 PM
bryanpkc retitled this revision from [Clang][AArch64] Add SME C intrinsics for read and write (mova) to [Clang][AArch64][SME] Add intrinsics for vector read/write (mova).
bryanpkc edited the summary of this revision. (Show Details)

Rebased on the new D127910.

bryanpkc updated this revision to Diff 487267.Jan 8 2023, 7:53 PM
bryanpkc edited the summary of this revision. (Show Details)

Re-upload to add diff context. No change to the patch itself.

bryanpkc updated this revision to Diff 487381.Jan 9 2023, 4:22 AM

Removed incorrect IsPreservesZA flag from SVWRITE intrinsics.

bryanpkc updated this revision to Diff 492678.Jan 27 2023, 2:28 AM
bryanpkc retitled this revision from [Clang][AArch64][SME] Add intrinsics for vector read/write (mova) to [Clang][AArch64][SME] Add vector read/write (mova) intrinsics .
bryanpkc edited the summary of this revision. (Show Details)

Update patch with more context.

bryanpkc updated this revision to Diff 500639.Feb 26 2023, 5:42 PM
bryanpkc edited the summary of this revision. (Show Details)

Incorporated review comments for D127910:

  • Added range checks for immediate operands.
  • Refactored the TableGen definitions using a multiclass.
  • Split IsMove flag into IsRead and IsWrite, and used them to avoid relying on LLVM intrinsic names during IR generation.

Hi @bryanpkc, thank you for updating this patch & applying the previous review comments here too.
I just have a couple of minor suggestions:

clang/include/clang/Basic/arm_sme.td
103

This is only a suggestion, but would it make the multiclasses simpler to just pass in either "q" or "" depending on the instruction, and append this to aarch64_sme_read/write?

clang/test/CodeGen/aarch64-sme-intrinsics/acle_sme_read.c
3

I think -target-feature +sve can be removed from this test and acle_sme_write.c

bryanpkc added inline comments.Mar 1 2023, 9:53 AM
clang/include/clang/Basic/arm_sme.td
103

Thanks for the suggestion, but simplifying the definition at the cost of complicating the interface does not seem worthwhile. I think the current implementation is more self-documenting and clearer about the intent, and reduces the cognitive burden for a future reader, to whom the uses of "" and "q" may not be obvious.

clang/test/CodeGen/aarch64-sme-intrinsics/acle_sme_read.c
3

Doing that will cause errors like these:

error: SVE vector type 'svbool_t' (aka '__SVBool_t') cannot be used in a target without sve

As I have explained in D127910, -target-feature +sme does not imply -target-feature +sve. But -march= processing will work as expected when D142702 lands.

Matt added a subscriber: Matt.Mar 3 2023, 2:34 PM
kmclaughlin accepted this revision.Mar 15 2023, 10:56 AM

Thank you @bryanpkc, this LGTM

clang/test/CodeGen/aarch64-sme-intrinsics/acle_sme_read.c
3

Apologies for leaving the same comment, I missed this on the previous patch

This revision is now accepted and ready to land.Mar 15 2023, 10:56 AM
bryanpkc updated this revision to Diff 522065.May 15 2023, 12:06 AM

Rebased on main, and removed dependence on the SME attributes patch as per https://reviews.llvm.org/D127910?vs=522039&id=522064#inline-1451681.

@sdesmalen Could you review this and the rest of the patch stack?

Other than the two nits, the patch looks good. Thanks.

clang/include/clang/Basic/arm_sme.td
82

I'd prefer to just pass in the LLVM IR intrinsic as a parameter to ZARead than doing this, because that makes it easier to see from the definitions what intrinsic the builtin maps to. (I have found myself grepping in these files quite regularly)

clang/include/clang/Basic/arm_sve_sme_incl.td
223

nit: IsReadZA ? (and IsWriteZA)

bryanpkc updated this revision to Diff 532430.Jun 17 2023, 3:55 PM
bryanpkc marked an inline comment as done.

Incorporated@sdesmalen's suggestions. Thanks for the review.

bryanpkc marked an inline comment as done.Jun 17 2023, 3:57 PM

@sdesmalen Could you take a look at the patch stack again?

sdesmalen added inline comments.Jul 3 2023, 1:04 AM
clang/test/CodeGen/aarch64-sme-intrinsics/acle_sme_read.c
14

The spelling has recently changed to the __arm_streaming. Also with the new attribute keywords, the position of the attributes is more strict and need sto be after the function arguments (e.g. svint8_t test_svread_..(...) ARM_STREAMING_ATTR {)

Sorry if I previously gave you the wrong steer here to add these macros, but I've changed my mind and think it's better to remove them for now. That means we won't have any streaming attributes on the functions in the tests, but we can (and will need to) add those when we add diagnostics for missing attributes, for example when using a shared ZA intrinsic when the function misses __arm_shared_za/__arm_new_za, or when using a streaming intrinsic when the function is not __arm_streaming. Writing this also made me realise the functions below would be missing __arm_shared_za attributes.

Can you remove these macros from the patches? Again, my apologies for the wrong steer!

bryanpkc updated this revision to Diff 540837.Jul 16 2023, 5:19 PM

Removed the ARM_STREAMING_ATTR macro from the tests as requested.

bryanpkc marked an inline comment as done.Jul 16 2023, 5:24 PM
bryanpkc added inline comments.
clang/test/CodeGen/aarch64-sme-intrinsics/acle_sme_read.c
14

@sdesmalen Thanks for the clarification and sorry for the late reply as I was on vacation. I have updated the patch as you suggested. I can also look into adding support for more attributes and corresponding semantic checks, if you guys haven't started already.

sdesmalen accepted this revision.Jul 17 2023, 6:42 AM

Patch LGTM, thanks.

Just a note that you'll need to remove the trailing __arm_streaming attribute from acle_sme_read.c in order to be able to land the patch without causing test failures.

clang/test/CodeGen/aarch64-sme-intrinsics/acle_sme_read.c
14

Thanks for the clarification and sorry for the late reply as I was on vacation. I have updated the patch as you suggested.

No worries, thanks for updating your patches!

I can also look into adding support for more attributes and corresponding semantic checks, if you guys haven't started already.

We've actually already built support for semantic checks on streaming/ZA attributes, I'll share some patches for that soon.

23

I think you've missed removing these?

bryanpkc updated this revision to Diff 541001.Jul 17 2023, 6:49 AM
bryanpkc marked an inline comment as done.

Fixed the acle_sme_read.c test case.

bryanpkc marked an inline comment as done.Jul 17 2023, 6:50 AM
bryanpkc added inline comments.
clang/test/CodeGen/aarch64-sme-intrinsics/acle_sme_read.c
23

You are right, my bad. Fixed.

sdesmalen accepted this revision.Jul 17 2023, 6:53 AM

Thanks!

This revision was landed with ongoing or failed builds.Jul 20 2023, 2:57 AM
This revision was automatically updated to reflect the committed changes.
bryanpkc marked an inline comment as done.