From: Richard Sandiford <richard.sandiford@arm.com>
To: Oluwatamilore Adebayo <Oluwatamilore.Adebayo@arm.com>
Cc: "gcc-patches\@gcc.gnu.org" <gcc-patches@gcc.gnu.org>,
"richard.guenther\@gmail.com" <richard.guenther@gmail.com>
Subject: Re: [PATCH] rtl: AArch64: New RTL for ABD
Date: Tue, 16 May 2023 13:30:22 +0100 [thread overview]
Message-ID: <mptwn18zcf5.fsf@arm.com> (raw)
In-Reply-To: <DB7PR08MB34526BFA97C71399317166B0F5769@DB7PR08MB3452.eurprd08.prod.outlook.com> (Oluwatamilore Adebayo's message of "Tue, 9 May 2023 17:14:33 +0100")
Sorry for the slow reply.
Oluwatamilore Adebayo <Oluwatamilore.Adebayo@arm.com> writes:
> From afa416dab831795f7e1114da2fb9e94ea3b8c519 Mon Sep 17 00:00:00 2001
> From: oluade01 <oluwatamilore.adebayo@arm.com>
> 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_<su>abd<mode>):
> Rename to <su>abd<mode>3.
> * config/aarch64/aarch64-sve.md (<su>abd<mode>_3): Rename
> to <su>abd<mode>3.
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
next prev parent reply other threads:[~2023-05-16 12:30 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-09 16:14 Oluwatamilore Adebayo
2023-05-16 12:30 ` Richard Sandiford [this message]
2023-06-06 15:11 ` Oluwatamilore Adebayo
2023-06-06 15:17 ` [PATCH 2/2] " Oluwatamilore Adebayo
2023-06-06 17:03 ` [PATCH] rtl: " Richard Sandiford
2023-06-08 10:37 ` Oluwatamilore Adebayo
2023-06-08 10:38 ` [PATCH 2/2] " Oluwatamilore Adebayo
2023-06-13 8:27 ` Oluwatamilore Adebayo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=mptwn18zcf5.fsf@arm.com \
--to=richard.sandiford@arm.com \
--cc=Oluwatamilore.Adebayo@arm.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=richard.guenther@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).