Re: [PATCH, V3] Optimize vec_splats of constant vec_extract for V2DI/V2DF, PR target 99293
On Tue, 2022-06-07 at 15:21 -0500, Segher Boessenkool wrote: > On Tue, Jun 07, 2022 at 02:26:17PM -0500, will schmidt wrote: > > On Mon, 2022-06-06 at 20:31 -0400, Michael Meissner wrote: > > > (define_insn "vsx_xxspltd_" > > >[(set (match_operand:VSX_D 0 "vsx_register_operand" "=wa") > > > -(unspec:VSX_D [(match_operand:VSX_D 1 > > > "vsx_register_operand" > > > "wa") > > Someone (you?) uses format=flawed. You cannot reply to emails that > contain patches that way, it messes up everything :-( Right.. Something on my end may be posessed, several of my emails today have tried to go all HTML on me, and or otherwise gone format-wonky, which I do not want. ;-) > > > > -(match_operand:QI 2 "u5bit_cint_operand" "i")] > > > - UNSPEC_VSX_XXSPLTD))] > > > + (vec_duplicate:VSX_D > > > + (vec_select: > > > + (match_operand:VSX_D 1 "gpc_reg_operand" "wa") > > > + (parallel [(match_operand:QI 2 "const_0_to_1_operand" > > > "i")]] > > >"VECTOR_MEM_VSX_P (mode)" > > > > Noting that > > (define_mode_iterator VSX_D [V2DF V2DI]) > > (define_mode_attr VS_scalar [(V1TI "TI") > > (V2DF "DF") > > (V2DI "DI") > > (V4SF "SF") > > (V4SI "SI") > > (V8HI "HI") > > (V16QI "QI")]) > > Yeah, the comment > ;; Map the scalar mode for a vector type > is misleading, in more ways than one :-( > > And the whole thing is just the same as VEC_base anyway, so it is > much > better to just use that. > > > Segher
Re: [PATCH, V3] Optimize vec_splats of constant vec_extract for V2DI/V2DF, PR target 99293
On Tue, Jun 07, 2022 at 02:26:17PM -0500, will schmidt wrote: > On Mon, 2022-06-06 at 20:31 -0400, Michael Meissner wrote: > > (define_insn "vsx_xxspltd_" > >[(set (match_operand:VSX_D 0 "vsx_register_operand" "=wa") > > -(unspec:VSX_D [(match_operand:VSX_D 1 "vsx_register_operand" > > "wa") Someone (you?) uses format=flawed. You cannot reply to emails that contain patches that way, it messes up everything :-( > > - (match_operand:QI 2 "u5bit_cint_operand" "i")] > > - UNSPEC_VSX_XXSPLTD))] > > + (vec_duplicate:VSX_D > > +(vec_select: > > + (match_operand:VSX_D 1 "gpc_reg_operand" "wa") > > + (parallel [(match_operand:QI 2 "const_0_to_1_operand" > > "i")]] > >"VECTOR_MEM_VSX_P (mode)" > > Noting that > (define_mode_iterator VSX_D [V2DF V2DI]) > (define_mode_attr VS_scalar [(V1TI"TI") >(V2DF "DF") >(V2DI "DI") >(V4SF "SF") >(V4SI "SI") >(V8HI "HI") >(V16QI "QI")]) Yeah, the comment ;; Map the scalar mode for a vector type is misleading, in more ways than one :-( And the whole thing is just the same as VEC_base anyway, so it is much better to just use that. Segher
Re: [PATCH, V3] Optimize vec_splats of constant vec_extract for V2DI/V2DF, PR target 99293
On Mon, 2022-06-06 at 20:31 -0400, Michael Meissner wrote: > Optimize vec_splats of constant vec_extract for V2DI/V2DF, PR target > 99293. > > This is version 3 of the patch. The original patch was: > > > Date: Mon, 28 Mar 2022 12:26:02 -0400 > > Subject: [PATCH 1/4] Optimize vec_splats of constant vec_extract > > for V2DI/V2DF, PR target 99293. > > Message-ID: > > https://gcc.gnu.org/pipermail/gcc-patches/2022-March/592420.html > > Version 2 of the patch was: > > > Date: Fri, 13 May 2022 10:49:26 -0400 > > Subject: [PATCH] Optimize vec_splats of constant V2DI/V2DF > > vec_extract, PR target/99293 > > Message-ID: > > https://gcc.gnu.org/pipermail/gcc-patches/2022-May/594797.html > > The differences between version 2 and version 3 was to clean up the > description > of what the patch does, and to make the example test case clear. > > In PR target/99293, it was pointed out that doing: > > vector long long dest0, dest1, src; > /* ... */ > dest0 = vec_splats (vec_extract (src, 0)); > dest1 = vec_splats (vec_extract (src, 1)); > > would generate slower code. > > It generates the following code on power8: > > ;; vec_splats (vec_extract (src, 0)) > xxpermdi 0,34,34,3 > xxpermdi 34,0,0,0 > > ;; vec_splats (vec_extract (src, 1)) > xxlor 0,34,34 > xxpermdi 34,0,0,0 > > However on power9 and power10 it generates: > > ;; vec_splats (vec_extract (src, 0)) > mfvsld 3,34 > mtvsrdd 34,9,9 > > ;; vec_splats (vec_extract (src, 1)) > mfvsrd 9,34 > mtvsrdd 34,9,9 > > This is due to the power9 having the mfvsrld instruction which can > extract > either 64-bit element into a GPR. While there are alternatives for > both > vector registers and GPR registers, the register allocator prefers to > put > DImode into GPR registers. > > In this case, it is better to have a single combiner pattern that can > generate > a single xxpermdi, instead of 2 insnsns (the extract and then the > concat). > This is true if the two operations are move from vector register and > move to > vector register. As Segher pointed out in a previous version of the > patch, the > combiner already tries doing creating a (vec_duplicate (vec_select > ...)) > pattern, but we didn't provide one. > > This patch reworks vsx_xxspltd_ for V2DImode and V2DFmode so > that it now > uses VEC_DUPLICATE, which the combiner checks for. Ok. > > I have built Spec 2017 with this patch installed, and the cam4_r > benchmark > is the only benchmark that generated different code (3 > mfvsrld/mtvsrdd > pairs of instructions were replaced with xxpermdi). > > I have built bootstrap versions on the following systems and I have > run > the regression tests. There were no regressions in the runs: > > Power9 little endian, --with-cpu=power9 > Power10 little endian, --with-cpu=power10 > Power8 big endian, --with-cpu=power8 (both 32-bit & 64-bit > tests) Ok. > > Can I install this into the trunk? After a burn-in period, can I > backport > and install this into GCC 11 and GCC 10 branches? > > 2022-06-06 Michael Meissner > > gcc/ > PR target/99293 > * config/rs6000/rs6000-p8swap.cc (rtx_is_swappable_p): Remove > UNSPEC_VSX_XXSPLTD case. > * config/rs6000/vsx.md (UNSPEC_VSX_XXSPLTD): Delete. > (vsx_xxspltd_): Rewrite to use VEC_DUPLICATE. > > gcc/testsuite: > PR target/99293 > * gcc.target/powerpc/builtins-1.c: Update insn count. > * gcc.target/powerpc/pr99293.c: New test. > --- > gcc/config/rs6000/rs6000-p8swap.cc| 1 - > gcc/config/rs6000/vsx.md | 19 +++ > gcc/testsuite/gcc.target/powerpc/builtins-1.c | 2 +- > gcc/testsuite/gcc.target/powerpc/pr99293.c| 51 > +++ > 4 files changed, 62 insertions(+), 11 deletions(-) > create mode 100644 gcc/testsuite/gcc.target/powerpc/pr99293.c > > diff --git a/gcc/config/rs6000/rs6000-p8swap.cc > b/gcc/config/rs6000/rs6000-p8swap.cc > index 275702fee1b..3160fcbdeca 100644 > --- a/gcc/config/rs6000/rs6000-p8swap.cc > +++ b/gcc/config/rs6000/rs6000-p8swap.cc > @@ -807,7 +807,6 @@ rtx_is_swappable_p (rtx op, unsigned int > *special) > case UNSPEC_VUPKLU_V4SF: > return 0; > case UNSPEC_VSPLT_DIRECT: > - case UNSPEC_VSX_XXSPLTD: > *special = SH_SPLAT; > return 1; > case UNSPEC_REDUC_PLUS: > diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md > index 1b75538f42f..a1a1ce95195 100644 > --- a/gcc/config/rs6000/vsx.md > +++ b/gcc/config/rs6000/vsx.md > @@ -296,7 +296,6 @@ (define_c_enum "unspec" > UNSPEC_VSX_XXPERM > > UNSPEC_VSX_XXSPLTW > - UNSPEC_VSX_XXSPLTD > UNSPEC_VSX_DIVSD > UNSPEC_VSX_DIVUD > UNSPEC_VSX_DIVSQ Ok. > @@ -4673,16 +4672,18 @@ (define_insn "vsx_vsplt_di" > ;; V2DF/V2DI splat for use by vec_splat builtin > (define_insn "vsx_xxspltd_" >[(set (match_operand:VSX_D 0 "vsx_register_operand" "
[PATCH, V3] Optimize vec_splats of constant vec_extract for V2DI/V2DF, PR target 99293
Optimize vec_splats of constant vec_extract for V2DI/V2DF, PR target 99293. This is version 3 of the patch. The original patch was: | Date: Mon, 28 Mar 2022 12:26:02 -0400 | Subject: [PATCH 1/4] Optimize vec_splats of constant vec_extract for V2DI/V2DF, PR target 99293. | Message-ID: | https://gcc.gnu.org/pipermail/gcc-patches/2022-March/592420.html Version 2 of the patch was: | Date: Fri, 13 May 2022 10:49:26 -0400 | Subject: [PATCH] Optimize vec_splats of constant V2DI/V2DF vec_extract, PR target/99293 | Message-ID: | https://gcc.gnu.org/pipermail/gcc-patches/2022-May/594797.html The differences between version 2 and version 3 was to clean up the description of what the patch does, and to make the example test case clear. In PR target/99293, it was pointed out that doing: vector long long dest0, dest1, src; /* ... */ dest0 = vec_splats (vec_extract (src, 0)); dest1 = vec_splats (vec_extract (src, 1)); would generate slower code. It generates the following code on power8: ;; vec_splats (vec_extract (src, 0)) xxpermdi 0,34,34,3 xxpermdi 34,0,0,0 ;; vec_splats (vec_extract (src, 1)) xxlor 0,34,34 xxpermdi 34,0,0,0 However on power9 and power10 it generates: ;; vec_splats (vec_extract (src, 0)) mfvsld 3,34 mtvsrdd 34,9,9 ;; vec_splats (vec_extract (src, 1)) mfvsrd 9,34 mtvsrdd 34,9,9 This is due to the power9 having the mfvsrld instruction which can extract either 64-bit element into a GPR. While there are alternatives for both vector registers and GPR registers, the register allocator prefers to put DImode into GPR registers. In this case, it is better to have a single combiner pattern that can generate a single xxpermdi, instead of 2 insnsns (the extract and then the concat). This is true if the two operations are move from vector register and move to vector register. As Segher pointed out in a previous version of the patch, the combiner already tries doing creating a (vec_duplicate (vec_select ...)) pattern, but we didn't provide one. This patch reworks vsx_xxspltd_ for V2DImode and V2DFmode so that it now uses VEC_DUPLICATE, which the combiner checks for. I have built Spec 2017 with this patch installed, and the cam4_r benchmark is the only benchmark that generated different code (3 mfvsrld/mtvsrdd pairs of instructions were replaced with xxpermdi). I have built bootstrap versions on the following systems and I have run the regression tests. There were no regressions in the runs: Power9 little endian, --with-cpu=power9 Power10 little endian, --with-cpu=power10 Power8 big endian, --with-cpu=power8 (both 32-bit & 64-bit tests) Can I install this into the trunk? After a burn-in period, can I backport and install this into GCC 11 and GCC 10 branches? 2022-06-06 Michael Meissner gcc/ PR target/99293 * config/rs6000/rs6000-p8swap.cc (rtx_is_swappable_p): Remove UNSPEC_VSX_XXSPLTD case. * config/rs6000/vsx.md (UNSPEC_VSX_XXSPLTD): Delete. (vsx_xxspltd_): Rewrite to use VEC_DUPLICATE. gcc/testsuite: PR target/99293 * gcc.target/powerpc/builtins-1.c: Update insn count. * gcc.target/powerpc/pr99293.c: New test. --- gcc/config/rs6000/rs6000-p8swap.cc| 1 - gcc/config/rs6000/vsx.md | 19 +++ gcc/testsuite/gcc.target/powerpc/builtins-1.c | 2 +- gcc/testsuite/gcc.target/powerpc/pr99293.c| 51 +++ 4 files changed, 62 insertions(+), 11 deletions(-) create mode 100644 gcc/testsuite/gcc.target/powerpc/pr99293.c diff --git a/gcc/config/rs6000/rs6000-p8swap.cc b/gcc/config/rs6000/rs6000-p8swap.cc index 275702fee1b..3160fcbdeca 100644 --- a/gcc/config/rs6000/rs6000-p8swap.cc +++ b/gcc/config/rs6000/rs6000-p8swap.cc @@ -807,7 +807,6 @@ rtx_is_swappable_p (rtx op, unsigned int *special) case UNSPEC_VUPKLU_V4SF: return 0; case UNSPEC_VSPLT_DIRECT: - case UNSPEC_VSX_XXSPLTD: *special = SH_SPLAT; return 1; case UNSPEC_REDUC_PLUS: diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md index 1b75538f42f..a1a1ce95195 100644 --- a/gcc/config/rs6000/vsx.md +++ b/gcc/config/rs6000/vsx.md @@ -296,7 +296,6 @@ (define_c_enum "unspec" UNSPEC_VSX_XXPERM UNSPEC_VSX_XXSPLTW - UNSPEC_VSX_XXSPLTD UNSPEC_VSX_DIVSD UNSPEC_VSX_DIVUD UNSPEC_VSX_DIVSQ @@ -4673,16 +4672,18 @@ (define_insn "vsx_vsplt_di" ;; V2DF/V2DI splat for use by vec_splat builtin (define_insn "vsx_xxspltd_" [(set (match_operand:VSX_D 0 "vsx_register_operand" "=wa") -(unspec:VSX_D [(match_operand:VSX_D 1 "vsx_register_operand" "wa") - (match_operand:QI 2 "u5bit_cint_operand" "i")] - UNSPEC_VSX_XXSPLTD))] + (vec_duplicate:VSX_D +(vec_select: + (match_operand:VSX_D 1 "gpc_reg_operand" "wa")
