[PATCH] AArch64: Add ACLE MOPS support

[PATCH] AArch64: Add ACLE MOPS support

[Wilco Dijkstra]

Add __ARM_FEATURE_MOPS predefine.  Add support for ACLE __arm_mops_memset_tag.

Passes regress, OK for commit?

gcc:
        * config/aaarch64/aarch64-c.cc (aarch64_update_cpp_builtins):
        Add __ARM_FEATURE_MOPS predefine.
        * config/aarch64/arm_acle.h: Add __arm_mops_memset_tag().

gcc/testsuite:
        * gcc.target/aarch64/acle/memtag_5.c: Add new test.

---

diff --git a/gcc/config/aarch64/aarch64-c.cc b/gcc/config/aarch64/aarch64-c.cc
index fe1a20e4e546a68e5f7eddff3bbb0d3e831fbd9b..884a7ba5d10b58fbe182a765041cf80bdaec9615 100644
--- a/gcc/config/aarch64/aarch64-c.cc
+++ b/gcc/config/aarch64/aarch64-c.cc
@@ -260,6 +260,7 @@ aarch64_update_cpp_builtins (cpp_reader *pfile)
   aarch64_def_or_undef (TARGET_SME_I16I64, "__ARM_FEATURE_SME_I16I64", pfile);
   aarch64_def_or_undef (TARGET_SME_F64F64, "__ARM_FEATURE_SME_F64F64", pfile);
   aarch64_def_or_undef (TARGET_SME2, "__ARM_FEATURE_SME2", pfile);
+  aarch64_def_or_undef (TARGET_MOPS, "__ARM_FEATURE_MOPS", pfile);
 
   /* Not for ACLE, but required to keep "float.h" correct if we switch
      target between implementations that do or do not support ARMv8.2-A
diff --git a/gcc/config/aarch64/arm_acle.h b/gcc/config/aarch64/arm_acle.h
index 2aa681090fa205449cf1ac63151565f960716189..22ee4b211a55ca6537a1d9e3bf4dad09585071fb 100644
--- a/gcc/config/aarch64/arm_acle.h
+++ b/gcc/config/aarch64/arm_acle.h
@@ -344,6 +344,21 @@ __rndrrs (uint64_t *__res)
 
 #pragma GCC pop_options
 
+#if defined (__ARM_FEATURE_MOPS) && defined (__ARM_FEATURE_MEMORY_TAGGING)
+__extension__ extern __inline void *
+__attribute__ ((__always_inline__, __gnu_inline__, __artificial__))
+__arm_mops_memset_tag (void *__ptr, int __val, size_t __size)
+{
+  void *__ptr2 = __ptr;
+  __asm volatile ("setgp\t[%0]!, %1!, %x2\n\t"
+		  "setgm\t[%0]!, %1!, %x2\n\t"
+		  "setge\t[%0]!, %1!, %x2"
+		  : "+r" (__ptr2), "+r" (__size)
+		  : "rZ" (__val) : "cc", "memory");
+  return __ptr;
+}
+#endif
+
 #define __arm_rsr(__regname) \
   __builtin_aarch64_rsr (__regname)
 
diff --git a/gcc/testsuite/gcc.target/aarch64/acle/memtag_5.c b/gcc/testsuite/gcc.target/aarch64/acle/memtag_5.c
new file mode 100644
index 0000000000000000000000000000000000000000..79ba1eb39d7c6d577fbe98a3285f8cc618428823
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/acle/memtag_5.c
@@ -0,0 +1,22 @@
+/* { dg-do compile } */
+/* { dg-options "-march=armv8.8-a+memtag -O2" } */
+/* { dg-final { check-function-bodies "**" "" "" { target lp64 } } } */
+
+#include "arm_acle.h"
+
+#ifndef __ARM_FEATURE_MOPS
+# error __ARM_FEATURE_MOPS not defined!
+#endif
+
+/*
+** set_tag:
+**	mov	(x[0-9]+), x0
+**	setgp	\[\1\]\!, x1\!, xzr
+**	setgm	\[\1\]\!, x1\!, xzr
+**	setge	\[\1\]\!, x1\!, xzr
+**	ret
+*/
+void *set_tag (void *p, size_t size)
+{
+  return __arm_mops_memset_tag (p, 0, size);
+}

Re: [PATCH] AArch64: Add ACLE MOPS support

[Richard Sandiford]

Wilco Dijkstra <Wilco.Dijkstra@arm.com> writes:
> Add __ARM_FEATURE_MOPS predefine.  Add support for ACLE __arm_mops_memset_tag.
>
> Passes regress, OK for commit?
>
> gcc:
>         * config/aaarch64/aarch64-c.cc (aarch64_update_cpp_builtins):
>         Add __ARM_FEATURE_MOPS predefine.
>         * config/aarch64/arm_acle.h: Add __arm_mops_memset_tag().
>
> gcc/testsuite:
>         * gcc.target/aarch64/acle/memtag_5.c: Add new test.
>
> ---
>
> diff --git a/gcc/config/aarch64/aarch64-c.cc b/gcc/config/aarch64/aarch64-c.cc
> index fe1a20e4e546a68e5f7eddff3bbb0d3e831fbd9b..884a7ba5d10b58fbe182a765041cf80bdaec9615 100644
> --- a/gcc/config/aarch64/aarch64-c.cc
> +++ b/gcc/config/aarch64/aarch64-c.cc
> @@ -260,6 +260,7 @@ aarch64_update_cpp_builtins (cpp_reader *pfile)
>    aarch64_def_or_undef (TARGET_SME_I16I64, "__ARM_FEATURE_SME_I16I64", pfile);
>    aarch64_def_or_undef (TARGET_SME_F64F64, "__ARM_FEATURE_SME_F64F64", pfile);
>    aarch64_def_or_undef (TARGET_SME2, "__ARM_FEATURE_SME2", pfile);
> +  aarch64_def_or_undef (TARGET_MOPS, "__ARM_FEATURE_MOPS", pfile);
>  
>    /* Not for ACLE, but required to keep "float.h" correct if we switch
>       target between implementations that do or do not support ARMv8.2-A
> diff --git a/gcc/config/aarch64/arm_acle.h b/gcc/config/aarch64/arm_acle.h
> index 2aa681090fa205449cf1ac63151565f960716189..22ee4b211a55ca6537a1d9e3bf4dad09585071fb 100644
> --- a/gcc/config/aarch64/arm_acle.h
> +++ b/gcc/config/aarch64/arm_acle.h
> @@ -344,6 +344,21 @@ __rndrrs (uint64_t *__res)
>  
>  #pragma GCC pop_options
>  
> +#if defined (__ARM_FEATURE_MOPS) && defined (__ARM_FEATURE_MEMORY_TAGGING)
> +__extension__ extern __inline void *
> +__attribute__ ((__always_inline__, __gnu_inline__, __artificial__))
> +__arm_mops_memset_tag (void *__ptr, int __val, size_t __size)
> +{
> +  void *__ptr2 = __ptr;
> +  __asm volatile ("setgp\t[%0]!, %1!, %x2\n\t"
> +		  "setgm\t[%0]!, %1!, %x2\n\t"
> +		  "setge\t[%0]!, %1!, %x2"
> +		  : "+r" (__ptr2), "+r" (__size)
> +		  : "rZ" (__val) : "cc", "memory");
> +  return __ptr;
> +}
> +#endif
> +

I think this should be in a push_options/pop_options block, as for other
intrinsics that require certain features.

What was the reason for using an inline asm rather than a builtin?
Feels a bit old school. :)  Using a builtin should mean that the
RTL optimisers see the extent of the write.

Thanks,
Richard

>  #define __arm_rsr(__regname) \
>    __builtin_aarch64_rsr (__regname)
>  
> diff --git a/gcc/testsuite/gcc.target/aarch64/acle/memtag_5.c b/gcc/testsuite/gcc.target/aarch64/acle/memtag_5.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..79ba1eb39d7c6d577fbe98a3285f8cc618428823
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/acle/memtag_5.c
> @@ -0,0 +1,22 @@
> +/* { dg-do compile } */
> +/* { dg-options "-march=armv8.8-a+memtag -O2" } */
> +/* { dg-final { check-function-bodies "**" "" "" { target lp64 } } } */
> +
> +#include "arm_acle.h"
> +
> +#ifndef __ARM_FEATURE_MOPS
> +# error __ARM_FEATURE_MOPS not defined!
> +#endif
> +
> +/*
> +** set_tag:
> +**	mov	(x[0-9]+), x0
> +**	setgp	\[\1\]\!, x1\!, xzr
> +**	setgm	\[\1\]\!, x1\!, xzr
> +**	setge	\[\1\]\!, x1\!, xzr
> +**	ret
> +*/
> +void *set_tag (void *p, size_t size)
> +{
> +  return __arm_mops_memset_tag (p, 0, size);
> +}

Re: [PATCH] AArch64: Add ACLE MOPS support

[Wilco Dijkstra]

Hi Richard,

> I think this should be in a push_options/pop_options block, as for other
> intrinsics that require certain features.

But then the intrinsic would always be defined, which is contrary to what the
ACLE spec demands - it would not give a compilation error at the callsite
but give assembler errors (potentially in different functions after inlining).

> What was the reason for using an inline asm rather than a builtin?
> Feels a bit old school. :)  Using a builtin should mean that the
> RTL optimisers see the extent of the write.

Given this intrinsic will be used very rarely, if ever, it does not make sense
to provide anything more than the basic functionality.

Cheers,
Wilco

Re: [PATCH] AArch64: Add ACLE MOPS support

[Kyrill Tkachov]

[-- Attachment #1: Type: text/plain, Size: 1275 bytes --]

Hi Wilco,

On Fri, May 31, 2024 at 6:38 PM Wilco Dijkstra <Wilco.Dijkstra@arm.com>
wrote:

> Hi Richard,
>
> > I think this should be in a push_options/pop_options block, as for other
> > intrinsics that require certain features.
>
> But then the intrinsic would always be defined, which is contrary to what
> the
> ACLE spec demands - it would not give a compilation error at the callsite
> but give assembler errors (potentially in different functions after
> inlining).
>
> What was the reason for using an inline asm rather than a builtin?
> > Feels a bit old school. :)  Using a builtin should mean that the
> > RTL optimisers see the extent of the write.
>
> Given this intrinsic will be used very rarely, if ever, it does not make
> sense
> to provide anything more than the basic functionality.
>

I agree that it's unlikely to get much use.
IMO we should be moving the arm_acle.h header to be implemented in
the #pragma GCC aarch64 "arm_acle.h" at the top as much as possible.
So I'd expect handle_arm_acle_h to be extended to inject these definitions
when appropriate and during expansion it'd just generate the RTL pattern
for it, which needn't be exposed as an implementation-defined builtin.
Thanks,
Kyrill

Cheers,
> Wilco

Re: [PATCH] AArch64: Add ACLE MOPS support

[Richard Sandiford]

Wilco Dijkstra <Wilco.Dijkstra@arm.com> writes:
> Hi Richard,
>
>> I think this should be in a push_options/pop_options block, as for other
>> intrinsics that require certain features.
>
> But then the intrinsic would always be defined, which is contrary to what the
> ACLE spec demands - it would not give a compilation error at the callsite
> but give assembler errors (potentially in different functions after inlining).

Inlining will fail with an error if the callsite doesn't have the right
features.  E.g.: https://godbolt.org/z/7zz59PhTE

The error message isn't great, but it is at least an error. :)

>> What was the reason for using an inline asm rather than a builtin?
>> Feels a bit old school. :)  Using a builtin should mean that the
>> RTL optimisers see the extent of the write.
>
> Given this intrinsic will be used very rarely, if ever, it does not make sense
> to provide anything more than the basic functionality.

But a lot of effort went into making the old inline asm ACLE
implementations use builtins instead.  It even seems to have been
a complete transition.  (Although we still have:

/* Start of temporary inline asm implementations.  */
...
/* End of temporary inline asm.  */

heh.)

So this feels like a regression in terms of implementation methodology.

I won't object if another maintainer approves the function in this form,
but I'd only be comfortable approving a builtin version.

Thanks,
Richard