[PATCH] D128648: [Clang][AArch64][SME] Add vector read/write (mova) intrinsics

[Sander de Smalen via Phabricator via cfe-commits]

sdesmalen accepted this revision.
sdesmalen added a comment.

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.



================
Comment at: clang/test/CodeGen/aarch64-sme-intrinsics/acle_sme_read.c:22
+//
+svint8_t test_svread_hor_za8_s8(svint8_t zd, svbool_t pg, uint32_t slice_base) __arm_streaming {
+    return SME_ACLE_FUNC(svread_hor_za8, _s8, _m)(zd, pg, 0, slice_base, 0);
----------------
I think you've missed removing these?


================
Comment at: clang/test/CodeGen/aarch64-sme-intrinsics/acle_sme_read.c:13
+#else
+#define ARM_STREAMING_ATTR __attribute__((arm_streaming))
+#endif
----------------
bryanpkc wrote:
> sdesmalen wrote:
> > 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!
> @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.
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128648