Hi Kyrill,
> -----Original Message-----
> From: Kyrylo Tkachov <ktkac...@nvidia.com>
> Sent: Monday, October 14, 2024 12:15 PM
> To: Tamar Christina <tamar.christ...@arm.com>
> Cc: GCC Patches <gcc-patches@gcc.gnu.org>; nd <n...@arm.com>; Richard
> Earnshaw <richard.earns...@arm.com>; ktkac...@gcc.gnu.org; Richard
> Sandiford <richard.sandif...@arm.com>
> Subject: Re: [PATCH 3/4]AArch64: enable zero-extends using TBLs for Adv. SIMD
>
> 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've checked internally with the CPU teams and verified on simulators, this is
likely an
error in the SWoG of several cores. These will be corrected in the next
releases.
>
> I won’t block this patch/series on that point, just a consideration if you’re
> interested.
I did think about it for a long time, but the problem is the single register
version of TBL
for SVE won't work, as you mentioned and I had in the cover letter. So you'd
need the
two register version for the byte case, and that's SVE2 only. I think short
and above
would work though.
I was struggling with how to even represent the sequence in VLA. I realized
that you'd
have to get the VL with rdvl, and then use a predicated mov to fill the index
register with
the indices to the zero register.
The problem is though how to create the "selection" mask. An INDEX gets you the
sequential
numbers, but you then need to space them out. e.g for byte to int you want
{x,x,x,x,x,i,x,x,x,x,x,i+1,...} which I really can't think of anyway to
represent in gimple but also
can't think of a way to generate in SVE code that doesn't require a lot of
code. For instance
there's no predicated INDEX. The only thing I can think of is either INDEX +
unpacks + pred mov.
Anyway, I'd be happy to consider this for VLA in general, though I do hope we
can get non-VLA
in first, but I really don't know of a good sequence here.
Although back to your original point, yes we could have used SVE for the Adv.
SIMD version by
creating the mask using LD1R instead of LDR. But I'm concerned since there's no
predicated
version of TBL, that on longer VLs we'd do a lot of work that is just ignored.
It's a good question that gave me lots to think about though..
Thanks,
Tamar
> 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..b90577f4fc8157b3e02936256
> c8af8b2b7fac144 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..3c088ced63543c203d1cc0
> 20de5d67807b48b3fb
> > --- /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>
