[PATCH] rs6000: Fix up easy_vector_constant_msb handling [PR101384]

[PATCH] rs6000: Fix up easy_vector_constant_msb handling [PR101384]

[Jakub Jelinek]

Hi!

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.

--- gcc/config/rs6000/rs6000-protos.h.jj	2021-07-13 09:07:03.697092286 +0200
+++ gcc/config/rs6000/rs6000-protos.h	2021-07-13 11:28:54.876243593 +0200
@@ -30,7 +30,7 @@ extern void init_cumulative_args (CUMULA
 				  tree, machine_mode);
 #endif /* TREE_CODE */
 
-extern bool easy_altivec_constant (rtx, machine_mode);
+extern int easy_altivec_constant (rtx, machine_mode);
 extern bool xxspltib_constant_p (rtx, machine_mode, int *, int *);
 extern int vspltis_shifted (rtx);
 extern HOST_WIDE_INT const_vector_elt_as_int (rtx, unsigned int);
--- gcc/config/rs6000/rs6000.c.jj	2021-07-13 09:07:03.715092036 +0200
+++ gcc/config/rs6000/rs6000.c	2021-07-13 12:18:08.715706507 +0200
@@ -6134,6 +6134,27 @@ vspltis_constant (rtx op, unsigned step,
   splat_val = val;
   msb_val = val >= 0 ? 0 : -1;
 
+  if (val == 0 && step > 1)
+    {
+      /* Special case for loading most significant bit with step > 1.
+	 In that case, match 0s in all but step-1s elements, where match
+	 EASY_VECTOR_MSB.  */
+      for (i = 1; i < nunits; ++i)
+	{
+	  unsigned elt = BYTES_BIG_ENDIAN ? nunits - 1 - i : i;
+	  HOST_WIDE_INT elt_val = const_vector_elt_as_int (op, elt);
+	  if ((i & (step - 1)) == step - 1)
+	    {
+	      if (!EASY_VECTOR_MSB (elt_val, inner))
+		break;
+	    }
+	  else if (elt_val)
+	    break;
+	}
+      if (i == nunits)
+	return true;
+    }
+
   /* Construct the value to be splatted, if possible.  If not, return 0.  */
   for (i = 2; i <= copies; i *= 2)
     {
@@ -6146,6 +6167,7 @@ vspltis_constant (rtx op, unsigned step,
           | (small_val & mask)))
 	return false;
       splat_val = small_val;
+      inner = smallest_int_mode_for_size (bitsize);
     }
 
   /* Check if SPLAT_VAL can really be the operand of a vspltis[bhw].  */
@@ -6160,8 +6182,9 @@ 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))
+     loading up -1 and shifting the value left by -1.  Only do this for
+     step 1 here, for larger steps it is done earlier.  */
+  else if (EASY_VECTOR_MSB (splat_val, inner) && step == 1)
     ;
 
   else
@@ -6271,15 +6294,15 @@ vspltis_shifted (rtx op)
 	}
     }
 
-  /* If all elements are equal, we don't need to do VLSDOI.  */
+  /* If all elements are equal, we don't need to do VSLDOI.  */
   return 0;
 }
 
 
-/* Return true if OP is of the given MODE and can be synthesized
-   with a vspltisb, vspltish or vspltisw.  */
+/* Return non-zero (element mode byte size) if OP is of the given MODE
+   and can be synthesized with a vspltisb, vspltish or vspltisw.  */
 
-bool
+int
 easy_altivec_constant (rtx op, machine_mode mode)
 {
   unsigned step, copies;
@@ -6287,39 +6310,39 @@ easy_altivec_constant (rtx op, machine_m
   if (mode == VOIDmode)
     mode = GET_MODE (op);
   else if (mode != GET_MODE (op))
-    return false;
+    return 0;
 
   /* V2DI/V2DF was added with VSX.  Only allow 0 and all 1's as easy
      constants.  */
   if (mode == V2DFmode)
-    return zero_constant (op, mode);
+    return zero_constant (op, mode) ? 8 : 0;
 
   else if (mode == V2DImode)
     {
       if (!CONST_INT_P (CONST_VECTOR_ELT (op, 0))
 	  || !CONST_INT_P (CONST_VECTOR_ELT (op, 1)))
-	return false;
+	return 0;
 
       if (zero_constant (op, mode))
-	return true;
+	return 8;
 
       if (INTVAL (CONST_VECTOR_ELT (op, 0)) == -1
 	  && INTVAL (CONST_VECTOR_ELT (op, 1)) == -1)
-	return true;
+	return 8;
 
-      return false;
+      return 0;
     }
 
   /* V1TImode is a special container for TImode.  Ignore for now.  */
   else if (mode == V1TImode)
-    return false;
+    return 0;
 
   /* Start with a vspltisw.  */
   step = GET_MODE_NUNITS (mode) / 4;
   copies = 1;
 
   if (vspltis_constant (op, step, copies))
-    return true;
+    return 4;
 
   /* Then try with a vspltish.  */
   if (step == 1)
@@ -6328,7 +6351,7 @@ easy_altivec_constant (rtx op, machine_m
     step >>= 1;
 
   if (vspltis_constant (op, step, copies))
-    return true;
+    return 2;
 
   /* And finally a vspltisb.  */
   if (step == 1)
@@ -6337,12 +6360,12 @@ easy_altivec_constant (rtx op, machine_m
     step >>= 1;
 
   if (vspltis_constant (op, step, copies))
-    return true;
+    return 1;
 
   if (vspltis_shifted (op) != 0)
-    return true;
+    return GET_MODE_SIZE (GET_MODE_INNER (mode));
 
-  return false;
+  return 0;
 }
 
 /* Generate a VEC_DUPLICATE representing a vspltis[bhw] instruction whose
--- gcc/config/rs6000/predicates.md.jj	2021-05-31 10:11:15.138979068 +0200
+++ gcc/config/rs6000/predicates.md	2021-07-13 12:45:18.480295174 +0200
@@ -683,15 +683,26 @@ (define_predicate "easy_vector_constant_
 (define_predicate "easy_vector_constant_msb"
   (and (match_code "const_vector")
        (and (match_test "TARGET_ALTIVEC")
-	    (match_test "easy_altivec_constant (op, mode)")))
+	    (match_test "easy_altivec_constant (op, mode)")
+	    (match_test "vspltis_shifted (op) == 0")))
 {
   HOST_WIDE_INT val;
-  int elt;
+  int elt, sz = easy_altivec_constant (op, mode);
+  machine_mode inner = GET_MODE_INNER (mode);
+  int isz = GET_MODE_SIZE (inner);
   if (mode == V2DImode || mode == V2DFmode)
     return 0;
   elt = BYTES_BIG_ENDIAN ? GET_MODE_NUNITS (mode) - 1 : 0;
+  if (isz < sz)
+    {
+      if (const_vector_elt_as_int (op, elt) != 0)
+	return 0;
+      elt += (BYTES_BIG_ENDIAN ? -1 : 1) * (sz - isz) / isz;
+    }
+  else if (isz > sz)
+    inner = smallest_int_mode_for_size (sz * BITS_PER_UNIT);
   val = const_vector_elt_as_int (op, elt);
-  return EASY_VECTOR_MSB (val, GET_MODE_INNER (mode));
+  return EASY_VECTOR_MSB (val, inner);
 })
 
 ;; Return true if this is an easy altivec constant that we form
--- gcc/config/rs6000/altivec.md.jj	2021-07-13 09:07:03.697092286 +0200
+++ gcc/config/rs6000/altivec.md	2021-07-13 12:54:25.619804641 +0200
@@ -317,22 +317,26 @@ (define_split
   [(const_int 0)]
 {
   rtx dest = operands[0];
-  machine_mode mode = GET_MODE (operands[0]);
+  machine_mode mode;
   rtvec v;
   int i, num_elements;
 
-  if (mode == V4SFmode)
+  switch (easy_altivec_constant (operands[1], <MODE>mode))
     {
-      mode = V4SImode;
-      dest = gen_lowpart (V4SImode, dest);
+    case 1: mode = V16QImode; break;
+    case 2: mode = V8HImode; break;
+    case 4: mode = V4SImode; break;
+    default: gcc_unreachable ();
     }
+  if (mode != <MODE>mode)
+    dest = gen_lowpart (mode, dest);
 
   num_elements = GET_MODE_NUNITS (mode);
   v = rtvec_alloc (num_elements);
   for (i = 0; i < num_elements; i++)
     RTVEC_ELT (v, i) = constm1_rtx;
 
-  emit_insn (gen_vec_initv4sisi (dest, gen_rtx_PARALLEL (mode, v)));
+  rs6000_expand_vector_init (dest, gen_rtx_PARALLEL (mode, v));
   emit_insn (gen_rtx_SET (dest, gen_rtx_ASHIFT (mode, dest, dest)));
   DONE;
 })
--- gcc/testsuite/gcc.dg/pr101384.c.jj	2021-07-13 13:45:42.971992584 +0200
+++ gcc/testsuite/gcc.dg/pr101384.c	2021-07-13 13:45:32.427135184 +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;
+}
--- gcc/testsuite/gcc.target/powerpc/pr101384-1.c.jj	2021-07-13 14:02:57.003030314 +0200
+++ gcc/testsuite/gcc.target/powerpc/pr101384-1.c	2021-07-13 14:02:40.868247714 +0200
@@ -0,0 +1,79 @@
+/* PR target/101384 */
+/* { dg-do compile { target le } } */
+/* { dg-options "-O2 -maltivec" } */
+/* { dg-require-effective-target powerpc_altivec_ok } */
+/* { dg-final { scan-assembler-times {\mvspltis[whb] [^\n\r]*,-1\M} 9 } } */
+/* { dg-final { scan-assembler-times {\mvslw\M} 3 } } */
+/* { dg-final { scan-assembler-times {\mvslh\M} 3 } } */
+/* { dg-final { scan-assembler-times {\mvslb\M} 3 } } */
+
+typedef unsigned char __attribute__((__vector_size__ (16))) U;
+typedef unsigned short __attribute__((__vector_size__ (16))) V;
+typedef unsigned int __attribute__((__vector_size__ (16))) W;
+
+U u;
+V v;
+W w;
+
+U
+f1 (void)
+{
+  U y = (U) { 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80 } + u;
+  return y;
+}
+
+U
+f2 (void)
+{
+  U y = (U) { 0, 0x80, 0, 0x80, 0, 0x80, 0, 0x80, 0, 0x80, 0, 0x80, 0, 0x80, 0, 0x80 } + u;
+  return y;
+}
+
+U
+f3 (void)
+{
+  U y = (U) { 0, 0, 0, 0x80, 0, 0, 0, 0x80, 0, 0, 0, 0x80, 0, 0, 0, 0x80 } + u;
+  return y;
+}
+
+V
+f4 (void)
+{
+  V y = (V) { 0x8080, 0x8080, 0x8080, 0x8080, 0x8080, 0x8080, 0x8080, 0x8080 } + v;
+  return y;
+}
+
+V
+f5 (void)
+{
+  V y = (V) { 0x8000, 0x8000, 0x8000, 0x8000, 0x8000, 0x8000, 0x8000, 0x8000 } + v;
+  return y;
+}
+
+V
+f6 (void)
+{
+  V y = (V) { 0, 0x8000, 0, 0x8000, 0, 0x8000, 0, 0x8000 } + v;
+  return y;
+}
+
+W
+f7 (void)
+{
+  W y = (W) { 0x80808080, 0x80808080, 0x80808080, 0x80808080 } + w;
+  return y;
+}
+
+W
+f8 (void)
+{
+  W y = (W) { 0x80008000, 0x80008000, 0x80008000, 0x80008000 } + w;
+  return y;
+}
+
+W
+f9 (void)
+{
+  W y = (W) { 0x80000000, 0x80000000, 0x80000000, 0x80000000 } + w;
+  return y;
+}
--- gcc/testsuite/gcc.target/powerpc/pr101384-2.c.jj	2021-07-13 14:03:04.282932227 +0200
+++ gcc/testsuite/gcc.target/powerpc/pr101384-2.c	2021-07-13 14:04:03.463134848 +0200
@@ -0,0 +1,79 @@
+/* PR target/101384 */
+/* { dg-do compile { target be } } */
+/* { dg-options "-O2 -maltivec" } */
+/* { dg-require-effective-target powerpc_altivec_ok } */
+/* { dg-final { scan-assembler-times {\mvspltis[whb] [^\n\r]*,-1\M} 9 } } */
+/* { dg-final { scan-assembler-times {\mvslw\M} 3 } } */
+/* { dg-final { scan-assembler-times {\mvslh\M} 3 } } */
+/* { dg-final { scan-assembler-times {\mvslb\M} 3 } } */
+
+typedef unsigned char __attribute__((__vector_size__ (16))) U;
+typedef unsigned short __attribute__((__vector_size__ (16))) V;
+typedef unsigned int __attribute__((__vector_size__ (16))) W;
+
+U u;
+V v;
+W w;
+
+U
+f1 (void)
+{
+  U y = (U) { 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80 } + u;
+  return y;
+}
+
+U
+f2 (void)
+{
+  U y = (U) { 0x80, 0, 0x80, 0, 0x80, 0, 0x80, 0, 0x80, 0, 0x80, 0, 0x80, 0, 0x80, 0 } + u;
+  return y;
+}
+
+U
+f3 (void)
+{
+  U y = (U) { 0x80, 0, 0, 0, 0x80, 0, 0, 0, 0x80, 0, 0, 0, 0x80, 0, 0, 0 } + u;
+  return y;
+}
+
+V
+f4 (void)
+{
+  V y = (V) { 0x8080, 0x8080, 0x8080, 0x8080, 0x8080, 0x8080, 0x8080, 0x8080 } + v;
+  return y;
+}
+
+V
+f5 (void)
+{
+  V y = (V) { 0x8000, 0x8000, 0x8000, 0x8000, 0x8000, 0x8000, 0x8000, 0x8000 } + v;
+  return y;
+}
+
+V
+f6 (void)
+{
+  V y = (V) { 0x8000, 0, 0x8000, 0, 0x8000, 0, 0x8000, 0 } + v;
+  return y;
+}
+
+W
+f7 (void)
+{
+  W y = (W) { 0x80808080, 0x80808080, 0x80808080, 0x80808080 } + w;
+  return y;
+}
+
+W
+f8 (void)
+{
+  W y = (W) { 0x80008000, 0x80008000, 0x80008000, 0x80008000 } + w;
+  return y;
+}
+
+W
+f9 (void)
+{
+  W y = (W) { 0x80000000, 0x80000000, 0x80000000, 0x80000000 } + w;
+  return y;
+}

	Jakub

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

[Jakub Jelinek]

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.

--- 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;
+}


	Jakub

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

[Segher Boessenkool]

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

Re: [PATCH] rs6000: Fix up easy_vector_constant_msb handling [PR101384]

[Segher Boessenkool]

Hi!

On Tue, Jul 13, 2021 at 09:30:43PM +0200, Jakub Jelinek wrote:
> 	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.

> -  if (mode == V4SFmode)
> +  switch (easy_altivec_constant (operands[1], <MODE>mode))
>      {
> -      mode = V4SImode;
> -      dest = gen_lowpart (V4SImode, dest);
> +    case 1: mode = V16QImode; break;
> +    case 2: mode = V8HImode; break;
> +    case 4: mode = V4SImode; break;
> +    default: gcc_unreachable ();
>      }

    case 1:
      mode = V16QImode;
      break;
etc. (and/or make a convenience function for it).

> --- gcc/testsuite/gcc.dg/pr101384.c.jj	2021-07-13 13:45:42.971992584 +0200
> +++ gcc/testsuite/gcc.dg/pr101384.c	2021-07-13 13:45:32.427135184 +0200
> @@ -0,0 +1,39 @@
> +/* PR target/101384 */
> +/* { dg-do run } */
> +/* { dg-options "-O2 -Wno-psabi -w" } */

If you have -w anyway, do you / why do you still need -Wno-psabi?

> --- gcc/testsuite/gcc.target/powerpc/pr101384-1.c.jj	2021-07-13 14:02:57.003030314 +0200
> +++ gcc/testsuite/gcc.target/powerpc/pr101384-1.c	2021-07-13 14:02:40.868247714 +0200
> @@ -0,0 +1,79 @@
> +/* PR target/101384 */
> +/* { dg-do compile { target le } } */
> +/* { dg-options "-O2 -maltivec" } */
> +/* { dg-require-effective-target powerpc_altivec_ok } */
> +/* { dg-final { scan-assembler-times {\mvspltis[whb] [^\n\r]*,-1\M} 9 } } */

If you put (?p) at the front of your regex it gets "partial newline-
sensitive matching", which means that . will not match newlines.  See
https://www.tcl.tk/man/tcl/TclCmd/re_syntax.html#M95 for details.  Here,
you could also use
/* { dg-final { scan-assembler-times {\mvspltis[whb] [^,]*,-1\M} 9 } } */
(but that (more traditional) approach quickly becomes clumsy).

Okay for trunk with or without those improvements (with the switch
formatting fix though).  Thanks!


Segher

Re: [PATCH] rs6000: Fix up easy_vector_constant_msb handling [PR101384]

[Jakub Jelinek]

On Tue, Jul 20, 2021 at 09:48:26AM -0500, Segher Boessenkool wrote:
> > -  if (mode == V4SFmode)
> > +  switch (easy_altivec_constant (operands[1], <MODE>mode))
> >      {
> > -      mode = V4SImode;
> > -      dest = gen_lowpart (V4SImode, dest);
> > +    case 1: mode = V16QImode; break;
> > +    case 2: mode = V8HImode; break;
> > +    case 4: mode = V4SImode; break;
> > +    default: gcc_unreachable ();
> >      }
> 
>     case 1:
>       mode = V16QImode;
>       break;
> etc. (and/or make a convenience function for it).

Ok, will fix.

> > --- gcc/testsuite/gcc.dg/pr101384.c.jj	2021-07-13 13:45:42.971992584 +0200
> > +++ gcc/testsuite/gcc.dg/pr101384.c	2021-07-13 13:45:32.427135184 +0200
> > @@ -0,0 +1,39 @@
> > +/* PR target/101384 */
> > +/* { dg-do run } */
> > +/* { dg-options "-O2 -Wno-psabi -w" } */
> 
> If you have -w anyway, do you / why do you still need -Wno-psabi?

I think not all of the -Wpsabi diagnostics is emitted with warning{,_at}
etc. that -w disables, others are emitted with inform.  The -Wno-psabi
also makes it clear that it is the psabi stuff that is what the testcase
cares about.  Whether -w is also needed or not is something I don't know,
in the past it certainly was needed on various architectures, but maybe it
got fixed and only -Wno-psabi would do the trick?
If so, perhaps we could replace all -Wno-psabi -w occurrences in testsuite
dg-options with just -Wno-psabi and see how far we get.
find testsuite/ -type f | xargs grep -- '-w -Wno-psabi' 2>/dev/null | grep -v ChangeLog | wc -l
49
find testsuite/ -type f | xargs grep -- '-Wno-psabi -w' 2>/dev/null | grep -v ChangeLog | wc -l
24

	Jakub

Re: [PATCH] rs6000: Fix up easy_vector_constant_msb handling [PR101384]

[Segher Boessenkool]

On Tue, Jul 20, 2021 at 05:01:00PM +0200, Jakub Jelinek wrote:
> On Tue, Jul 20, 2021 at 09:48:26AM -0500, Segher Boessenkool wrote:
> > > --- gcc/testsuite/gcc.dg/pr101384.c.jj	2021-07-13 13:45:42.971992584 +0200
> > > +++ gcc/testsuite/gcc.dg/pr101384.c	2021-07-13 13:45:32.427135184 +0200
> > > @@ -0,0 +1,39 @@
> > > +/* PR target/101384 */
> > > +/* { dg-do run } */
> > > +/* { dg-options "-O2 -Wno-psabi -w" } */
> > 
> > If you have -w anyway, do you / why do you still need -Wno-psabi?
> 
> I think not all of the -Wpsabi diagnostics is emitted with warning{,_at}
> etc. that -w disables, others are emitted with inform.

/* An informative note at LOCATION.  Use this for additional details on an error
   message.  */
void
inform (location_t location, const char *gmsgid, ...)

So inform is misused in -Wpsabi?

If using it like this is deemed correct, then inhibit_warnings should
turn it off just like it turns off all *stronger* warnings.  The current
situation doesn't make much sense.

> The -Wno-psabi
> also makes it clear that it is the psabi stuff that is what the testcase
> cares about.  Whether -w is also needed or not is something I don't know,
> in the past it certainly was needed on various architectures, but maybe it
> got fixed and only -Wno-psabi would do the trick?
> If so, perhaps we could replace all -Wno-psabi -w occurrences in testsuite
> dg-options with just -Wno-psabi and see how far we get.
> find testsuite/ -type f | xargs grep -- '-w -Wno-psabi' 2>/dev/null | grep -v ChangeLog | wc -l
> 49
> find testsuite/ -type f | xargs grep -- '-Wno-psabi -w' 2>/dev/null | grep -v ChangeLog | wc -l
> 24

Note we will disable the -Wpsabi vector warnings for rs6000 from GCC 12
on.  It should have been done earlier, but we need a time machine to
install a time machine in the past, etc. :-)


Segher

Re: [PATCH] rs6000: Fix up easy_vector_constant_msb handling [PR101384]

[Jakub Jelinek]

On Tue, Jul 20, 2021 at 10:17:08AM -0500, Segher Boessenkool wrote:
> On Tue, Jul 20, 2021 at 05:01:00PM +0200, Jakub Jelinek wrote:
> > On Tue, Jul 20, 2021 at 09:48:26AM -0500, Segher Boessenkool wrote:
> > > > --- gcc/testsuite/gcc.dg/pr101384.c.jj	2021-07-13 13:45:42.971992584 +0200
> > > > +++ gcc/testsuite/gcc.dg/pr101384.c	2021-07-13 13:45:32.427135184 +0200
> > > > @@ -0,0 +1,39 @@
> > > > +/* PR target/101384 */
> > > > +/* { dg-do run } */
> > > > +/* { dg-options "-O2 -Wno-psabi -w" } */
> > > 
> > > If you have -w anyway, do you / why do you still need -Wno-psabi?
> > 
> > I think not all of the -Wpsabi diagnostics is emitted with warning{,_at}
> > etc. that -w disables, others are emitted with inform.
> 
> /* An informative note at LOCATION.  Use this for additional details on an error
>    message.  */
> void
> inform (location_t location, const char *gmsgid, ...)
> 
> So inform is misused in -Wpsabi?

I bet it is done intentionally not to trigger -Werror, e.g. i386.c uses it
for the notes that some old GCC version had different ABI for certain
passing than the current one.

> If using it like this is deemed correct, then inhibit_warnings should
> turn it off just like it turns off all *stronger* warnings.  The current
> situation doesn't make much sense.
> 
> > The -Wno-psabi
> > also makes it clear that it is the psabi stuff that is what the testcase
> > cares about.  Whether -w is also needed or not is something I don't know,
> > in the past it certainly was needed on various architectures, but maybe it
> > got fixed and only -Wno-psabi would do the trick?
> > If so, perhaps we could replace all -Wno-psabi -w occurrences in testsuite
> > dg-options with just -Wno-psabi and see how far we get.
> > find testsuite/ -type f | xargs grep -- '-w -Wno-psabi' 2>/dev/null | grep -v ChangeLog | wc -l
> > 49
> > find testsuite/ -type f | xargs grep -- '-Wno-psabi -w' 2>/dev/null | grep -v ChangeLog | wc -l
> > 24
> 
> Note we will disable the -Wpsabi vector warnings for rs6000 from GCC 12
> on.  It should have been done earlier, but we need a time machine to
> install a time machine in the past, etc. :-)

I could understand dropping -Wpsabi warnings of the kind that some very old
GCC version had different ABI if sufficient number of releases passed since
then, but at least x86 also has -Wpsabi warnings that returning a certain
vector or taking certain vector as parameter has different ABI without
some particular ISA option.  And those options are valid all the time and
something people should be aware, e.g. returning 16-byte vector without
-msse, or 32-byte vector without -mavx, or 64-byte vector without -mavx512f
- without those ISA switches they are passed/returned as generic vectors,
while with that option in vector registers.

	Jakub

Re: [PATCH] rs6000: Fix up easy_vector_constant_msb handling [PR101384]

[Segher Boessenkool]

On Tue, Jul 20, 2021 at 05:24:57PM +0200, Jakub Jelinek wrote:
> On Tue, Jul 20, 2021 at 10:17:08AM -0500, Segher Boessenkool wrote:
> > > I think not all of the -Wpsabi diagnostics is emitted with warning{,_at}
> > > etc. that -w disables, others are emitted with inform.
> > 
> > /* An informative note at LOCATION.  Use this for additional details on an error
> >    message.  */
> > void
> > inform (location_t location, const char *gmsgid, ...)
> > 
> > So inform is misused in -Wpsabi?
> 
> I bet it is done intentionally not to trigger -Werror, e.g. i386.c uses it
> for the notes that some old GCC version had different ABI for certain
> passing than the current one.

The huge workaround that is -Werror needs more workarounds, how great :-(

> > If using it like this is deemed correct, then inhibit_warnings should
> > turn it off just like it turns off all *stronger* warnings.  The current
> > situation doesn't make much sense.
> > 
> > > The -Wno-psabi
> > > also makes it clear that it is the psabi stuff that is what the testcase
> > > cares about.  Whether -w is also needed or not is something I don't know,
> > > in the past it certainly was needed on various architectures, but maybe it
> > > got fixed and only -Wno-psabi would do the trick?
> > > If so, perhaps we could replace all -Wno-psabi -w occurrences in testsuite
> > > dg-options with just -Wno-psabi and see how far we get.
> > > find testsuite/ -type f | xargs grep -- '-w -Wno-psabi' 2>/dev/null | grep -v ChangeLog | wc -l
> > > 49
> > > find testsuite/ -type f | xargs grep -- '-Wno-psabi -w' 2>/dev/null | grep -v ChangeLog | wc -l
> > > 24
> > 
> > Note we will disable the -Wpsabi vector warnings for rs6000 from GCC 12
> > on.  It should have been done earlier, but we need a time machine to
> > install a time machine in the past, etc. :-)
> 
> I could understand dropping -Wpsabi warnings of the kind that some very old
> GCC version had different ABI if sufficient number of releases passed since
> then,

That is exactly what we will do, yes.

> but at least x86 also has -Wpsabi warnings that returning a certain
> vector or taking certain vector as parameter has different ABI without
> some particular ISA option.  And those options are valid all the time and
> something people should be aware, e.g. returning 16-byte vector without
> -msse, or 32-byte vector without -mavx, or 64-byte vector without -mavx512f
> - without those ISA switches they are passed/returned as generic vectors,
> while with that option in vector registers.

Yup, understood.  I'm just removing the warnings from rs6000 that are
past their usefulness (and are super annoying in practice).  I brought
it up here because that will remove the need for -Wpsabi in the
testsuite in many cases.  Maybe what I call "many" is heavily influenced
by what I run most though :-)


Segher