Re: [PATCH v2 1/1] aarch64: Add SVE AES2 ACLE builtins

On Fri, Jul 10, 2026 at 10:37:10AM +0000, Sivan Shani wrote:
> Add ACLE support for the SVE AES2 multi-vector AES indexed
> instructions and the 128-bit PMULL/PMLAL pair forms.
> 
> This adds builtin shapes and expanders for svaese_lane, svaesd_lane,
> svaesemc_lane, svaesdimc_lane, svpmull_pair and svpmlal_pair, together
> with the corresponding RTL patterns and tests.  Also add preprocessor
> feature macros for SVE AES2 and SSVE AES.
> 
> gcc/
>         * config/aarch64/aarch64-c.cc (aarch64_update_cpp_builtins): Define
>         __ARM_FEATURE_SVE_AES2 and __ARM_FEATURE_SSVE_AES.
>         * config/aarch64/aarch64.h (TARGET_SVE_AES2): Define.
>         * config/aarch64/aarch64.md (UNSPEC_SSVE_LANE_SELECT): New unspec.
>         * config/aarch64/aarch64-sve-builtins-functions.h
>         (unspec_based_aes_lane_function): New typedef.
>         (unspec_based_aes_lane_mc_function): Likewise.
>         * config/aarch64/aarch64-sve-builtins-shapes.cc
>         (binary_to_pair_opt_n_def): New shape.
>         (ternary_to_pair_opt_n_def): Likewise.
>         (binary_aes_lane_def): Likewise.
>         * config/aarch64/aarch64-sve-builtins-shapes.h: Declare new shapes.
>         * config/aarch64/aarch64-sve-builtins-sve2.cc: Add new function
>         entries.
>         * config/aarch64/aarch64-sve-builtins-sve2.def: Add new ACLE
>         builtin definitions.
>         * config/aarch64/aarch64-sve-builtins-sve2.h: Declare new functions.
>         * config/aarch64/aarch64-sve2.md: Add PMULL/PMLAL pair and AES
>         indexed multi-vector patterns.
>         * config/aarch64/iterators.md: Add iterators and attrs for new
>         patterns.
>         * config/aarch64/predicates.md (const_0_to_3_operand): New predicate.
> 
> gcc/testsuite/
>         * gcc.target/aarch64/pragma_cpp_predefs_5.c: Test new feature macros.
>         * gcc.target/aarch64/sve/acle/asm/test_sve_acle.h
>         (TEST_XN_INDEXED): New macro.

You also changed TEST_XN_SINGLE without using it.  If you remove that hunk,
then I think this patch is ok for master.

I have a few more comments below, but I'm happy for those to be left unchanged.

>         * gcc.target/aarch64/sve2/acle/asm/aesd_lane_u8.c: New test.
>         * gcc.target/aarch64/sve2/acle/asm/aesdimc_lane_u8.c: New test.
>         * gcc.target/aarch64/sve2/acle/asm/aese_lane_u8.c: New test.
>         * gcc.target/aarch64/sve2/acle/asm/aesemc_lane_u8.c: New test.
>         * gcc.target/aarch64/sve2/acle/asm/pmlal_pair_u64.c: New test.
>         * gcc.target/aarch64/sve2/acle/asm/pmull_pair_u64.c: New test.
>         * lib/target-supports.exp: Add sve-aes2 assembler probe.

...

> +/* svuint8x2_t svaes<...>_lane[_u8_x2] (svuint8x2_t zdn, svuint8_t zm, uint64_t
> +   index);
> +   and
> +   svuint8x4_t svaes<...>_lane[_u8_x4] (svuint8x4_t zdn, svuint8_t zm, uint64_t
> +   index);
> +   When index is in range[0-3]
> +*/
> +struct binary_aes_lane_def : public overloaded_base<0>
> +{
> +  bool explicit_group_suffix_p () const override { return false; }
> +
> +  void
> +  build (function_builder &b, const function_group_info &group) const override
> +  {
> +    b.add_overloaded_functions (group, MODE_none);
> +    build_all (b, "t0,t0,v0,su64", group, MODE_none);
> +  }
> +
> +  tree
> +  resolve (function_resolver &r) const override
> +  {
> +    if (!r.check_num_arguments (3))
> +      return error_mark_node;
> +
> +    sve_type type = r.infer_sve_type (0);
> +    if (!type)
> +      return error_mark_node;
> +
> +    if (type.num_vectors != 2 && type.num_vectors != 4)
> +      return error_mark_node;

I think this check is unnecessary, and I think the function would be slightly
more readable written as a single if(... && ... && ... && ...).  However, it's
still fine as is.

> +
> +    if (!r.require_vector_type (1, VECTOR_TYPE_svuint8_t))
> +      return error_mark_node;
> +
> +    if (!r.require_integer_immediate (2))
> +      return error_mark_node;
> +
> +    return r.resolve_to (MODE_none, type);
> +  }

...

> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/acle/asm/test_sve_acle.h b/gcc/testsuite/gcc.target/aarch64/sve/acle/asm/test_sve_acle.h
> index aeb58ead0dd..586ddb45ae0 100644
> --- a/gcc/testsuite/gcc.target/aarch64/sve/acle/asm/test_sve_acle.h
> +++ b/gcc/testsuite/gcc.target/aarch64/sve/acle/asm/test_sve_acle.h
> @@ -687,6 +687,24 @@
>      __asm volatile ("" :: "w" (RES));				\
>    }
>  
> +#define TEST_XN_INDEXED(NAME, TTYPE, VTYPE, CODE1, CODE2)      \
> +       PROTO (NAME, TTYPE, (TTYPE t, VTYPE v)) \
> +       {       \
> +          register TTYPE z0 __asm ("z0");      \
> +          register TTYPE z1 __asm ("z1");      \
> +          register TTYPE z2 __asm ("z2");      \
> +          register TTYPE z3 __asm ("z3");      \

I'm a little wary of using overlapping register asms (since the tuples cover
multiple registers), and this was another reason why I suggested using
TEST_XN_SINGLE instead (which uses tuple register asms that are not
overlapping).  However, if the tests are passing then probably this is ok.


Incidentally, I don't really understand this rationale in your cover letter:
> I kept `TEST_XN_INDEXED` because it matches the intrinsic signature and the ABI
> register allocation, making it easier to write tests with the expected assembly
> for this class of instructions. It should also be useful for similar cases in
> the future.
but I'm ok with keeping the new macro.


> +          register VTYPE z4 __asm ("z4");      \
> +          register VTYPE z5 __asm ("z5");      \
> +          register VTYPE z6 __asm ("z6");      \
> +          register VTYPE z7 __asm ("z7");      \
> +          register uint64_t x0 __asm ("x0");   \

> +          __asm volatile ("" : "=r" (x0));				\
Why is this volatile asm needed?  I don't understand why it might be necessary,
but it should at least be harmless.

> +          INVOKE (CODE1, CODE2);       \
> +          __asm volatile ("" :: "w" (t));      \
> +          return t;    \
> +       }
> +
>  #define TEST_DUAL_XN(NAME, TTYPE1, TTYPE2, RES, CODE1, CODE2)	\
>    PROTO (NAME, void, ())					\
>    {								\
> @@ -716,6 +734,8 @@
>      __asm volatile ("" : "=w" (z0), "=w" (z1), "=w" (z5),	\
>  		    "=w" (z7), "=w" (z16), "=w" (z18),		\
>  		    "=w" (z23), "=w" (z24), "=w" (z28));	\
> +    register uint64_t x0 __asm ("x0");   \
> +    __asm volatile ("" : "=r" (x0));				\

This hunk should be dropped if you're not using TEST_XN_SINGLE in your patch.

>      INVOKE (RES = CODE1, RES = CODE2);				\
>      __asm volatile ("" :: "w" (RES));				\
>    }
>