From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id 6A1F03858439 for ; Tue, 16 May 2023 12:30:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 6A1F03858439 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=arm.com 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 DCD152F4; Tue, 16 May 2023 05:31:08 -0700 (PDT) Received: from localhost (e121540-lin.manchester.arm.com [10.32.110.72]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 92AC63F663; Tue, 16 May 2023 05:30:23 -0700 (PDT) From: Richard Sandiford To: Oluwatamilore Adebayo Mail-Followup-To: Oluwatamilore Adebayo ,"gcc-patches\@gcc.gnu.org" , "richard.guenther\@gmail.com" , richard.sandiford@arm.com Cc: "gcc-patches\@gcc.gnu.org" , "richard.guenther\@gmail.com" Subject: Re: [PATCH] rtl: AArch64: New RTL for ABD References: Date: Tue, 16 May 2023 13:30:22 +0100 In-Reply-To: (Oluwatamilore Adebayo's message of "Tue, 9 May 2023 17:14:33 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-29.0 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_DMARC_NONE,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,KAM_LOTSOFHASH,KAM_SHORT,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Sorry for the slow reply. Oluwatamilore Adebayo writes: > From afa416dab831795f7e1114da2fb9e94ea3b8c519 Mon Sep 17 00:00:00 2001 > From: oluade01 > Date: Fri, 14 Apr 2023 15:10:07 +0100 > Subject: [PATCH 2/4] AArch64: New RTL for ABD > > This patch adds new RTL and tests for sabd and uabd > > PR tree-optimization/109156 > > gcc/ChangeLog: > > * config/aarch64/aarch64-simd-builtins.def (sabd, uabd): > Change the mode to 3. > * config/aarch64/aarch64-simd.md (aarch64_abd): > Rename to abd3. > * config/aarch64/aarch64-sve.md (abd_3): Rename > to abd3. Thanks. These changes look good, once the vectoriser part is sorted, but I have some comments about the tests: > diff --git a/gcc/testsuite/gcc.target/aarch64/abd.h b/gcc/testsuite/gcc.target/aarch64/abd.h > new file mode 100644 > index 0000000000000000000000000000000000000000..bc38e8508056cf2623cddd6053bf1cec3fa4ece4 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/aarch64/abd.h > @@ -0,0 +1,62 @@ > +#ifdef ABD_IDIOM > + > +#define TEST1(S, TYPE) \ > +void fn_##S##_##TYPE (S TYPE * restrict a, \ > + S TYPE * restrict b, \ > + S TYPE * restrict out) { \ > + for (int i = 0; i < N; i++) { \ > + signed TYPE diff = b[i] - a[i]; \ > + out[i] = diff > 0 ? diff : -diff; \ > +} } > + > +#define TEST2(S, TYPE1, TYPE2) \ > +void fn_##S##_##TYPE1##_##TYPE1##_##TYPE2 \ > + (S TYPE1 * restrict a, \ > + S TYPE1 * restrict b, \ > + S TYPE2 * restrict out) { \ > + for (int i = 0; i < N; i++) { \ > + signed TYPE2 diff = b[i] - a[i]; \ > + out[i] = diff > 0 ? diff : -diff; \ > +} } > + > +#define TEST3(S, TYPE1, TYPE2, TYPE3) \ > +void fn_##S##_##TYPE1##_##TYPE2##_##TYPE3 \ > + (S TYPE1 * restrict a, \ > + S TYPE2 * restrict b, \ > + S TYPE3 * restrict out) { \ > + for (int i = 0; i < N; i++) { \ > + signed TYPE3 diff = b[i] - a[i]; \ > + out[i] = diff > 0 ? diff : -diff; \ > +} } > + > +#endif > + > +#ifdef ABD_ABS > + > +#define TEST1(S, TYPE) \ > +void fn_##S##_##TYPE (S TYPE * restrict a, \ > + S TYPE * restrict b, \ > + S TYPE * restrict out) { \ > + for (int i = 0; i < N; i++) \ > + out[i] = __builtin_abs(a[i] - b[i]); \ > +} > + > +#define TEST2(S, TYPE1, TYPE2) \ > +void fn_##S##_##TYPE1##_##TYPE1##_##TYPE2 \ > + (S TYPE1 * restrict a, \ > + S TYPE1 * restrict b, \ > + S TYPE2 * restrict out) { \ > + for (int i = 0; i < N; i++) \ > + out[i] = __builtin_abs(a[i] - b[i]); \ > +} > + > +#define TEST3(S, TYPE1, TYPE2, TYPE3) \ > +void fn_##S##_##TYPE1##_##TYPE2##_##TYPE3 \ > + (S TYPE1 * restrict a, \ > + S TYPE2 * restrict b, \ > + S TYPE3 * restrict out) { \ > + for (int i = 0; i < N; i++) \ > + out[i] = __builtin_abs(a[i] - b[i]); \ > +} > + > +#endif It would be good to mark all of these functions with __attribute__((noipa)), since I think interprocedural optimisations might otherwise defeat the runtime test in abd_run_1.c (in the sense that we might end up folding things at compile time and not testing the vector versions of the functions). > diff --git a/gcc/testsuite/gcc.target/aarch64/abd_2.c b/gcc/testsuite/gcc.target/aarch64/abd_2.c > new file mode 100644 > index 0000000000000000000000000000000000000000..45bcfabe05a395f6775f78f28c73eb536ba5654e > --- /dev/null > +++ b/gcc/testsuite/gcc.target/aarch64/abd_2.c > @@ -0,0 +1,34 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O3" } */ > + > +#pragma GCC target "+nosve" > +#define N 1024 > + > +#define ABD_ABS > +#include "abd.h" > + > +TEST1(signed, int) > +TEST1(signed, short) > +TEST1(signed, char) > + > +TEST2(signed, char, int) > +TEST2(signed, char, short) > + > +TEST3(signed, char, int, short) > +TEST3(signed, char, short, int) > + > +TEST1(unsigned, int) > +TEST1(unsigned, short) > +TEST1(unsigned, char) > + > +TEST2(unsigned, char, int) > +TEST2(unsigned, char, short) > + > +TEST3(unsigned, char, int, short) > +TEST3(unsigned, char, short, int) > + > +/* { dg-final { scan-assembler-times "sabd\\tv\[0-9\]+\.4s, v\[0-9\]+\.4s, v\[0-9\]+\.4s" 2 } } */ > +/* { dg-final { scan-assembler-times "sabd\\tv\[0-9\]+\.8h, v\[0-9\]+\.8h, v\[0-9\]+\.8h" 1 } } */ > +/* { dg-final { scan-assembler-times "sabd\\tv\[0-9\]+\.16b, v\[0-9\]+\.16b, v\[0-9\]+\.16b" 1 } } */ > +/* { dg-final { scan-assembler-times "uabd\\tv\[0-9\]+\.8h, v\[0-9\]+\.8h, v\[0-9\]+\.8h" 1 } } */ > +/* { dg-final { scan-assembler-times "uabd\\tv\[0-9\]+\.16b, v\[0-9\]+\.16b, v\[0-9\]+\.16b" 1 } } */ There are 14 tests, and it looks like 6 of them are expected to produce ABD instructions while 8 aren't. It isn't really clear which tests are which though. I think it'd help to split the file into two: - one containing only the tests that should produce ABD, so that the scan-assembler counts sum up to the number of tests - one containing only the tests that cannot use ABD, with: { dg-final { scan-assembler-not {\tsabd\t} } } { dg-final { scan-assembler-not {\tuabd\t} } } to enforce that > diff --git a/gcc/testsuite/gcc.target/aarch64/sve/abd_1.c b/gcc/testsuite/gcc.target/aarch64/sve/abd_1.c > new file mode 100644 > index 0000000000000000000000000000000000000000..6ba111a623a344877a9d2eabda29a629a0dc8258 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/aarch64/sve/abd_1.c > @@ -0,0 +1,34 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O3" } */ > + > +#pragma GCC target "arch=armv8-a" > +#define N 1024 > + > +#define ABD_ABS > +#include "../abd.h" > + > +TEST1(signed, int) > +TEST1(signed, short) > +TEST1(signed, char) > + > +TEST2(signed, char, int) > +TEST2(signed, char, short) > + > +TEST3(signed, char, int, short) > +TEST3(signed, char, short, int) > + > +TEST1(unsigned, int) > +TEST1(unsigned, short) > +TEST1(unsigned, char) > + > +TEST2(unsigned, char, int) > +TEST2(unsigned, char, short) > + > +TEST3(unsigned, char, int, short) > +TEST3(unsigned, char, short, int) > + > +/* { dg-final { scan-assembler-times "sabd\\tv\[0-9\]+\.4s, v\[0-9\]+\.4s, v\[0-9\]+\.4s" 2 } } */ > +/* { dg-final { scan-assembler-times "sabd\\tv\[0-9\]+\.8h, v\[0-9\]+\.8h, v\[0-9\]+\.8h" 1 } } */ > +/* { dg-final { scan-assembler-times "sabd\\tv\[0-9\]+\.16b, v\[0-9\]+\.16b, v\[0-9\]+\.16b" 1 } } */ > +/* { dg-final { scan-assembler-times "uabd\\tv\[0-9\]+\.8h, v\[0-9\]+\.8h, v\[0-9\]+\.8h" 1 } } */ > +/* { dg-final { scan-assembler-times "uabd\\tv\[0-9\]+\.16b, v\[0-9\]+\.16b, v\[0-9\]+\.16b" 1 } } */ > diff --git a/gcc/testsuite/gcc.target/aarch64/sve/abd_2.c b/gcc/testsuite/gcc.target/aarch64/sve/abd_2.c > new file mode 100644 > index 0000000000000000000000000000000000000000..6d4b3fec76279656ebf827c386481337451f82fa > --- /dev/null > +++ b/gcc/testsuite/gcc.target/aarch64/sve/abd_2.c > @@ -0,0 +1,33 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O3" } */ > + > +#pragma GCC target "arch=armv8-a" > +#define N 1024 > + > +#define ABD_IDIOM > +#include "../abd.h" > + > +TEST1(signed, int) > +TEST1(signed, short) > +TEST1(signed, char) > + > +TEST2(signed, char, int) > +TEST2(signed, char, short) > + > +TEST3(signed, char, int, short) > +TEST3(signed, char, short, int) > + > +TEST1(unsigned, int) > +TEST1(unsigned, short) > +TEST1(unsigned, char) > + > +TEST2(unsigned, char, int) > +TEST2(unsigned, char, short) > + > +TEST3(unsigned, char, int, short) > +TEST3(unsigned, char, short, int) > + > +/* { dg-final { scan-assembler-times "sabd\\tv\[0-9\]+\.4s, v\[0-9\]+\.4s, v\[0-9\]+\.4s" 2 } } */ > +/* { dg-final { scan-assembler-times "sabd\\tv\[0-9\]+\.8h, v\[0-9\]+\.8h, v\[0-9\]+\.8h" 8 } } */ > +/* { dg-final { scan-assembler-times "sabd\\tv\[0-9\]+\.16b, v\[0-9\]+\.16b, v\[0-9\]+\.16b" 2 } } */ > +/* { dg-final { scan-assembler-times "uabd\\tv\[0-9\]+\.8h, v\[0-9\]+\.8h, v\[0-9\]+\.8h" 2 } } */ For these SVE tests, it'd be better to drop the: #pragma GCC target "arch=armv8-a" lines and instead allow SVE to be used as normal. We should then be able to match the SVE instructions in the dg-final lines. Thanks, Richard