[PATCH] D128648: [Clang][AArch64][SME] Add vector read/write (mova) intrinsics
sdesmalen accepted this revision. sdesmalen added a comment. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128648/new/ https://reviews.llvm.org/D128648 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D128648: [Clang][AArch64][SME] Add vector read/write (mova) intrinsics
bryanpkc marked an inline comment as done.
bryanpkc added inline comments.
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);
sdesmalen wrote:
> I think you've missed removing these?
You are right, my bad. Fixed.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D128648/new/
https://reviews.llvm.org/D128648
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D128648: [Clang][AArch64][SME] Add vector read/write (mova) intrinsics
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
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D128648: [Clang][AArch64][SME] Add vector read/write (mova) intrinsics
bryanpkc marked an inline comment as done.
bryanpkc added inline comments.
Comment at: clang/test/CodeGen/aarch64-sme-intrinsics/acle_sme_read.c:13
+#else
+#define ARM_STREAMING_ATTR __attribute__((arm_streaming))
+#endif
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D128648/new/
https://reviews.llvm.org/D128648
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D128648: [Clang][AArch64][SME] Add vector read/write (mova) intrinsics
sdesmalen added inline comments.
Comment at: clang/test/CodeGen/aarch64-sme-intrinsics/acle_sme_read.c:13
+#else
+#define ARM_STREAMING_ATTR __attribute__((arm_streaming))
+#endif
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!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D128648/new/
https://reviews.llvm.org/D128648
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D128648: [Clang][AArch64][SME] Add vector read/write (mova) intrinsics
bryanpkc added a comment. @sdesmalen Could you take a look at the patch stack again? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128648/new/ https://reviews.llvm.org/D128648 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D128648: [Clang][AArch64][SME] Add vector read/write (mova) intrinsics
sdesmalen added a comment.
Other than the two nits, the patch looks good. Thanks.
Comment at: clang/include/clang/Basic/arm_sme.td:82
+def NAME # _H : SInst<"svread_hor_" # n_suffix # "[_{d}]", "ddPimi", t,
MergeOp1,
+ "aarch64_sme_read" # !cond(!eq(n_suffix, "za128") :
"q", true: "") # "_horiz",
+ [IsRead, IsStreaming, IsSharedZA, IsPreservesZA],
ch>;
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)
Comment at: clang/include/clang/Basic/arm_sve_sme_incl.td:219
def IsPreservesZA : FlagType<0x100>;
+def IsRead: FlagType<0x200>;
+def IsWrite : FlagType<0x400>;
nit: IsReadZA ? (and IsWriteZA)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D128648/new/
https://reviews.llvm.org/D128648
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D128648: [Clang][AArch64][SME] Add vector read/write (mova) intrinsics
bryanpkc added a comment. @sdesmalen Could you review this and the rest of the patch stack? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128648/new/ https://reviews.llvm.org/D128648 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D128648: [Clang][AArch64][SME] Add vector read/write (mova) intrinsics
bryanpkc added a comment. Rebased on main, and removed dependence on the SME attributes patch as per https://reviews.llvm.org/D127910?vs=522039=522064#inline-1451681. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128648/new/ https://reviews.llvm.org/D128648 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D128648: [Clang][AArch64][SME] Add vector read/write (mova) intrinsics
kmclaughlin accepted this revision. kmclaughlin added a comment. This revision is now accepted and ready to land. Thank you @bryanpkc, this LGTM Comment at: clang/test/CodeGen/aarch64-sme-intrinsics/acle_sme_read.c:2 +// REQUIRES: aarch64-registered-target +// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sme -target-feature +sve -S -O1 -Werror -emit-llvm -o - %s | FileCheck %s -check-prefixes=CHECK,CHECK-C +// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sme -target-feature +sve -S -O1 -Werror -emit-llvm -o - -x c++ %s | FileCheck %s -check-prefixes=CHECK,CHECK-CXX bryanpkc wrote: > kmclaughlin wrote: > > I think `-target-feature +sve` can be removed from this test and > > `acle_sme_write.c` > 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](https://reviews.llvm.org/D127910#4137844), > `-target-feature +sme` does not imply `-target-feature +sve`. But `-march=` > processing will work as expected when D142702 lands. Apologies for leaving the same comment, I missed this on the previous patch Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128648/new/ https://reviews.llvm.org/D128648 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D128648: [Clang][AArch64][SME] Add vector read/write (mova) intrinsics
bryanpkc added inline comments.
Comment at: clang/include/clang/Basic/arm_sme.td:103
+def NAME # _H : SInst<"svwrite_hor_" # n_suffix # "[_{d}]", "vimiPd", t,
MergeOp1,
+ "aarch64_sme_write" # !cond(!eq(n_suffix, "za128") :
"q", true: "") # "_horiz",
+ [IsWrite, IsStreaming, IsSharedZA], ch>;
kmclaughlin wrote:
> 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`?
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.
Comment at: clang/test/CodeGen/aarch64-sme-intrinsics/acle_sme_read.c:2
+// REQUIRES: aarch64-registered-target
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sme
-target-feature +sve -S -O1 -Werror -emit-llvm -o - %s | FileCheck %s
-check-prefixes=CHECK,CHECK-C
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sme
-target-feature +sve -S -O1 -Werror -emit-llvm -o - -x c++ %s | FileCheck %s
-check-prefixes=CHECK,CHECK-CXX
kmclaughlin wrote:
> I think `-target-feature +sve` can be removed from this test and
> `acle_sme_write.c`
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](https://reviews.llvm.org/D127910#4137844),
`-target-feature +sme` does not imply `-target-feature +sve`. But `-march=`
processing will work as expected when D142702 lands.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D128648/new/
https://reviews.llvm.org/D128648
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D128648: [Clang][AArch64][SME] Add vector read/write (mova) intrinsics
kmclaughlin added a comment.
Hi @bryanpkc, thank you for updating this patch & applying the previous review
comments here too.
I just have a couple of minor suggestions:
Comment at: clang/include/clang/Basic/arm_sme.td:103
+def NAME # _H : SInst<"svwrite_hor_" # n_suffix # "[_{d}]", "vimiPd", t,
MergeOp1,
+ "aarch64_sme_write" # !cond(!eq(n_suffix, "za128") :
"q", true: "") # "_horiz",
+ [IsWrite, IsStreaming, IsSharedZA], ch>;
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`?
Comment at: clang/test/CodeGen/aarch64-sme-intrinsics/acle_sme_read.c:2
+// REQUIRES: aarch64-registered-target
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sme
-target-feature +sve -S -O1 -Werror -emit-llvm -o - %s | FileCheck %s
-check-prefixes=CHECK,CHECK-C
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sme
-target-feature +sve -S -O1 -Werror -emit-llvm -o - -x c++ %s | FileCheck %s
-check-prefixes=CHECK,CHECK-CXX
I think `-target-feature +sve` can be removed from this test and
`acle_sme_write.c`
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D128648/new/
https://reviews.llvm.org/D128648
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
