| Message ID | b5c31297-4b0e-aaf5-227d-d69dbafb7e24@arm.com |
|---|---|
| State | Committed |
| Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 26A93385828E for <patchwork@sourceware.org>; Thu, 10 Nov 2022 11:20:41 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 26A93385828E DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1668079241; bh=Itxr0i/22m7afyvxb8nIOVKnxTyxf/gGFvAldHLkaY0=; h=Date:Subject:To:Cc:References:In-Reply-To:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=rFSx4UTNv6/+3whorVJpiVu9+eq17ovDJfk8apN9pXjeeYPs9uc6Etb9YwCNoTnXJ 5cY1A+PNjP8/LnjQ8yX6sEtC2pZZJZp+keggmdsMZsrmRNSZxRUsMJM6wZV5AsdEAb mtGugj/QL3H7hIrPSd/hGA8C7XfHGepuz8qp+wLA= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id 3FD853858D20 for <gcc-patches@gcc.gnu.org>; Thu, 10 Nov 2022 11:20:08 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 3FD853858D20 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 1F7581FB; Thu, 10 Nov 2022 03:20:14 -0800 (PST) Received: from [10.57.4.81] (unknown [10.57.4.81]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 3D9C93F534; Thu, 10 Nov 2022 03:20:07 -0800 (PST) Content-Type: multipart/mixed; boundary="------------2HF6rwGEQZHnY0WqXVi5W4Ww" Message-ID: <b5c31297-4b0e-aaf5-227d-d69dbafb7e24@arm.com> Date: Thu, 10 Nov 2022 11:20:01 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.4.0 Subject: [PATCH 2/2] aarch64: Add support for widening LDAPR instructions Content-Language: en-US To: "gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org> Cc: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>, Richard Earnshaw <Richard.Earnshaw@arm.com>, Richard Sandiford <richard.sandiford@arm.com> References: <e3d17147-aa74-aa6a-a435-2c8445e5b03b@arm.com> In-Reply-To: <e3d17147-aa74-aa6a-a435-2c8445e5b03b@arm.com> X-Spam-Status: No, score=-16.6 required=5.0 tests=BAYES_00, BODY_8BITS, GIT_PATCH_0, KAM_DMARC_NONE, KAM_DMARC_STATUS, KAM_LAZY_DOMAIN_SECURITY, KAM_SHORT, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: "Andre Vieira \(lists\) via Gcc-patches" <gcc-patches@gcc.gnu.org> Reply-To: "Andre Vieira \(lists\)" <andre.simoesdiasvieira@arm.com> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
| Series |
[1/2] aarch64: Enable the use of LDAPR for load-acquire semantics
|
|
Commit Message
Andre Vieira (lists)
Nov. 10, 2022, 11:20 a.m. UTC
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.simoesdiasvieira@arm.com> Kyrylo Tkachov <kyrylo.tkachov@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.
Comments
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.simoesdiasvieira@arm.com> > Kyrylo Tkachov <kyrylo.tkachov@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..9670bef7d8cb2b32c5146536d806a7e8bdffb2e3 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" +) + +(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..aed27e06235b1d266decf11745dacf94cc59e76d --- /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)
> -----Original Message----- > From: Andre Vieira (lists) <andre.simoesdiasvieira@arm.com> > Sent: Monday, November 14, 2022 2:10 PM > To: gcc-patches@gcc.gnu.org > Cc: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>; Richard Earnshaw > <Richard.Earnshaw@arm.com>; Richard Sandiford > <Richard.Sandiford@arm.com> > Subject: Re: [PATCH 2/2] aarch64: Add support for widening LDAPR > instructions > > 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? Ok once the first patch is approved. Thanks, Kyrill > > > > 2022-11-09 Andre Vieira <andre.simoesdiasvieira@arm.com> > > Kyrylo Tkachov <kyrylo.tkachov@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.
"Andre Vieira (lists)" <andre.simoesdiasvieira@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.simoesdiasvieira@arm.com> >> Kyrylo Tkachov <kyrylo.tkachov@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..9670bef7d8cb2b32c5146536d806a7e8bdffb2e3 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.) 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..aed27e06235b1d266decf11745dacf94cc59e76d > --- /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)
> -----Original Message----- > From: Richard Sandiford <richard.sandiford@arm.com> > Sent: Tuesday, November 15, 2022 6:05 PM > To: Andre Simoes Dias Vieira <Andre.SimoesDiasVieira@arm.com> > Cc: gcc-patches@gcc.gnu.org; Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>; > Richard Earnshaw <Richard.Earnshaw@arm.com> > Subject: Re: [PATCH 2/2] aarch64: Add support for widening LDAPR > instructions > > "Andre Vieira (lists)" <andre.simoesdiasvieira@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.simoesdiasvieira@arm.com> > >> Kyrylo Tkachov <kyrylo.tkachov@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)
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.sandiford@arm.com> >> Sent: Tuesday, November 15, 2022 6:05 PM >> To: Andre Simoes Dias Vieira <Andre.SimoesDiasVieira@arm.com> >> Cc: gcc-patches@gcc.gnu.org; Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>; >> Richard Earnshaw <Richard.Earnshaw@arm.com> >> Subject: Re: [PATCH 2/2] aarch64: Add support for widening LDAPR >> instructions >> >> "Andre Vieira (lists)" <andre.simoesdiasvieira@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.simoesdiasvieira@arm.com> >>>> Kyrylo Tkachov <kyrylo.tkachov@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)
"Andre Vieira (lists)" <andre.simoesdiasvieira@arm.com> writes: > 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). The comparison doesn't lead to extra instructions. It's a compile-time constant, so the invalid patterns will be automatically dropped and not result in any code. That is, having the condition reduces the amount of static data (fewer patterns means fewer data structures for them) and reduces the code size (less for recog to do). If we want an assert for invalid extensions (which might be a good thing), it should be in target-independent code, so that it triggers regardless of whether the target defines a (bogus) pattern for it. Thanks, Richard > > 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.sandiford@arm.com> >>> Sent: Tuesday, November 15, 2022 6:05 PM >>> To: Andre Simoes Dias Vieira <Andre.SimoesDiasVieira@arm.com> >>> Cc: gcc-patches@gcc.gnu.org; Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>; >>> Richard Earnshaw <Richard.Earnshaw@arm.com> >>> Subject: Re: [PATCH 2/2] aarch64: Add support for widening LDAPR >>> instructions >>> >>> "Andre Vieira (lists)" <andre.simoesdiasvieira@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.simoesdiasvieira@arm.com> >>>>> Kyrylo Tkachov <kyrylo.tkachov@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)
diff --git a/gcc/config/aarch64/atomics.md b/gcc/config/aarch64/atomics.md index 9a9a30945c6e482a81a1bf446fe05d5efc462d32..77e5b29ad2c41215aa1ca904efb990b087010cef 100644 --- a/gcc/config/aarch64/atomics.md +++ b/gcc/config/aarch64/atomics.md @@ -691,6 +691,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" +) + +(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..5a788ffb8787291d43fe200d1d7803b901186912 --- /dev/null +++ b/gcc/testsuite/gcc.target/aarch64/ldapr-ext.c @@ -0,0 +1,94 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -std=c99" } */ +/* { dg-require-effective-target aarch64_rcpc_ok } */ +/* { dg-add-options aarch64_rcpc } */ +/* { dg-final { check-function-bodies "**" "" "" } } */ +#include <stdatomic.h> + +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)