Re: [PATCH V2 5/7] aarch64: Implement system register r/w arm ACLE intrinsic functions

[Victor Do Nascimento]

On 10/26/23 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.


Iff your objection is not so big so as to you letting me have my way, 
then I shall not complain!  Thanks.

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 :).

+
+  /* Argument 0 (system register name) must be a string literal.  */
+  gcc_assert (!SSA_VAR_P (arg0)
+             && TREE_CODE (TREE_TYPE (arg0)) == POINTER_TYPE
+             && TREE_CODE (TREE_OPERAND (arg0,0)) == STRING_CST);

It looks like this requires arg0 to be an ADDR_EXPR.  It would be good
to check that rather than !SSA_VAR_P.

Minor formatting nit: should be a space after the comma.

+
+  const char *name_input = TREE_STRING_POINTER (TREE_OPERAND (arg0, 0));
+  size_t len = strlen (name_input);

This can use TREE_STRING_LENGTH.  That's preferable since it copes
with embedded nuls, e.g. "foo\0bar".

I figured that `c_strlen', as defined in `builtins.cc' would be helpful 
here, having logic to handle "odd" strings, both those which contain an 
embedded null and those lacking a null terminator altogether, returning 
NULL_TREE in both scenarios.  This allows us to deal with embedded nulls 
early on, rejecting them straight away.

Any thoughts on this?

Many thanks,
V.


+
+  if (len == 0)
+    {
+      error_at (EXPR_LOCATION (exp), "invalid system register name provided");
+      return const0_rtx;
+    }

Is this check necessary?  It looks like the following code would
correctly reject empty strings.


Not necessary.  As the code evolved, some checks I added early on in
development went on to become redundant, this being on example.  Thanks
for picking up on it.

+
+  char *sysreg_name = (char *) alloca (len + 1);
+  strcpy (sysreg_name, name_input);

We shouldn't use alloca for user input that hasn't been verified yet,
since it could be arbitrarily long.  Libiberty provides an xmemdup that
might be useful.  (xmemdup rather than xstrdup to cope with the embedded
nuls mentioned above.  Or perhaps we should just reject embedded nuls
immediately, so that later code doesn't need to worry about them.)

+
+  for (unsigned pos = 0; pos <= len; pos++)
+    sysreg_name[pos] = TOLOWER (sysreg_name[pos]);
+
+  const char* name_output = aarch64_retrieve_sysreg (sysreg_name, write_op);

Does this TOLOWERing make the checks for capital letters in
is_implem_def_reg redundant?


Potentially.

The quesion is, do we want  this explicit dependence of
`is_implem_def_reg' on this TOLOWERing?

My intention was to keep things fairly independent so they could be
reused elsewhere as easily as possible, should this need arise.  This
meant I didn't want `is_implem_def_reg' to rely on the passed input
having been pre-processed in any way, keeping it as general-purpose as
possible.

I think it's good if we have a canonical form internally.  And it looks
like the patches do have a canonical form, thanks to these TOLOWER calls.
IMO it's better to embrace that rather than support the possibility of
having multiple representations of the same thing in future.

Also, aarch64_lookup_sysreg_map requires the argument to be lower case
if it specifies a predefined register name, since we (rightly) don't
add every case variation of a register name to the hash table.  So it
seems more consistent to have the same precondition for both types of
register name, rather than requiring one to be lower case and allowing
the other to be mixed case.


I see your point!

Thanks,
Richard