Hi Tamar,
> On 14 Oct 2024, at 12:56, Tamar Christina <tamar.christ...@arm.com> wrote:
>
> External email: Use caution opening links or attachments
>
>
> Hi All,
>
> In this patch series I'm adding support for zero extending using permutes
> instead of requiring multi-step decomposition.
>
> This codegen has the benefit of needing fewer instructions and having much
> higher throughput than uxtl. We previously replaced pairs of uxtl/uxtl2s with
> ZIPs to increase throughput, but latency was still an issue due to the
> depencency chain created.
>
> To fix this we can now use TBLs. The indexes are listed output of the loop as
> well and can be shared amongst zero extends of the same type. The additional
> benefit of this is that if the values are being permuted after extensions they
> will be simplified and merged leading to better overall code.
>
> e.g. an LD4 can be replaced with LDR since the permutes being performance for
> the extensions can be merged with the load permutes. The way LOAD_LANES is
> currently implemented means this can't be done in GCC yet, but I'm aiming for
> this in GCC 15.
>
> I've additionally only added support for non-VLA. The problem with VLA is
> that
> the index registers are hard or impossible to make.
>
> On Adv. SIMD we use -1 to indicate an out of range register so we can
> transform
> the two regs TBL into a one reg one. However on e.g. a byte array, on VLA 255
> would be a valid entry. e.g, at VL > 2048. Which means that's already not a
> transformation we can make.
>
> Secondly the actual mask looks something like {x,x,x,n,n+1, x,x,x, n+2, n+3}
> and while I think I can represent this in vect_perm_builder, I couldn't think
> of
> any real efficient VLA way to build such masks.. It would require a lot of
> setup code.
>
> Lastly I don't think this transformation would make much sense for SVE, as SVE
> has loads and converts that can do multi-step types. For instance the loop
> below is already pretty good for SVE (though it's missed that the load can do
> more than one step, presumably because a single extend is merged only in RTL).
Thanks for working on this, it’s certainly an area that can be improved from
the current state!
On the SVE front, one thing we noticed recently is that on Neoverse V2 the SVE
single-register TBL instructions
have twice the throughput of the equivalent Neon ones. One has to be careful
with the indexes as SVE TBL instructions
can access lanes outside the 128-bit granule (and thus are not truly VLA) but
if the compiler controls the permute mask
as it does here they should be safe to emit.
I won’t block this patch/series on that point, just a consideration if you’re
interested.
Thanks,
Kyrill
>
> While I tried hard, for these reasons I don't support VLA, which I hope is
> ok..
>
> Concretely on AArch64 this changes:
>
> void test4(unsigned char *x, long long *y, int n) {
> for(int i = 0; i < n; i++) {
> y[i] = x[i];
> }
> }
>
> from generating:
>
> .L4:
> ldr q30, [x4], 16
> add x3, x3, 128
> zip1 v1.16b, v30.16b, v31.16b
> zip2 v30.16b, v30.16b, v31.16b
> zip1 v2.8h, v1.8h, v31.8h
> zip1 v0.8h, v30.8h, v31.8h
> zip2 v1.8h, v1.8h, v31.8h
> zip2 v30.8h, v30.8h, v31.8h
> zip1 v26.4s, v2.4s, v31.4s
> zip1 v29.4s, v0.4s, v31.4s
> zip1 v28.4s, v1.4s, v31.4s
> zip1 v27.4s, v30.4s, v31.4s
> zip2 v2.4s, v2.4s, v31.4s
> zip2 v0.4s, v0.4s, v31.4s
> zip2 v1.4s, v1.4s, v31.4s
> zip2 v30.4s, v30.4s, v31.4s
> stp q26, q2, [x3, -128]
> stp q28, q1, [x3, -96]
> stp q29, q0, [x3, -64]
> stp q27, q30, [x3, -32]
> cmp x4, x5
> bne .L4
>
> and instead we get:
>
> .L4:
> add x3, x3, 128
> ldr q23, [x4], 16
> tbl v5.16b, {v23.16b}, v31.16b
> tbl v4.16b, {v23.16b}, v30.16b
> tbl v3.16b, {v23.16b}, v29.16b
> tbl v2.16b, {v23.16b}, v28.16b
> tbl v1.16b, {v23.16b}, v27.16b
> tbl v0.16b, {v23.16b}, v26.16b
> tbl v22.16b, {v23.16b}, v25.16b
> tbl v23.16b, {v23.16b}, v24.16b
> stp q5, q4, [x3, -128]
> stp q3, q2, [x3, -96]
> stp q1, q0, [x3, -64]
> stp q22, q23, [x3, -32]
> cmp x4, x5
> bne .L4
>
> Which results in up to 40% performance uplift on certain workloads.
>
> Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
>
> Ok for master?
>
> Thanks,
> Tamar
>
> gcc/ChangeLog:
>
> * config/aarch64/aarch64.cc (aarch64_use_permute_for_promotion): New.
> (TARGET_VECTORIZE_USE_PERMUTE_FOR_PROMOTION): Use it.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/aarch64/vect-tbl-zero-extend_1.c: New test.
>
> ---
> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> index
> 102680a0efca1ce928e6945033c01cfb68a65152..b90577f4fc8157b3e02936256c8af8b2b7fac144
> 100644
> --- a/gcc/config/aarch64/aarch64.cc
> +++ b/gcc/config/aarch64/aarch64.cc
> @@ -28404,6 +28404,29 @@ aarch64_empty_mask_is_expensive (unsigned)
> return false;
> }
>
> +/* Implement TARGET_VECTORIZE_USE_PERMUTE_FOR_PROMOTION. Assume that
> + predicated operations when available are beneficial when doing more than
> + one step conversion. */
> +
> +static bool
> +aarch64_use_permute_for_promotion (const_tree in_type, const_tree out_type)
> +{
> + /* AArch64's vect_perm_constant doesn't currently support two 64 bit
> shuffles
> + into a 128 bit vector type. So for now reject it. */
> + if (maybe_ne (GET_MODE_BITSIZE (TYPE_MODE (in_type)),
> + GET_MODE_BITSIZE (TYPE_MODE (out_type))))
> + return false;
> +
> + auto bitsize_in = element_precision (in_type);
> + auto bitsize_out = element_precision (out_type);
> +
> + /* We don't want to use the permutes for a single widening step because
> we're
> + picking there between two zip and tbl sequences with the same throughput
> + and latencies. However the zip doesn't require a mask and uses less
> + registers so we prefer that. */
> + return (bitsize_out / bitsize_in) > 2;
> +}
> +
> /* Return 1 if pseudo register should be created and used to hold
> GOT address for PIC code. */
>
> @@ -31113,6 +31136,9 @@ aarch64_libgcc_floating_mode_supported_p
> #undef TARGET_VECTORIZE_CONDITIONAL_OPERATION_IS_EXPENSIVE
> #define TARGET_VECTORIZE_CONDITIONAL_OPERATION_IS_EXPENSIVE \
> aarch64_conditional_operation_is_expensive
> +#undef TARGET_VECTORIZE_USE_PERMUTE_FOR_PROMOTION
> +#define TARGET_VECTORIZE_USE_PERMUTE_FOR_PROMOTION \
> + aarch64_use_permute_for_promotion
> #undef TARGET_VECTORIZE_EMPTY_MASK_IS_EXPENSIVE
> #define TARGET_VECTORIZE_EMPTY_MASK_IS_EXPENSIVE \
> aarch64_empty_mask_is_expensive
> diff --git a/gcc/testsuite/gcc.target/aarch64/vect-tbl-zero-extend_1.c
> b/gcc/testsuite/gcc.target/aarch64/vect-tbl-zero-extend_1.c
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..3c088ced63543c203d1cc020de5d67807b48b3fb
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/vect-tbl-zero-extend_1.c
> @@ -0,0 +1,52 @@
> +/* { dg-do compile } */
> +/* { dg-additional-options "-O3 -std=c99 -march=armv8-a" } */
> +
> +void test1(unsigned short *x, double *y, int n) {
> + for(int i = 0; i < (n & -8); i++) {
> + unsigned short a = x[i*4+0];
> + unsigned short b = x[i*4+1];
> + unsigned short c = x[i*4+2];
> + unsigned short d = x[i*4+3];
> + y[i] = (double)a + (double)b + (double)c + (double)d;
> + }
> +}
> +
> +void test2(unsigned char *x, double *y, int n) {
> + for(int i = 0; i < (n & -8); i++) {
> + unsigned short a = x[i*4+0];
> + unsigned short b = x[i*4+1];
> + unsigned short c = x[i*4+2];
> + unsigned short d = x[i*4+3];
> + y[i] = (double)a + (double)b + (double)c + (double)d;
> + }
> +}
> +
> +void test3(unsigned short *x, double *y, int n) {
> + for(int i = 0; i < (n & -8); i++) {
> + unsigned int a = x[i];
> + y[i] = (double)a;
> + }
> +}
> +
> +void test4(unsigned short *x, long long *y, int n) {
> + for(int i = 0; i < (n & -8); i++) {
> + y[i] = x[i];
> + }
> +}
> +
> +void test5(unsigned int *x, long long *y, int n) {
> + for(int i = 0; i < (n & -8); i++) {
> + y[i] = x[i];
> + }
> +}
> +
> +void test6(unsigned char *x, long long *y, int n) {
> + for(int i = 0; i < (n & -8); i++) {
> + y[i] = x[i];
> + }
> +}
> +
> +/* { dg-final { scan-assembler-times {\tzip1} 1 } } */
> +/* { dg-final { scan-assembler-times {\tzip2} 1 } } */
> +/* { dg-final { scan-assembler-times {\ttbl} 64 } } */
> +/* { dg-final { scan-assembler-times {\.LC[0-9]+:} 12 } } */
>
>
>
>
> --
> <rb18855.patch>
