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

[Victor Do Nascimento]

On 10/27/23 14:18, Alex Coplan wrote:
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.


For the record, I do mean accidental in the "subsidiary" sense of the 
word, if that makes sense.  They are primarily RSR operations associated 
with the mrs instruction.  The number of args simply follows on from 
this fact.

If in some hypothetical scenario, another single-operand expression was 
to make it to `aarch64_expand_rwsr_builtin' with an fcode not covered by 
those in `read_op', being single-operand alone wouldn't be a strong 
enough condition for it to be classified a "read system register" operation.

That's the scenario that is guarded against by Richard's comment a 
little further down:

"If we do go for my:

  call_expr_nargs (exp) == 1

suggestion above, this switch should probably have a:
this switch should probably have a:

  default:
    gcc_unreachable ();"

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?


In all fairness, you're 100% right here and I had thought about this in 
the past.  Assuming no stray fcode makes it into this function, read_op 
is just !write_op.  I did things this way to be nice and explicit early 
on abouut what consistutes a valid read operation.

Happy to simplify.

Cheers,
V.


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