Re: [PATCH 2/2] aarch64: Add support for widening LDAPR instructions

[Andre Vieira (lists) via Gcc-patches]

Sorry for the late reply on this. I was wondering though why the check 
made sense. The way I see it, SI -> SI mode is either wrong or useless. 
So why not:
if it is wrong, error (gcc_assert?) so we know it was generated wrongly 
somehow and fix it;
if it is useless, still use this pattern as we avoid an extra 
instruction (doing useless work).

Unless, you expect the backend to be 'probing' for this and the way we 
tell it not to is to not implement any pattern that allows for this? But 
somehow that doesn't feel like the right approach...

On 17/11/2022 11:30, Kyrylo Tkachov wrote:

-----Original Message-----
From: Richard Sandiford <richard.sandif...@arm.com>
Sent: Tuesday, November 15, 2022 6:05 PM
To: Andre Simoes Dias Vieira <andre.simoesdiasvie...@arm.com>
Cc: gcc-patches@gcc.gnu.org; Kyrylo Tkachov <kyrylo.tkac...@arm.com>;
Richard Earnshaw <richard.earns...@arm.com>
Subject: Re: [PATCH 2/2] aarch64: Add support for widening LDAPR
instructions

"Andre Vieira (lists)" <andre.simoesdiasvie...@arm.com> writes:
Updated version of the patch to account for the testsuite changes in the
first patch.

On 10/11/2022 11:20, Andre Vieira (lists) via Gcc-patches wrote:
Hi,

This patch adds support for the widening LDAPR instructions.

Bootstrapped and regression tested on aarch64-none-linux-gnu.

OK for trunk?

2022-11-09  Andre Vieira  <andre.simoesdiasvie...@arm.com>
             Kyrylo Tkachov  <kyrylo.tkac...@arm.com>

gcc/ChangeLog:

         * config/aarch64/atomics.md
(*aarch64_atomic_load<ALLX:mode>_rcpc_zext): New pattern.
         (*aarch64_atomic_load<ALLX:mode>_rcpc_zext): Likewise.

gcc/testsuite/ChangeLog:

         * gcc.target/aarch64/ldapr-ext.c: New test.
diff --git a/gcc/config/aarch64/atomics.md
b/gcc/config/aarch64/atomics.md
index
dc5f52ee8a4b349c0d8466a16196f83604893cbb..9670bef7d8cb2b32c5146536
d806a7e8bdffb2e3 100644
--- a/gcc/config/aarch64/atomics.md
+++ b/gcc/config/aarch64/atomics.md
@@ -704,6 +704,28 @@
    }
  )

+(define_insn "*aarch64_atomic_load<ALLX:mode>_rcpc_zext"
+  [(set (match_operand:GPI 0 "register_operand" "=r")
+    (zero_extend:GPI
+      (unspec_volatile:ALLX
+        [(match_operand:ALLX 1 "aarch64_sync_memory_operand" "Q")
+         (match_operand:SI 2 "const_int_operand")]                 ;;
model
+       UNSPECV_LDAP)))]
+  "TARGET_RCPC"
+  "ldapr<ALLX:atomic_sfx>\t%<GPI:w>0, %1"
It would be good to add:

   <GPI:sizen> > <ALLX:sizen>

to the condition, so that we don't provide bogus SI->SI and DI->DI
extensions.  (They shouldn't be generated, but it's better not to provide
them anyway.)

I agree. I'm pushing the attached patch to trunk.

gcc/ChangeLog:

         * config/aarch64/atomics.md 
(*aarch64_atomic_load<ALLX:mode>_rcpc_zext):
         Add mode size check to condition.
         (*aarch64_atomic_load<ALLX:mode>_rcpc_sext): Likewise.

Thanks,
Richard

+)
+
+(define_insn "*aarch64_atomic_load<ALLX:mode>_rcpc_sext"
+  [(set (match_operand:GPI  0 "register_operand" "=r")
+    (sign_extend:GPI
+      (unspec_volatile:ALLX
+        [(match_operand:ALLX 1 "aarch64_sync_memory_operand" "Q")
+         (match_operand:SI 2 "const_int_operand")]                 ;;
model
+       UNSPECV_LDAP)))]
+  "TARGET_RCPC"
+  "ldaprs<ALLX:atomic_sfx>\t%<GPI:w>0, %1"
+)
+
  (define_insn "atomic_store<mode>"
    [(set (match_operand:ALLI 0 "aarch64_rcpc_memory_operand" "=Q,Ust")
      (unspec_volatile:ALLI
diff --git a/gcc/testsuite/gcc.target/aarch64/ldapr-ext.c
b/gcc/testsuite/gcc.target/aarch64/ldapr-ext.c
new file mode 100644
index
0000000000000000000000000000000000000000..aed27e06235b1d266decf11
745dacf94cc59e76d
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/ldapr-ext.c
@@ -0,0 +1,94 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -std=c99" } */
+/* { dg-final { check-function-bodies "**" "" "" } } */
+#include <stdatomic.h>
+
+#pragma GCC target "+rcpc"
+
+atomic_ullong u64;
+atomic_llong s64;
+atomic_uint u32;
+atomic_int s32;
+atomic_ushort u16;
+atomic_short s16;
+atomic_uchar u8;
+atomic_schar s8;
+
+#define TEST(name, ldsize, rettype)                                \
+rettype                                                            \
+test_##name (void)                                         \
+{                                                          \
+  return atomic_load_explicit (&ldsize, memory_order_acquire);     \
+}
+
+/*
+**test_u8_u64:
+**...
+** ldaprb  x0, \[x[0-9]+\]
+** ret
+*/
+
+TEST(u8_u64, u8, unsigned long long)
+
+/*
+**test_s8_s64:
+**...
+** ldaprsb x0, \[x[0-9]+\]
+** ret
+*/
+
+TEST(s8_s64, s8, long long)
+
+/*
+**test_u16_u64:
+**...
+** ldaprh  x0, \[x[0-9]+\]
+** ret
+*/
+
+TEST(u16_u64, u16, unsigned long long)
+
+/*
+**test_s16_s64:
+**...
+** ldaprsh x0, \[x[0-9]+\]
+** ret
+*/
+
+TEST(s16_s64, s16, long long)
+
+/*
+**test_u8_u32:
+**...
+** ldaprb  w0, \[x[0-9]+\]
+** ret
+*/
+
+TEST(u8_u32, u8, unsigned)
+
+/*
+**test_s8_s32:
+**...
+** ldaprsb w0, \[x[0-9]+\]
+** ret
+*/
+
+TEST(s8_s32, s8, int)
+
+/*
+**test_u16_u32:
+**...
+** ldaprh  w0, \[x[0-9]+\]
+** ret
+*/
+
+TEST(u16_u32, u16, unsigned)
+
+/*
+**test_s16_s32:
+**...
+** ldaprsh w0, \[x[0-9]+\]
+** ret
+*/
+
+TEST(s16_s32, s16, int)