On 26/10/2023 16:23, Richard Sandiford wrote:
> Victor Do Nascimento <victor.donascime...@arm.com> writes:
> > On 10/18/23 21:39, Richard Sandiford wrote:
> >> Victor Do Nascimento <victor.donascime...@arm.com> writes:
> >>> Implement the aarch64 intrinsics for reading and writing system
> >>> registers with the following signatures:
> >>>
> >>> uint32_t __arm_rsr(const char *special_register);
> >>> uint64_t __arm_rsr64(const char *special_register);
> >>> void* __arm_rsrp(const char *special_register);
> >>> float __arm_rsrf(const char *special_register);
> >>> double __arm_rsrf64(const char *special_register);
> >>> void __arm_wsr(const char *special_register, uint32_t value);
> >>> void __arm_wsr64(const char *special_register, uint64_t value);
> >>> void __arm_wsrp(const char *special_register, const void *value);
> >>> void __arm_wsrf(const char *special_register, float value);
> >>> void __arm_wsrf64(const char *special_register, double value);
> >>>
> >>> gcc/ChangeLog:
> >>>
> >>> * gcc/config/aarch64/aarch64-builtins.cc (enum aarch64_builtins):
> >>> Add enums for new builtins.
> >>> (aarch64_init_rwsr_builtins): New.
> >>> (aarch64_general_init_builtins): Call aarch64_init_rwsr_builtins.
> >>> (aarch64_expand_rwsr_builtin): New.
> >>> (aarch64_general_expand_builtin): Call aarch64_general_expand_builtin.
> >>> * gcc/config/aarch64/aarch64.md (read_sysregdi): New insn_and_split.
> >>> (write_sysregdi): Likewise.
> >>> * gcc/config/aarch64/arm_acle.h (__arm_rsr): New.
> >>> (__arm_rsrp): Likewise.
> >>> (__arm_rsr64): Likewise.
> >>> (__arm_rsrf): Likewise.
> >>> (__arm_rsrf64): Likewise.
> >>> (__arm_wsr): Likewise.
> >>> (__arm_wsrp): Likewise.
> >>> (__arm_wsr64): Likewise.
> >>> (__arm_wsrf): Likewise.
> >>> (__arm_wsrf64): Likewise.
> >>>
> >>> gcc/testsuite/ChangeLog:
> >>>
> >>> * gcc/testsuite/gcc.target/aarch64/acle/rwsr.c: New.
> >>> * gcc/testsuite/gcc.target/aarch64/acle/rwsr-1.c: Likewise.
> >>> ---
> >>> gcc/config/aarch64/aarch64-builtins.cc | 200 ++++++++++++++++++
> >>> gcc/config/aarch64/aarch64.md | 17 ++
> >>> gcc/config/aarch64/arm_acle.h | 30 +++
> >>> .../gcc.target/aarch64/acle/rwsr-1.c | 20 ++
> >>> gcc/testsuite/gcc.target/aarch64/acle/rwsr.c | 144 +++++++++++++
> >>> 5 files changed, 411 insertions(+)
> >>> create mode 100644 gcc/testsuite/gcc.target/aarch64/acle/rwsr-1.c
> >>> create mode 100644 gcc/testsuite/gcc.target/aarch64/acle/rwsr.c
> >>>
> >>> diff --git a/gcc/config/aarch64/aarch64-builtins.cc
> >>> b/gcc/config/aarch64/aarch64-builtins.cc
> >>> index 04f59fd9a54..d8bb2a989a5 100644
> >>> --- a/gcc/config/aarch64/aarch64-builtins.cc
> >>> +++ b/gcc/config/aarch64/aarch64-builtins.cc
> >>> @@ -808,6 +808,17 @@ enum aarch64_builtins
> >>> AARCH64_RBIT,
> >>> AARCH64_RBITL,
> >>> AARCH64_RBITLL,
> >>> + /* System register builtins. */
> >>> + AARCH64_RSR,
> >>> + AARCH64_RSRP,
> >>> + AARCH64_RSR64,
> >>> + AARCH64_RSRF,
> >>> + AARCH64_RSRF64,
> >>> + AARCH64_WSR,
> >>> + AARCH64_WSRP,
> >>> + AARCH64_WSR64,
> >>> + AARCH64_WSRF,
> >>> + AARCH64_WSRF64,
> >>> AARCH64_BUILTIN_MAX
> >>> };
> >>>
> >>> @@ -1798,6 +1809,65 @@ aarch64_init_rng_builtins (void)
> >>> AARCH64_BUILTIN_RNG_RNDRRS);
> >>> }
> >>>
> >>> +/* Add builtins for reading system register. */
> >>> +static void
> >>> +aarch64_init_rwsr_builtins (void)
> >>> +{
> >>> + tree fntype = NULL;
> >>> + tree const_char_ptr_type
> >>> + = build_pointer_type (build_type_variant (char_type_node, true,
> >>> false));
> >>> +
> >>> +#define AARCH64_INIT_RWSR_BUILTINS_DECL(F, N, T) \
> >>> + aarch64_builtin_decls[AARCH64_##F] \
> >>> + = aarch64_general_add_builtin ("__builtin_aarch64_"#N, T,
> >>> AARCH64_##F);
> >>> +
> >>> + fntype
> >>> + = build_function_type_list (uint32_type_node, const_char_ptr_type,
> >>> NULL);
> >>> + AARCH64_INIT_RWSR_BUILTINS_DECL (RSR, rsr, fntype);
> >>> +
> >>> + fntype
> >>> + = build_function_type_list (ptr_type_node, const_char_ptr_type,
> >>> NULL);
> >>> + AARCH64_INIT_RWSR_BUILTINS_DECL (RSRP, rsrp, fntype);
> >>> +
> >>> + fntype
> >>> + = build_function_type_list (uint64_type_node, const_char_ptr_type,
> >>> NULL);
> >>> + AARCH64_INIT_RWSR_BUILTINS_DECL (RSR64, rsr64, fntype);
> >>> +
> >>> + fntype
> >>> + = build_function_type_list (float_type_node, const_char_ptr_type,
> >>> NULL);
> >>> + AARCH64_INIT_RWSR_BUILTINS_DECL (RSRF, rsrf, fntype);
> >>> +
> >>> + fntype
> >>> + = build_function_type_list (double_type_node, const_char_ptr_type,
> >>> NULL);
> >>> + AARCH64_INIT_RWSR_BUILTINS_DECL (RSRF64, rsrf64, fntype);
> >>> +
> >>> + fntype
> >>> + = build_function_type_list (void_type_node, const_char_ptr_type,
> >>> + uint32_type_node, NULL);
> >>> +
> >>> + AARCH64_INIT_RWSR_BUILTINS_DECL (WSR, wsr, fntype);
> >>> +
> >>> + fntype
> >>> + = build_function_type_list (void_type_node, const_char_ptr_type,
> >>> + const_ptr_type_node, NULL);
> >>> + AARCH64_INIT_RWSR_BUILTINS_DECL (WSRP, wsrp, fntype);
> >>> +
> >>> + fntype
> >>> + = build_function_type_list (void_type_node, const_char_ptr_type,
> >>> + uint64_type_node, NULL);
> >>> + AARCH64_INIT_RWSR_BUILTINS_DECL (WSR64, wsr64, fntype);
> >>> +
> >>> + fntype
> >>> + = build_function_type_list (void_type_node, const_char_ptr_type,
> >>> + float_type_node, NULL);
> >>> + AARCH64_INIT_RWSR_BUILTINS_DECL (WSRF, wsrf, fntype);
> >>> +
> >>> + fntype
> >>> + = build_function_type_list (void_type_node, const_char_ptr_type,
> >>> + double_type_node, NULL);
> >>> + AARCH64_INIT_RWSR_BUILTINS_DECL (WSRF64, wsrf64, fntype);
> >>> +}
> >>> +
> >>> /* Initialize the memory tagging extension (MTE) builtins. */
> >>> struct
> >>> {
> >>> @@ -2019,6 +2089,8 @@ aarch64_general_init_builtins (void)
> >>> aarch64_init_rng_builtins ();
> >>> aarch64_init_data_intrinsics ();
> >>>
> >>> + aarch64_init_rwsr_builtins ();
> >>> +
> >>> tree ftype_jcvt
> >>> = build_function_type_list (intSI_type_node, double_type_node,
> >>> NULL);
> >>> aarch64_builtin_decls[AARCH64_JSCVT]
> >>> @@ -2599,6 +2671,123 @@ aarch64_expand_rng_builtin (tree exp, rtx target,
> >>> int fcode, int ignore)
> >>> return target;
> >>> }
> >>>
> >>> +/* Expand the read/write system register builtin EXPs. */
> >>> +rtx
> >>> +aarch64_expand_rwsr_builtin (tree exp, rtx target, int fcode)
> >>> +{
> >>> + tree arg0, arg1;
> >>> + rtx const_str, input_val, subreg;
> >>> + enum machine_mode mode;
> >>> + class expand_operand ops[2];
> >>> +
> >>> + arg0 = CALL_EXPR_ARG (exp, 0);
> >>> +
> >>> + bool write_op = (fcode == AARCH64_WSR
> >>> + || fcode == AARCH64_WSRP
> >>> + || fcode == AARCH64_WSR64
> >>> + || fcode == AARCH64_WSRF
> >>> + || fcode == AARCH64_WSRF64);
> >>> + bool read_op = (fcode == AARCH64_RSR
> >>> + || fcode == AARCH64_RSRP
> >>> + || fcode == AARCH64_RSR64
> >>> + || fcode == AARCH64_RSRF
> >>> + || fcode == AARCH64_RSRF64);
> >>
> >> Instead of this, how about using:
> >>
> >> if (call_expr_nargs (exp) == 1)
> >>
> >> for the read path?
> >>
> >
> > Personally, I don't like that. It's a `read_op' because of the fcode it
> > has, the fact they share the same number of arguments is accidental.
> > With this implementation, we outline very early on exactly what
> > constitutes both a valid write and a valid read operation.
>
> I don't agree that it's accidental. :) But fair enough, I won't push it.
FWIW I agree with Richard that it isn't accidental that reads have one argument
and writes have two, it's an inherent property of how the operations work.
I think it would be better to use call_expr_nargs (exp) == 1 as suggested
above.
IIUC, presumably the operation must either be a read or a write so having two
separate booleans (read_op and write_op) doesn't make much sense either?
>
> > I'm happy to change things around if you feel strongly enough about it
> > or see some material advantage to doing things your way that I've missed :).
Thanks,
Alex
