> -----Original Message----- > From: Richard Biener <rguent...@suse.de> > Sent: Monday, June 20, 2022 12:56 PM > To: Tamar Christina <tamar.christ...@arm.com> > Cc: Andrew Pinski via Gcc-patches <gcc-patches@gcc.gnu.org>; nd > <n...@arm.com> > Subject: RE: [PATCH]middle-end Add optimized float addsub without > needing VEC_PERM_EXPR. > > On Mon, 20 Jun 2022, Tamar Christina wrote: > > > > -----Original Message----- > > > From: Richard Biener <rguent...@suse.de> > > > Sent: Saturday, June 18, 2022 11:49 AM > > > To: Andrew Pinski via Gcc-patches <gcc-patches@gcc.gnu.org> > > > Cc: Tamar Christina <tamar.christ...@arm.com>; nd <n...@arm.com> > > > Subject: Re: [PATCH]middle-end Add optimized float addsub without > > > needing VEC_PERM_EXPR. > > > > > > > > > > > > > Am 17.06.2022 um 22:34 schrieb Andrew Pinski via Gcc-patches <gcc- > > > patc...@gcc.gnu.org>: > > > > > > > > On Thu, Jun 16, 2022 at 3:59 AM Tamar Christina via Gcc-patches > > > > <gcc-patches@gcc.gnu.org> wrote: > > > >> > > > >> Hi All, > > > >> > > > >> For IEEE 754 floating point formats we can replace a sequence of > > > >> alternative > > > >> +/- with fneg of a wider type followed by an fadd. This > > > >> +eliminated the need for > > > >> using a permutation. This patch adds a math.pd rule to recognize > > > >> and do this rewriting. > > > > > > > > I don't think this is correct. You don't check the format of the > > > > floating point to make sure this is valid (e.g. REAL_MODE_FORMAT's > > > > signbit_rw/signbit_ro field). > > > > Yes I originally had this check, but I wondered whether it would be needed. > > I'm not aware of any vector ISA where the 32-bit and 16-bit floats > > don't follow the IEEE data layout and semantics here. > > > > My preference would be to ask the target about the data format of its > > vector Floating points because I don't think there needs to be a direct > correlation between > > The scalar and vector formats strictly speaking. But I know Richi won't > > like > that so > > the check is probably most likely. > > > > > > Also would just be better if you do the xor in integer mode (using > > > > signbit_rw field for the correct bit)? > > > > And then making sure the target optimizes the xor to the neg > > > > instruction when needed? > > > > I don't really see the advantage of this one. It's not removing an > > instruction and it's assuming the vector ISA can do integer ops on a > > floating point vector cheaply. Since match.pd doesn't have the > > ability to do costing I'd rather not do this. > > > > > I’m also worried about using FP operations for the negate here. > > > When @1 is constant do we still constant fold this correctly? > > > > We never did constant folding for this case, the folding > > infrastructure doesn't know how to fold the VEC_PERM_EXPR. So even > > with @0 and @1 constant no folding takes place even today if we vectorize. > > > > > > > > For costing purposes it would be nice to make this visible to the > vectorizer. > > > > > > > I initially wanted to use VEC_ADDSUB for this, but noticed it didn't > > trigger in a number of place I had expected it to. While looking into > > it I noticed it's because this follows the x86 instruction semantics so > > left it > alone. > > > > It felt like adding a third pattern here might be confusing. However I > > can also use the SLP pattern matcher to rewrite it without an optab if you > prefer that? > > > > The operations will then be costed normally. > > > > > Also is this really good for all targets? Can there be issues with > > > reformatting when using FP ops as in your patch or with using > > > integer XOR as suggested making this more expensive than the blend? > > > > I don't think with the fp ops alone, since it's using two fp ops already > > and > after the change 2 fp ops. > > and I can't image that a target would have a slow -a. > > Wouldn't a target need to re-check if lanes are NaN or denormal if after a > SFmode lane operation a DFmode lane operation follows? IIRC that is what > usually makes punning "integer" vectors as FP vectors costly.
I guess this really depends on the target. > > Note one option would be to emit a multiply with { 1, -1, 1, -1 } on GIMPLE > where then targets could opt-in to handle this via a DFmode negate via a > combine pattern? Not sure if this can be even done starting from the vec- > perm RTL IL. > But multiplies can be so expensive that the VEC_PERM_EXPR would still be better. At least as you say, the target costed for that. > I fear whether (neg:V2DF (subreg:V2DF (reg:V4SF))) is a good idea will > heavily depend on the target CPU (not only the ISA). For RISC-V for example > I think the DF lanes do not overlap with two SF lanes (so same with gcn I > think). Right, so I think the conclusion is I need to move this to the backend. Thanks, Tamar > > Richard. > > > The XOR one I wouldn't do, as the vector int and vector float could > > for instance be in different register files or FP be a co-processor > > etc. Mixing FP and Integer ops in this case I can image can lead to > > something suboptimal. Also for targets with masking/predication the > VEC_PERM_EXP could potentially be lowered to a mask/predicate in the > backend. Whereas the XOR approach is far less likely. > > > > Thanks, > > Tamar > > > > > > > > Richard. > > > > > > > Thanks, > > > > Andrew Pinski > > > > > > > > > > > > > > > >> > > > >> For > > > >> > > > >> void f (float *restrict a, float *restrict b, float *res, int n) { > > > >> for (int i = 0; i < (n & -4); i+=2) > > > >> { > > > >> res[i+0] = a[i+0] + b[i+0]; > > > >> res[i+1] = a[i+1] - b[i+1]; > > > >> } > > > >> } > > > >> > > > >> we generate: > > > >> > > > >> .L3: > > > >> ldr q1, [x1, x3] > > > >> ldr q0, [x0, x3] > > > >> fneg v1.2d, v1.2d > > > >> fadd v0.4s, v0.4s, v1.4s > > > >> str q0, [x2, x3] > > > >> add x3, x3, 16 > > > >> cmp x3, x4 > > > >> bne .L3 > > > >> > > > >> now instead of: > > > >> > > > >> .L3: > > > >> ldr q1, [x0, x3] > > > >> ldr q2, [x1, x3] > > > >> fadd v0.4s, v1.4s, v2.4s > > > >> fsub v1.4s, v1.4s, v2.4s > > > >> tbl v0.16b, {v0.16b - v1.16b}, v3.16b > > > >> str q0, [x2, x3] > > > >> add x3, x3, 16 > > > >> cmp x3, x4 > > > >> bne .L3 > > > >> > > > >> Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. > > > >> > > > >> Thanks to George Steed for the idea. > > > >> > > > >> Ok for master? > > > >> > > > >> Thanks, > > > >> Tamar > > > >> > > > >> gcc/ChangeLog: > > > >> > > > >> * match.pd: Add fneg/fadd rule. > > > >> > > > >> gcc/testsuite/ChangeLog: > > > >> > > > >> * gcc.target/aarch64/simd/addsub_1.c: New test. > > > >> * gcc.target/aarch64/sve/addsub_1.c: New test. > > > >> > > > >> --- inline copy of patch -- > > > >> diff --git a/gcc/match.pd b/gcc/match.pd index > > > >> > > > > 51b0a1b562409af535e53828a10c30b8a3e1ae2e..af1c98d4a2831f38258d6fc1bb > > > e > > > >> 811c8ee6c7c6e 100644 > > > >> --- a/gcc/match.pd > > > >> +++ b/gcc/match.pd > > > >> @@ -7612,6 +7612,49 @@ and, > > > >> (simplify (reduc (op @0 VECTOR_CST@1)) > > > >> (op (reduc:type @0) (reduc:type @1)))) > > > >> > > > >> +/* Simplify vector floating point operations of alternating sub/add > pairs > > > >> + into using an fneg of a wider element type followed by a normal > add. > > > >> + under IEEE 754 the fneg of the wider type will negate every even > entry > > > >> + and when doing an add we get a sub of the even and add of every > odd > > > >> + elements. */ > > > >> +(simplify > > > >> + (vec_perm (plus:c @0 @1) (minus @0 @1) VECTOR_CST@2) (if > > > >> +(!VECTOR_INTEGER_TYPE_P (type) && !BYTES_BIG_ENDIAN) > > > >> + (with > > > >> + { > > > >> + /* Build a vector of integers from the tree mask. */ > > > >> + vec_perm_builder builder; > > > >> + if (!tree_to_vec_perm_builder (&builder, @2)) > > > >> + return NULL_TREE; > > > >> + > > > >> + /* Create a vec_perm_indices for the integer vector. */ > > > >> + poly_uint64 nelts = TYPE_VECTOR_SUBPARTS (type); > > > >> + vec_perm_indices sel (builder, 2, nelts); > > > >> + } > > > >> + (if (sel.series_p (0, 2, 0, 2)) > > > >> + (with > > > >> + { > > > >> + machine_mode vec_mode = TYPE_MODE (type); > > > >> + auto elem_mode = GET_MODE_INNER (vec_mode); > > > >> + auto nunits = exact_div (GET_MODE_NUNITS (vec_mode), 2); > > > >> + tree stype; > > > >> + switch (elem_mode) > > > >> + { > > > >> + case E_HFmode: > > > >> + stype = float_type_node; > > > >> + break; > > > >> + case E_SFmode: > > > >> + stype = double_type_node; > > > >> + break; > > > >> + default: > > > >> + return NULL_TREE; > > > >> + } > > > >> + tree ntype = build_vector_type (stype, nunits); > > > >> + if (!ntype) > > > >> + return NULL_TREE; > > > >> + } > > > >> + (plus (view_convert:type (negate (view_convert:ntype @1))) > > > >> + @0)))))) > > > >> + > > > >> (simplify > > > >> (vec_perm @0 @1 VECTOR_CST@2) > > > >> (with > > > >> diff --git a/gcc/testsuite/gcc.target/aarch64/simd/addsub_1.c > > > >> b/gcc/testsuite/gcc.target/aarch64/simd/addsub_1.c > > > >> new file mode 100644 > > > >> index > > > >> > > > > 0000000000000000000000000000000000000000..1fb91a34c421bbd2894faa0db > > > bf > > > >> 1b47ad43310c4 > > > >> --- /dev/null > > > >> +++ b/gcc/testsuite/gcc.target/aarch64/simd/addsub_1.c > > > >> @@ -0,0 +1,56 @@ > > > >> +/* { dg-do compile } */ > > > >> +/* { dg-require-effective-target arm_v8_2a_fp16_neon_ok } */ > > > >> +/* { dg-options "-Ofast" } */ > > > >> +/* { dg-add-options arm_v8_2a_fp16_neon } */ > > > >> +/* { dg-final { check-function-bodies "**" "" "" { target { le } > > > >> +} } } */ > > > >> + > > > >> +#pragma GCC target "+nosve" > > > >> + > > > >> +/* > > > >> +** f1: > > > >> +** ... > > > >> +** fneg v[0-9]+.2d, v[0-9]+.2d > > > >> +** fadd v[0-9]+.4s, v[0-9]+.4s, v[0-9]+.4s > > > >> +** ... > > > >> +*/ > > > >> +void f1 (float *restrict a, float *restrict b, float *res, int n) { > > > >> + for (int i = 0; i < (n & -4); i+=2) > > > >> + { > > > >> + res[i+0] = a[i+0] + b[i+0]; > > > >> + res[i+1] = a[i+1] - b[i+1]; > > > >> + } > > > >> +} > > > >> + > > > >> +/* > > > >> +** d1: > > > >> +** ... > > > >> +** fneg v[0-9]+.4s, v[0-9]+.4s > > > >> +** fadd v[0-9]+.8h, v[0-9]+.8h, v[0-9]+.8h > > > >> +** ... > > > >> +*/ > > > >> +void d1 (_Float16 *restrict a, _Float16 *restrict b, _Float16 > > > >> +*res, int n) { > > > >> + for (int i = 0; i < (n & -8); i+=2) > > > >> + { > > > >> + res[i+0] = a[i+0] + b[i+0]; > > > >> + res[i+1] = a[i+1] - b[i+1]; > > > >> + } > > > >> +} > > > >> + > > > >> +/* > > > >> +** e1: > > > >> +** ... > > > >> +** fadd v[0-9]+.2d, v[0-9]+.2d, v[0-9]+.2d > > > >> +** fsub v[0-9]+.2d, v[0-9]+.2d, v[0-9]+.2d > > > >> +** ins v[0-9]+.d\[1\], v[0-9]+.d\[1\] > > > >> +** ... > > > >> +*/ > > > >> +void e1 (double *restrict a, double *restrict b, double *res, > > > >> +int n) { > > > >> + for (int i = 0; i < (n & -4); i+=2) > > > >> + { > > > >> + res[i+0] = a[i+0] + b[i+0]; > > > >> + res[i+1] = a[i+1] - b[i+1]; > > > >> + } > > > >> +} > > > >> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/addsub_1.c > > > >> b/gcc/testsuite/gcc.target/aarch64/sve/addsub_1.c > > > >> new file mode 100644 > > > >> index > > > >> > > > > 0000000000000000000000000000000000000000..ea7f9d9db2c8c9a3efe5c7951a > > > 3 > > > >> 14a29b7a7a922 > > > >> --- /dev/null > > > >> +++ b/gcc/testsuite/gcc.target/aarch64/sve/addsub_1.c > > > >> @@ -0,0 +1,52 @@ > > > >> +/* { dg-do compile } */ > > > >> +/* { dg-options "-Ofast" } */ > > > >> +/* { dg-final { check-function-bodies "**" "" "" { target { le } > > > >> +} } } */ > > > >> + > > > >> +/* > > > >> +** f1: > > > >> +** ... > > > >> +** fneg z[0-9]+.d, p[0-9]+/m, z[0-9]+.d > > > >> +** fadd z[0-9]+.s, z[0-9]+.s, z[0-9]+.s > > > >> +** ... > > > >> +*/ > > > >> +void f1 (float *restrict a, float *restrict b, float *res, int n) { > > > >> + for (int i = 0; i < (n & -4); i+=2) > > > >> + { > > > >> + res[i+0] = a[i+0] + b[i+0]; > > > >> + res[i+1] = a[i+1] - b[i+1]; > > > >> + } > > > >> +} > > > >> + > > > >> +/* > > > >> +** d1: > > > >> +** ... > > > >> +** fneg z[0-9]+.s, p[0-9]+/m, z[0-9]+.s > > > >> +** fadd z[0-9]+.h, z[0-9]+.h, z[0-9]+.h > > > >> +** ... > > > >> +*/ > > > >> +void d1 (_Float16 *restrict a, _Float16 *restrict b, _Float16 > > > >> +*res, int n) { > > > >> + for (int i = 0; i < (n & -8); i+=2) > > > >> + { > > > >> + res[i+0] = a[i+0] + b[i+0]; > > > >> + res[i+1] = a[i+1] - b[i+1]; > > > >> + } > > > >> +} > > > >> + > > > >> +/* > > > >> +** e1: > > > >> +** ... > > > >> +** fsub z[0-9]+.d, z[0-9]+.d, z[0-9]+.d > > > >> +** movprfx z[0-9]+.d, p[0-9]+/m, z[0-9]+.d > > > >> +** fadd z[0-9]+.d, p[0-9]+/m, z[0-9]+.d, z[0-9]+.d > > > >> +** ... > > > >> +*/ > > > >> +void e1 (double *restrict a, double *restrict b, double *res, > > > >> +int n) { > > > >> + for (int i = 0; i < (n & -4); i+=2) > > > >> + { > > > >> + res[i+0] = a[i+0] + b[i+0]; > > > >> + res[i+1] = a[i+1] - b[i+1]; > > > >> + } > > > >> +} > > > >> > > > >> > > > >> > > > >> > > > >> -- > > > > -- > Richard Biener <rguent...@suse.de> > SUSE Software Solutions Germany GmbH, Frankenstraße 146, 90461 > Nuernberg, Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, > Boudien Moerman; HRB 36809 (AG Nuernberg)