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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
