[PATCH 0/4] Optimize vec_splats of vec_extract, PR target/99293

[PATCH 0/4] Optimize vec_splats of vec_extract, PR target/99293

[Michael Meissner]

The following 4 patches fix PR target/99293.  This bug complains that on power9
and power10:

	vector long long v, v0, v1;
	// ...
	v0 = __builtin_vec_splats (__builtin_vec_extract (v, 0));
	v1 = __builtin_vec_splats (__builtin_vec_extract (v, 1));

generates move from vector register and move to vector register instructions
instead of keeping the data within the vector registers.

The first patch adds a combiner patterns to match this case and generate a
single xxpermdi instruction, instead of two instructions (the extract and then
the splats operations).

The second and third patches fix the insn attributes to be correct in the
extract and concat operations.

The fourth patch allows the target to be traditional Altivec registers in
addition to traditional floating point registers and GPRs.

I have built bootstrap versions on the following systems with these patches
applied.  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)

-- 
Michael Meissner, IBM
PO Box 98, Ayer, Massachusetts, USA, 01432
email: meissner@linux.ibm.com

[PATCH 1/4] Optimize vec_splats of constant vec_extract for V2DI/V2DF, PR target 99293.

[Michael Meissner]

Optimize vec_splats of constant vec_extract for V2DI/V2DF, PR target 99293.

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.

However in this case, it is better to have a single combiner pattern that
can generate a single xxpermdi, instead of doing 2 insnsns (the extract
and then the concat).  This is particularly true if the two operations are
move from vector register and move to vector register.

I have built Spec 2017 with this patch installed, and the cam4_r benchmark
is the only benchmark that generated different code.  On a power9, I did
not notice any significant changes in the runtime of cam4_r.

I have built bootstrap versions on the following systems.  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-03-28   Michael Meissner  <meissner@linux.ibm.com>

gcc/
	PR target/99293
	* config/rs6000/vsx.md (vsx_splat_const_extract_<mode>): New
	combiner insn.

gcc/testsuite:
	PR target/99293
	* gcc.target/powerpc/pr99293.c: New test.
---
 gcc/config/rs6000/vsx.md                   | 19 ++++++++++++
 gcc/testsuite/gcc.target/powerpc/pr99293.c | 36 ++++++++++++++++++++++
 2 files changed, 55 insertions(+)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pr99293.c

diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md
index 15bd86dfdfb..ddafbe471dd 100644
--- a/gcc/config/rs6000/vsx.md
+++ b/gcc/config/rs6000/vsx.md
@@ -4616,6 +4616,25 @@ (define_insn "vsx_splat_v4si_di"
   [(set_attr "type" "vecperm")
    (set_attr "isa" "p8v,*")])
 
+;; Optimize SPLAT of an extract from a V2DF/V2DI vector with a constant element
+;; PR target/99293
+(define_insn "*vsx_splat_const_extract_<mode>"
+  [(set (match_operand:VSX_D 0 "vsx_register_operand" "=wa")
+	(vec_duplicate:VSX_D
+	 (vec_select:<VS_scalar>
+	  (match_operand:VSX_D 1 "vsx_register_operand" "wa")
+	  (parallel [(match_operand 2 "const_0_to_1_operand" "n")]))))]
+  "VECTOR_MEM_VSX_P (<MODE>mode)"
+{
+  int which_word = INTVAL (operands[2]);
+  if (!BYTES_BIG_ENDIAN)
+    which_word = 1 - which_word;
+
+  operands[3] = GEN_INT (which_word ? 3 : 0);
+  return "xxpermdi %x0,%x1,%x1,%3";
+}
+  [(set_attr "type" "vecperm")])
+
 ;; V4SF splat (ISA 3.0)
 (define_insn_and_split "vsx_splat_v4sf"
   [(set (match_operand:V4SF 0 "vsx_register_operand" "=wa,wa,wa")
diff --git a/gcc/testsuite/gcc.target/powerpc/pr99293.c b/gcc/testsuite/gcc.target/powerpc/pr99293.c
new file mode 100644
index 00000000000..13b5ed5b0b1
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr99293.c
@@ -0,0 +1,36 @@
+/* { dg-do compile { target powerpc*-*-* } } */
+/* { dg-require-effective-target powerpc_vsx_ok } */
+/* { dg-options "-O2 -mvsx" } */
+
+/* Test for PR 99263, which wants to do:
+   __builtin_vec_splats (__builtin_vec_extract (v, n))
+
+   where v is a V2DF or V2DI vector and n is either 0 or 1.  Previously the
+   compiler would do a direct move to the GPR registers to select the item and
+   a direct move from the GPR registers to do the splat.  */
+
+vector long long
+splat_dup_ll_0 (vector long long v)
+{
+  return __builtin_vec_splats (__builtin_vec_extract (v, 0));
+}
+	
+vector long long
+splat_dup_ll_1 (vector long long v)
+{
+  return __builtin_vec_splats (__builtin_vec_extract (v, 1));
+}
+
+vector double
+splat_dup_d_0 (vector double v)
+{
+  return __builtin_vec_splats (__builtin_vec_extract (v, 0));
+}
+	
+vector double
+splat_dup_d_1 (vector double v)
+{
+  return __builtin_vec_splats (__builtin_vec_extract (v, 1));
+}
+	
+/* { dg-final { scan-assembler-times "xxpermdi" 4 } } */
-- 
2.35.1


-- 
Michael Meissner, IBM
PO Box 98, Ayer, Massachusetts, USA, 01432
email: meissner@linux.ibm.com

[PATCH 2/4] Make vsx_splat_<mode>_reg use correct insn attributes, PR target/99293

[Michael Meissner]

Make vsx_splat_<mode>_reg use correct insn attributes, PR target/99293

In looking at PR target/99293, I noticed that the code in the insn
vsx_splat_<mode>_reg used "vecmove" as the "type" insn attribute when the
"mtvsrdd" is generated.  It should use "mfvsr".  I also added a "p9v" isa
attribute for that alternative.

I have built the spec 2017 benchmark with this patch (#2) and the next
patch (#3), along with the first patch in the series for power9 and
power10 targets.  Most of the floating point benchmarks changed code
slightly, due to the scheduling changes that came from changing the insn
type attribute.  I ran the spec 2017 suite on power9, and I did not not
notice any significant changes from these changes.

The power9 benchmarks that had code differences with these 2 patches
applied in addition to the build with just the first patch applied were:

	namd_r, pareset_r, povray_r, wrf_r, blender_r, cam4_r,
	deepsjeng_r, imagick_r, roms_r

The power9 benchmarks that had code differences with these 2 patches
applied in addition to the build with just the first patch applied were
(cactuBSSN_r had changes for power10 but not power9):

	cactuBSSN_r, namd_r, pareset_r, povray_r, wrf_r, blender_r,
	cam4_r, deepsjeng_r, imagick_r, nab_r, roms_r

I have built bootstrap versions on the following systems.  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-03-28   Michael Meissner  <meissner@linux.ibm.com>

gcc/
	PR target/99293
	* config/rs6000/rs6000.md (vsx_splat_<mode>_reg): Use the correct
	insn type attribute.  Add "p9v" isa attribute for generating the
	mtvsrdd instruction.
---
 gcc/config/rs6000/vsx.md | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md
index ddafbe471dd..ad722cff70f 100644
--- a/gcc/config/rs6000/vsx.md
+++ b/gcc/config/rs6000/vsx.md
@@ -4580,7 +4580,8 @@ (define_insn "vsx_splat_<mode>_reg"
   "@
    xxpermdi %x0,%x1,%x1,0
    mtvsrdd %x0,%1,%1"
-  [(set_attr "type" "vecperm,vecmove")])
+  [(set_attr "type" "vecperm,mtvsr")
+   (set_attr "isa" "*,p9v")])
 
 (define_insn "vsx_splat_<mode>_mem"
   [(set (match_operand:VSX_D 0 "vsx_register_operand" "=wa")
-- 
2.35.1


-- 
Michael Meissner, IBM
PO Box 98, Ayer, Massachusetts, USA, 01432
email: meissner@linux.ibm.com

[PATCH 3/4] Make vsx_extract_<mode> use correct insn attributes, PR target 99293.

[Michael Meissner]

Make vsx_extract_<mode> use correct insn attributes, PR target 99293.

In looking at PR target/99293, I noticed that the insn "type" attribute is
incorrect for the vsx_extract_<mode> insns.  In particular:

    1)	Simple vector register move should be vecmove (alternative 1);
    2)	Xxpermdi should be vecperm (alternative 2); (and)
    3)	Mfvsrld should be mfvsr (alternative 4).

This patch fixes those attributes.

I have built the spec 2017 benchmark with this patch (#3) and the previous
patch (#2), along with the first patch in the series for power9 and
power10 targets.  Most of the floating point benchmarks changed code
slightly, due to the scheduling changes that came from changing the insn
type attribute.  I ran the spec 2017 suite on power9, and I did not not
notice any significant changes from these changes.

The power9 benchmarks that had code differences with these 2 patches
applied in addition to the build with just the first patch applied were:

	namd_r, pareset_r, povray_r, wrf_r, blender_r, cam4_r,
	deepsjeng_r, imagick_r, roms_r

The power9 benchmarks that had code differences with these 2 patches
applied in addition to the build with just the first patch applied were
(cactuBSSN_r had changes for power10 but not power9):

	cactuBSSN_r, namd_r, pareset_r, povray_r, wrf_r, blender_r,
	cam4_r, deepsjeng_r, imagick_r, nab_r, roms_r

I have built bootstrap versions on the following systems.  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-03-28   Michael Meissner  <meissner@linux.ibm.com>

gcc/
	PR target/99293
	* config/rs6000/rs6000.md (vsx_extract_<mode>): Use the correct
	insn type for the alternatives.
---
 gcc/config/rs6000/vsx.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md
index ad722cff70f..2a23807c2dc 100644
--- a/gcc/config/rs6000/vsx.md
+++ b/gcc/config/rs6000/vsx.md
@@ -3451,7 +3451,7 @@ (define_insn "vsx_extract_<mode>"
   else
     gcc_unreachable ();
 }
-  [(set_attr "type" "veclogical,mfvsr,mfvsr,vecperm")
+  [(set_attr "type" "vecmove,vecperm,mfvsr,mfvsr")
    (set_attr "isa" "*,*,p8v,p9v")])
 
 ;; Optimize extracting a single scalar element from memory.
-- 
2.35.1


-- 
Michael Meissner, IBM
PO Box 98, Ayer, Massachusetts, USA, 01432
email: meissner@linux.ibm.com

[PATCH 4/4] Allow vsx_extract_<mode> to use Altivec registers, PR target/99293

[Michael Meissner]

Allow vsx_extract_<mode> to use Altivec registers, PR target/99293

In looking at PR target/99293, I noticed that the vsx_extract_<mode>
pattern for V2DImode and V2DFmode only allowed traditional floating point
registers, and it did not allow Altivec registers.  The original code was
written a few years ago when we used the old register allocator, and
support for scalar floating point in Altivec registers was just being
added to GCC.

I have built the spec 2017 benchmark suite With all 4 patches in this
series applied, and compared it to the build with the previous 3 patches
applied.  In addition to the changes from the previous 3 patches, this
patch now changes the code for the following 3 benchmarks (2 floating
point, 1 integer):

	bwaves_r, fotonik3d_r, xalancbmk_r

I have built bootstrap versions on the following systems.  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-03-28   Michael Meissner  <meissner@linux.ibm.com>

gcc/
	PR target/99293
	* config/rs6000/rs6000.md (vsx_extract_<mode>): Allow destination
	to be an Altivec register.
---
 gcc/config/rs6000/vsx.md | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md
index 2a23807c2dc..d30fd4f2596 100644
--- a/gcc/config/rs6000/vsx.md
+++ b/gcc/config/rs6000/vsx.md
@@ -3397,15 +3397,12 @@ (define_expand "vsx_set_<mode>"
 ;; Optimize cases were we can do a simple or direct move.
 ;; Or see if we can avoid doing the move at all
 
-;; There are some unresolved problems with reload that show up if an Altivec
-;; register was picked.  Limit the scalar value to FPRs for now.
-
 (define_insn "vsx_extract_<mode>"
-  [(set (match_operand:<VS_scalar> 0 "gpc_reg_operand" "=d, d,  wr, wr")
+  [(set (match_operand:<VS_scalar> 0 "gpc_reg_operand" "=wa, wa, wr, wr")
 	(vec_select:<VS_scalar>
-	 (match_operand:VSX_D 1 "gpc_reg_operand"      "wa, wa, wa, wa")
+	 (match_operand:VSX_D 1 "gpc_reg_operand"       "wa, wa, wa, wa")
 	 (parallel
-	  [(match_operand:QI 2 "const_0_to_1_operand"  "wD, n,  wD, n")])))]
+	  [(match_operand:QI 2 "const_0_to_1_operand"   "wD, n,  wD, n")])))]
   "VECTOR_MEM_VSX_P (<MODE>mode)"
 {
   int element = INTVAL (operands[2]);
-- 
2.35.1


-- 
Michael Meissner, IBM
PO Box 98, Ayer, Massachusetts, USA, 01432
email: meissner@linux.ibm.com

Re: [PATCH 1/4] Optimize vec_splats of constant vec_extract for V2DI/V2DF, PR target 99293.

[Segher Boessenkool]

On Mon, Mar 28, 2022 at 12:26:02PM -0400, Michael Meissner wrote:
> 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.

As I said in comment 2 in the PR, it is because we do not have this
pattern yet, we only have vec_concat.  The instruction combiner tries
to use this pattern, but it doesn't exist :-)

> +;; Optimize SPLAT of an extract from a V2DF/V2DI vector with a constant element
> +;; PR target/99293
> +(define_insn "*vsx_splat_const_extract_<mode>"
> +  [(set (match_operand:VSX_D 0 "vsx_register_operand" "=wa")
> +	(vec_duplicate:VSX_D
> +	 (vec_select:<VS_scalar>
> +	  (match_operand:VSX_D 1 "vsx_register_operand" "wa")
> +	  (parallel [(match_operand 2 "const_0_to_1_operand" "n")]))))]
> +  "VECTOR_MEM_VSX_P (<MODE>mode)"
> +{
> +  int which_word = INTVAL (operands[2]);

dword, not word.

> +  if (!BYTES_BIG_ENDIAN)
> +    which_word = 1 - which_word;
> +
> +  operands[3] = GEN_INT (which_word ? 3 : 0);
> +  return "xxpermdi %x0,%x1,%x1,%3";

Please use gen_vsx_xxspltd_v2di, instead.  Which itself should not use
an unspec of course, but that is another patch.

> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr99293.c
> @@ -0,0 +1,36 @@
> +/* { dg-do compile { target powerpc*-*-* } } */

Don't.  This is gcc.target/powerpc/ already.

> +/* { dg-final { scan-assembler-times "xxpermdi" 4 } } */

\m \M

Thanks,


Segher

Re: [PATCH 2/4] Make vsx_splat_<mode>_reg use correct insn attributes, PR target/99293

[Segher Boessenkool]

Hi!

On Mon, Mar 28, 2022 at 12:27:05PM -0400, Michael Meissner wrote:
> In looking at PR target/99293, I noticed that the code in the insn
> vsx_splat_<mode>_reg used "vecmove" as the "type" insn attribute when the
> "mtvsrdd" is generated.  It should use "mfvsr".  I also added a "p9v" isa
> attribute for that alternative.

s/mfvsr/mtvsr/

But, mtvsrd and mtvsrdd have very different scheduling properties (like,
on p10 it is 1 cycle vs. 3 cycles).

Also, there are two insn patterns for mtvsrdd, and you are only touching
one here.

> 	* config/rs6000/rs6000.md (vsx_splat_<mode>_reg): Use the correct
> 	insn type attribute.  Add "p9v" isa attribute for generating the
> 	mtvsrdd instruction.

This is in vsx.md, instead.

> --- a/gcc/config/rs6000/vsx.md
> +++ b/gcc/config/rs6000/vsx.md
> @@ -4580,7 +4580,8 @@ (define_insn "vsx_splat_<mode>_reg"
>    "@
>     xxpermdi %x0,%x1,%x1,0
>     mtvsrdd %x0,%1,%1"
> -  [(set_attr "type" "vecperm,vecmove")])
> +  [(set_attr "type" "vecperm,mtvsr")
> +   (set_attr "isa" "*,p9v")])

"we" requires "p9v".  Please do a full conversion when getting rid of
this?  That includes requiring TARGET_POWERPC64 for it (not -m64 as its
documentation says; the existing implementation of "we" is correct).


Segher

Re: [PATCH 3/4] Make vsx_extract_<mode> use correct insn attributes, PR target 99293.

[Segher Boessenkool]

Hi!

On Mon, Mar 28, 2022 at 12:28:04PM -0400, Michael Meissner wrote:
> In looking at PR target/99293, I noticed that the insn "type" attribute is
> incorrect for the vsx_extract_<mode> insns.  In particular:
> 
>     1)	Simple vector register move should be vecmove (alternative 1);
>     2)	Xxpermdi should be vecperm (alternative 2); (and)
>     3)	Mfvsrld should be mfvsr (alternative 4).
> 
> This patch fixes those attributes.

But the code does not correspond well to the alternatives, even worse
for BE.  It would be much clearer (and even possibly correct!) if it
would just use the alternative # in the code, instead of using twenty
different conditions.  There are some important cases that have no
alternative right now, like, when op 1 is the same as op 0: it should
have the constraint "0" for op 1 then, and have cost 0.  The register
allocator will then hopefully try to make that happen.

> --- a/gcc/config/rs6000/vsx.md
> +++ b/gcc/config/rs6000/vsx.md
> @@ -3451,7 +3451,7 @@ (define_insn "vsx_extract_<mode>"
>    else
>      gcc_unreachable ();
>  }
> -  [(set_attr "type" "veclogical,mfvsr,mfvsr,vecperm")
> +  [(set_attr "type" "vecmove,vecperm,mfvsr,mfvsr")
>     (set_attr "isa" "*,*,p8v,p9v")])

The generated code is one of
  no-op
  mfvsrd
  fmr
  xxlor
  mfvsrld
  xxpermdi

Which of the 4 alts are meant to correspond to which of those six
possible generated pieces of code?


Segher

Re: [PATCH 1/4] Optimize vec_splats of constant vec_extract for V2DI/V2DF, PR target 99293.

[Michael Meissner]

On Mon, Mar 28, 2022 at 12:14:09PM -0500, Segher Boessenkool wrote:
> On Mon, Mar 28, 2022 at 12:26:02PM -0400, Michael Meissner wrote:
> > 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.
> 
> As I said in comment 2 in the PR, it is because we do not have this
> pattern yet, we only have vec_concat.  The instruction combiner tries
> to use this pattern, but it doesn't exist :-)

Ok.  I will create that function and see if it works.

> > +;; Optimize SPLAT of an extract from a V2DF/V2DI vector with a constant element
> > +;; PR target/99293
> > +(define_insn "*vsx_splat_const_extract_<mode>"
> > +  [(set (match_operand:VSX_D 0 "vsx_register_operand" "=wa")
> > +	(vec_duplicate:VSX_D
> > +	 (vec_select:<VS_scalar>
> > +	  (match_operand:VSX_D 1 "vsx_register_operand" "wa")
> > +	  (parallel [(match_operand 2 "const_0_to_1_operand" "n")]))))]
> > +  "VECTOR_MEM_VSX_P (<MODE>mode)"
> > +{
> > +  int which_word = INTVAL (operands[2]);
> 
> dword, not word.

Ok.

> > +  if (!BYTES_BIG_ENDIAN)
> > +    which_word = 1 - which_word;
> > +
> > +  operands[3] = GEN_INT (which_word ? 3 : 0);
> > +  return "xxpermdi %x0,%x1,%x1,%3";
> 
> Please use gen_vsx_xxspltd_v2di, instead.  Which itself should not use
> an unspec of course, but that is another patch.

There is also vsx_concat_<mode>_3 which does not use an UNSPEC.  Maybe
eventually rewrite vsx_xxspltd_v2di to be a define_insn_and_split.

> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/powerpc/pr99293.c
> > @@ -0,0 +1,36 @@
> > +/* { dg-do compile { target powerpc*-*-* } } */
> 
> Don't.  This is gcc.target/powerpc/ already.

Ok.

> > +/* { dg-final { scan-assembler-times "xxpermdi" 4 } } */
> 
> \m \M

Ok.

-- 
Michael Meissner, IBM
PO Box 98, Ayer, Massachusetts, USA, 01432
email: meissner@linux.ibm.com

Re: [PATCH 1/4] Optimize vec_splats of constant vec_extract for V2DI/V2DF, PR target 99293.

[Segher Boessenkool]

On Mon, Mar 28, 2022 at 06:30:41PM -0400, Michael Meissner wrote:
> On Mon, Mar 28, 2022 at 12:14:09PM -0500, Segher Boessenkool wrote:
> > On Mon, Mar 28, 2022 at 12:26:02PM -0400, Michael Meissner wrote:
> > > 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.
> > 
> > As I said in comment 2 in the PR, it is because we do not have this
> > pattern yet, we only have vec_concat.  The instruction combiner tries
> > to use this pattern, but it doesn't exist :-)
> 
> Ok.  I will create that function and see if it works.

That is what this patch *does*!  *That* needs to be in the commit
message!

> > > +  if (!BYTES_BIG_ENDIAN)
> > > +    which_word = 1 - which_word;
> > > +
> > > +  operands[3] = GEN_INT (which_word ? 3 : 0);
> > > +  return "xxpermdi %x0,%x1,%x1,%3";
> > 
> > Please use gen_vsx_xxspltd_v2di, instead.  Which itself should not use
> > an unspec of course, but that is another patch.
> 
> There is also vsx_concat_<mode>_3 which does not use an UNSPEC.  Maybe
> eventually rewrite vsx_xxspltd_v2di to be a define_insn_and_split.

It almost always is a bad idea to have a 1->1 split: you are rewriting
the canonical way to write something to something non-canonical.  (Where
"canonical" means "actually canonical, and/or what GCC generates
anyway").

It doesn't save anything over an extra define_insn either, anyway?


Segher

Re: [PATCH 4/4] Allow vsx_extract_<mode> to use Altivec registers, PR target/99293

[Segher Boessenkool]

On Mon, Mar 28, 2022 at 12:28:55PM -0400, Michael Meissner wrote:
> In looking at PR target/99293, I noticed that the vsx_extract_<mode>
> pattern for V2DImode and V2DFmode only allowed traditional floating point
> registers, and it did not allow Altivec registers.  The original code was
> written a few years ago when we used the old register allocator, and
> support for scalar floating point in Altivec registers was just being
> added to GCC.

vsx_extract_<mode> is from 2009...  How time flies :-)

This comment is from 2016 though.  Still before LRA was default for us
of course ;-)

If would have been nice if we had a testcase for this breakage, so that
we could now be confident it really has been fixed.  But the "reload"
here likely means "old reload", so okay.

> 	PR target/99293

It has essentially nothing to do with that PR, right?  Or I just do not
see it, always a possibility of course.

> 	* config/rs6000/rs6000.md (vsx_extract_<mode>): Allow destination
> 	to be an Altivec register.

... to be any VSX register.

Okay for trunk with those things fixed.  Thanks!


Segher

Re: [PATCH 4/4] Allow vsx_extract_<mode> to use Altivec registers, PR target/99293

[Michael Meissner]

On Mon, Mar 28, 2022 at 06:59:14PM -0500, Segher Boessenkool wrote:
> On Mon, Mar 28, 2022 at 12:28:55PM -0400, Michael Meissner wrote:
> > In looking at PR target/99293, I noticed that the vsx_extract_<mode>
> > pattern for V2DImode and V2DFmode only allowed traditional floating point
> > registers, and it did not allow Altivec registers.  The original code was
> > written a few years ago when we used the old register allocator, and
> > support for scalar floating point in Altivec registers was just being
> > added to GCC.
> 
> vsx_extract_<mode> is from 2009...  How time flies :-)
> 
> This comment is from 2016 though.  Still before LRA was default for us
> of course ;-)

The support for scalars in Altivec registers wasn't really done until the 2016
time frame.  At the time I had tried to use VSX registers for this, but I could
never get a reproducable case for the failure other one spec benchmark not
building with some flags (most likely spec 2017's 521.wrf_r or spec 2006's
481.wrf).  So I opted to just keep it limited to traditional FPR registers, and
maybe fix it some time later.

> If would have been nice if we had a testcase for this breakage, so that
> we could now be confident it really has been fixed.  But the "reload"
> here likely means "old reload", so okay.

Yes, it was the old reload.

> > 	PR target/99293
> 
> It has essentially nothing to do with that PR, right?  Or I just do not
> see it, always a possibility of course.

It was just that I noticed the change in looking at PR target/99293.  I did
remove the reference from the checkin commit.

> > 	* config/rs6000/rs6000.md (vsx_extract_<mode>): Allow destination
> > 	to be an Altivec register.
> 
> ... to be any VSX register.

Thanks.

> Okay for trunk with those things fixed.  Thanks!

Done.

-- 
Michael Meissner, IBM
PO Box 98, Ayer, Massachusetts, USA, 01432
email: meissner@linux.ibm.com

Re: [PATCH 2/4] Make vsx_splat_<mode>_reg use correct insn attributes, PR target/99293

[Michael Meissner]

On Mon, Mar 28, 2022 at 03:28:39PM -0500, Segher Boessenkool wrote:
> Hi!
> 
> On Mon, Mar 28, 2022 at 12:27:05PM -0400, Michael Meissner wrote:
> > In looking at PR target/99293, I noticed that the code in the insn
> > vsx_splat_<mode>_reg used "vecmove" as the "type" insn attribute when the
> > "mtvsrdd" is generated.  It should use "mfvsr".  I also added a "p9v" isa
> > attribute for that alternative.
> 
> s/mfvsr/mtvsr/
> 
> But, mtvsrd and mtvsrdd have very different scheduling properties (like,
> on p10 it is 1 cycle vs. 3 cycles).


I must admit, I assumed vecmove was a stand-in for XXMR (i.e. XXLOR).  Since
its use is used for other cases (mtvsrdd, xxsel/vsel, x{s,v}abs*, x{s,v}nabs*,
xsiexpq*), it is probably better to just let things lie, and perhaps relook at
it in the GCC 13 time frame.
 
> Also, there are two insn patterns for mtvsrdd, and you are only touching
> one here.

I think you meant that comment about the third patch (to vsx_extract_<mode>)
and not to this patch (to vsx_splat_<mode>_reg) where there are only two
alternatives (the first being xxpermdi and the second being mtvsrdd).

> > 	* config/rs6000/rs6000.md (vsx_splat_<mode>_reg): Use the correct
> > 	insn type attribute.  Add "p9v" isa attribute for generating the
> > 	mtvsrdd instruction.
> 
> This is in vsx.md, instead.
> 
> > --- a/gcc/config/rs6000/vsx.md
> > +++ b/gcc/config/rs6000/vsx.md
> > @@ -4580,7 +4580,8 @@ (define_insn "vsx_splat_<mode>_reg"
> >    "@
> >     xxpermdi %x0,%x1,%x1,0
> >     mtvsrdd %x0,%1,%1"
> > -  [(set_attr "type" "vecperm,vecmove")])
> > +  [(set_attr "type" "vecperm,mtvsr")
> > +   (set_attr "isa" "*,p9v")])
> 
> "we" requires "p9v".  Please do a full conversion when getting rid of
> this?  That includes requiring TARGET_POWERPC64 for it (not -m64 as its
> documentation says; the existing implementation of "we" is correct).

That is more complex, and likely it should be a GCC 13 thing.  Off the top of
my head, we would need a new "isa" variant (p9v64) that combines p9v and
64-bit.  Originally, I had changed the "we" to "wa", but then I realized it
wouldn't work for 32-bit, but I left in setting the alternative.

-- 
Michael Meissner, IBM
PO Box 98, Ayer, Massachusetts, USA, 01432
email: meissner@linux.ibm.com

Re: [PATCH 3/4] Make vsx_extract_<mode> use correct insn attributes, PR target 99293.

[Michael Meissner]

On Mon, Mar 28, 2022 at 05:06:00PM -0500, Segher Boessenkool wrote:
> Hi!
> 
> On Mon, Mar 28, 2022 at 12:28:04PM -0400, Michael Meissner wrote:
> > In looking at PR target/99293, I noticed that the insn "type" attribute is
> > incorrect for the vsx_extract_<mode> insns.  In particular:
> > 
> >     1)	Simple vector register move should be vecmove (alternative 1);
> >     2)	Xxpermdi should be vecperm (alternative 2); (and)
> >     3)	Mfvsrld should be mfvsr (alternative 4).
> > 
> > This patch fixes those attributes.
> 
> But the code does not correspond well to the alternatives, even worse
> for BE.  It would be much clearer (and even possibly correct!) if it
> would just use the alternative # in the code, instead of using twenty
> different conditions.  There are some important cases that have no
> alternative right now, like, when op 1 is the same as op 0: it should
> have the constraint "0" for op 1 then, and have cost 0.  The register
> allocator will then hopefully try to make that happen.
> 
> > --- a/gcc/config/rs6000/vsx.md
> > +++ b/gcc/config/rs6000/vsx.md
> > @@ -3451,7 +3451,7 @@ (define_insn "vsx_extract_<mode>"
> >    else
> >      gcc_unreachable ();
> >  }
> > -  [(set_attr "type" "veclogical,mfvsr,mfvsr,vecperm")
> > +  [(set_attr "type" "vecmove,vecperm,mfvsr,mfvsr")
> >     (set_attr "isa" "*,*,p8v,p9v")])
> 
> The generated code is one of
>   no-op
>   mfvsrd
>   fmr
>   xxlor
>   mfvsrld
>   xxpermdi
> 
> Which of the 4 alts are meant to correspond to which of those six
> possible generated pieces of code?

The mop comment, fmr, and xxlor are all alternative 1 (target is a VSX
register, element number is which double-word that that is in the upper 64-bits
of the register).

The xxpermdi is alternative 2 (target is a VSX register, elment number is which
double-word that is in the lower 64-bits of the register).

The mfvsrd is alternative 3 (target is a GPR register, element number is which
double-word that is in the upper 64-bits of the register).

The mfvsrld is alternative 4 (target is a GPR register, elment number is which
double-word that is in the lower 64-bits of the register).

In terms of this patch, I had originally rewritten the insn as a
define_insn_and_split, and changed the move case into just a move to let the
later passes just delete moves to the same register.  However, it caused some
unrelated errors (in the gcc.dg/tree-ssa/builtin-s*printf tests), so I backed
out the code to use split to create moves.

But it looks like alternative 2 and alternative 4 have the insn attribute
"type" mixed up (alternative 2 should be vecperm, alternative 4 should be move
from vsr).



-- 
Michael Meissner, IBM
PO Box 98, Ayer, Massachusetts, USA, 01432
email: meissner@linux.ibm.com

Re: [PATCH 2/4] Make vsx_splat_<mode>_reg use correct insn attributes, PR target/99293

[Segher Boessenkool]

On Wed, Mar 30, 2022 at 06:41:59PM -0400, Michael Meissner wrote:
> On Mon, Mar 28, 2022 at 03:28:39PM -0500, Segher Boessenkool wrote:
> > On Mon, Mar 28, 2022 at 12:27:05PM -0400, Michael Meissner wrote:
> > > In looking at PR target/99293, I noticed that the code in the insn
> > > vsx_splat_<mode>_reg used "vecmove" as the "type" insn attribute when the
> > > "mtvsrdd" is generated.  It should use "mfvsr".  I also added a "p9v" isa
> > > attribute for that alternative.
> > 
> > s/mfvsr/mtvsr/
> > 
> > But, mtvsrd and mtvsrdd have very different scheduling properties (like,
> > on p10 it is 1 cycle vs. 3 cycles).
> 
> I must admit, I assumed vecmove was a stand-in for XXMR (i.e. XXLOR).

That is "veclogical".  I don't think there is any core where this is
optimised specially?

> Since
> its use is used for other cases (mtvsrdd, xxsel/vsel, x{s,v}abs*, x{s,v}nabs*,
> xsiexpq*), it is probably better to just let things lie, and perhaps relook at
> it in the GCC 13 time frame.

Yes, we need to make better categories.  The problem is to come up with
something that is close enough to what the relevant cores actually do,
but in such a way that we do not end up with gazillions of nonsensical
separate instruction types.

What we care about most for p9 and p10 vector insns is whether something
is a 3-cycle op or not.  But this differs per core, and in ways that
are a little ad-hoc (looked at from far away anyway).

For the integer insns we ended up with extra attributes (not just
"type"), which is both compact and expressive.  We should try to do
something like that for vector ops as well.  We now have both p9 an
p10, with two implementations it should be clearer what a good direction
to take will be here.

> > Also, there are two insn patterns for mtvsrdd, and you are only touching
> > one here.
> 
> I think you meant that comment about the third patch (to vsx_extract_<mode>)
> and not to this patch (to vsx_splat_<mode>_reg) where there are only two
> alternatives (the first being xxpermdi and the second being mtvsrdd).

I mean vsx_concat_<mode> and vsx_splat_<mode>_reg.  Both have mtvsrdd
(both as alternative 1), but you only update the "type" of the latter
here.

> > > --- a/gcc/config/rs6000/vsx.md
> > > +++ b/gcc/config/rs6000/vsx.md
> > > @@ -4580,7 +4580,8 @@ (define_insn "vsx_splat_<mode>_reg"
> > >    "@
> > >     xxpermdi %x0,%x1,%x1,0
> > >     mtvsrdd %x0,%1,%1"
> > > -  [(set_attr "type" "vecperm,vecmove")])
> > > +  [(set_attr "type" "vecperm,mtvsr")
> > > +   (set_attr "isa" "*,p9v")])
> > 
> > "we" requires "p9v".  Please do a full conversion when getting rid of
> > this?  That includes requiring TARGET_POWERPC64 for it (not -m64 as its
> > documentation says; the existing implementation of "we" is correct).
> 
> That is more complex, and likely it should be a GCC 13 thing.

Yes.

> Off the top of
> my head, we would need a new "isa" variant (p9v64) that combines p9v and
> 64-bit.

Not at all no.  Things that *use* the "isa" attribute can use other
attributes as well, if they want.  The reason we have "p9v" is because
it is so common that a shorthand helps (and *all* p9 vector insns need
either it or separate stuff).

> Originally, I had changed the "we" to "wa", but then I realized it
> wouldn't work for 32-bit, but I left in setting the alternative.

Yeah, when I got rid of many of the w* things I left mostly the harder
ones for later.  Sorry!


Segher