Patch ping (was Re: [PATCH] rs6000: Fix up easy_vector_constant_msb handling [PR101384])

Message ID 20210720124303.GB2380545@tucnak
State New
Headers show
Series
  • Patch ping (was Re: [PATCH] rs6000: Fix up easy_vector_constant_msb handling [PR101384])
Related show

Commit Message

Marek Polacek via Gcc-patches July 20, 2021, 12:43 p.m.
On Tue, Jul 13, 2021 at 09:30:43PM +0200, Jakub Jelinek via Gcc-patches wrote:
> The following gcc.dg/pr101384.c testcase is miscompiled on

> powerpc64le-linux.

> easy_altivec_constant has code to try construct vector constants with

> different element sizes, perhaps different from CONST_VECTOR's mode.  But as

> written, that works fine for vspltis[bhw] cases, but not for the vspltisw

> x,-1; vsl[bhw] x,x,x case, because that creates always a V16QImode, V8HImode

> or V4SImode constant containing broadcasted constant with just the MSB set. 

> The vspltis_constant function etc. expects the vspltis[bhw] instructions

> where the small [-16..15] or even [-32..30] constant is sign-extended to the

> remaining step bytes, but that is not the case for the 0x80...00 constants,

> with step > 1 we can't handle e.g.

> { 0x80, 0xff, 0xff, 0xff, 0x80, 0xff, 0xff, 0xff, 0x80, 0xff, 0xff, 0xff, 0x80, 0xff, 0xff, 0xff }

> vectors but do want to handle e.g.

> { 0, 0, 0, 0x80, 0, 0, 0, 0x80, 0, 0, 0, 0x80, 0, 0, 0, 0x80 }

> and similarly with copies > 1 we do want to handle e.g.

> { 0x80808080, 0x80808080, 0x80808080, 0x80808080 }.

> 

> Bootstrapped/regtested on powerpc64le-linux and powerpc64-linux (the latter

> regtested with -m32/-m64), ok for trunk?

> 

> Perhaps for backports it would be best to limit the EASY_VECTOR_MSB case

> matching to step == 1 && copies == 1, because that is the only case the

> splitter handled correctly, but as can be seen in the gcc.target tests, the

> patch tries to handle it for all the cases.  Do you want that other patch

> or prefer this patch for the backports too?

> 

> 2021-07-13  Jakub Jelinek  <jakub@redhat.com>

> 

> 	PR target/101384

> 	* config/rs6000/rs6000-protos.h (easy_altivec_constant): Change return

> 	type from bool to int.

> 	* config/rs6000/rs6000.c (vspltis_constant): Fix up handling the

> 	EASY_VECTOR_MSB case if either step or copies is not 1.

> 	(vspltis_shifted): Fix comment typo.

> 	(easy_altivec_constant): Change return type from bool to int, instead

> 	of returning true return byte size of the element mode that should be

> 	used to synthetize the constant.

> 	* config/rs6000/predicates.md (easy_vector_constant_msb): Require

> 	that vspltis_shifted is 0, handle the case where easy_altivec_constant

> 	assumes using different vector mode from CONST_VECTOR's mode.

> 	* config/rs6000/altivec.md (easy_vector_constant_msb splitter): Use

> 	easy_altivec_constant to determine mode in which -1 >> -1 should be

> 	performed, use rs6000_expand_vector_init instead of gen_vec_initv4sisi.

> 

> 	* gcc.dg/pr101384.c: New test.

> 	* gcc.target/powerpc/pr101384-1.c: New test.

> 	* gcc.target/powerpc/pr101384-2.c: New test.


I'd like to ping this patch.

For gcc 11, I've bootstrapped/regtested on powerpc64le-linux and
powerpc64-linux (the latter regtested -m32/-m64) also a simpler version
below, which restricts it to the case that the code handles properly.

2021-07-20  Jakub Jelinek  <jakub@redhat.com>

	PR target/101384
	* config/rs6000/rs6000.c (vspltis_constant): Accept EASY_VECTOR_MSB
	only if step and copies are equal to 1.

	* gcc.dg/pr101384.c: New test.



	Jakub

Comments

Segher Boessenkool July 20, 2021, 2:28 p.m. | #1
Hi!

On Tue, Jul 20, 2021 at 02:43:03PM +0200, Jakub Jelinek wrote:
> For gcc 11, I've bootstrapped/regtested on powerpc64le-linux and

> powerpc64-linux (the latter regtested -m32/-m64) also a simpler version

> below, which restricts it to the case that the code handles properly.

> 

> 2021-07-20  Jakub Jelinek  <jakub@redhat.com>

> 

> 	PR target/101384

> 	* config/rs6000/rs6000.c (vspltis_constant): Accept EASY_VECTOR_MSB

> 	only if step and copies are equal to 1.

> 

> 	* gcc.dg/pr101384.c: New test.


Okay for all backports.  Thanks!


Segher

Patch

--- gcc/config/rs6000/rs6000.c.jj	2021-07-18 12:50:43.816219546 +0200
+++ gcc/config/rs6000/rs6000.c	2021-07-20 10:46:23.880632997 +0200
@@ -6144,7 +6144,7 @@  vspltis_constant (rtx op, unsigned step,
 
   /* Also check if are loading up the most significant bit which can be done by
      loading up -1 and shifting the value left by -1.  */
-  else if (EASY_VECTOR_MSB (splat_val, inner))
+  else if (EASY_VECTOR_MSB (splat_val, inner) && step == 1 && copies == 1)
     ;
 
   else
--- gcc/testsuite/gcc.dg/pr101384.c.jj	2021-07-20 10:45:22.828486154 +0200
+++ gcc/testsuite/gcc.dg/pr101384.c	2021-07-20 10:45:22.828486154 +0200
@@ -0,0 +1,39 @@ 
+/* PR target/101384 */
+/* { dg-do run } */
+/* { dg-options "-O2 -Wno-psabi -w" } */
+
+typedef unsigned char __attribute__((__vector_size__ (16))) U;
+typedef unsigned short __attribute__((__vector_size__ (8 * sizeof (short)))) V;
+
+U u;
+V v;
+
+__attribute__((noipa)) U
+foo (void)
+{
+  U y = (U) { 0x80, 0xff, 0xff, 0xff, 0x80, 0xff, 0xff, 0xff,
+              0x80, 0xff, 0xff, 0xff, 0x80, 0xff, 0xff, 0xff } + u;
+  return y;
+}
+
+__attribute__((noipa)) V
+bar (void)
+{
+  V y = (V) { 0x8000, 0xffff, 0x8000, 0xffff,
+              0x8000, 0xffff, 0x8000, 0xffff } + v;
+  return y;
+}
+
+int
+main ()
+{
+  U x = foo ();
+  for (unsigned i = 0; i < 16; i++)
+    if (x[i] != ((i & 3) ? 0xff : 0x80))
+      __builtin_abort ();
+  V y = bar ();
+  for (unsigned i = 0; i < 8; i++)
+    if (y[i] != ((i & 1) ? 0xffff : 0x8000))
+      __builtin_abort ();
+  return 0;
+}