[PATCH, v2] rs6000: Vector shift-right should honor modulo semantics

[PATCH, v2] rs6000: Vector shift-right should honor modulo semantics

[Bill Schmidt]

Hi!

We had a problem report for code attempting to implement a vector right-shift for a
vector long long (V2DImode) type.  The programmer noted that we don't have a vector
splat-immediate for this mode, but cleverly realized he could use a vector char
splat-immediate since only the lower 6 bits of the shift count are read.  This is a
documented feature of both the vector shift built-ins and the underlying instruction.

Starting with GCC 8, the vector shifts are expanded early in rs6000_gimple_fold_builtin.
However, the GIMPLE folding does not currently perform the required TRUNC_MOD_EXPR to
implement the built-in semantics.  It appears that this was caught earlier for vector
shift-left and fixed, but the same problem was not fixed for vector shift-right.
This patch fixes that.

I've added executable tests for both shift-right algebraic and shift-right logical.
Both fail prior to applying the patch, and work correctly afterwards.

Minor differences from v1 of this patch:
 * Deleted code to defer some vector splats to expand, which was unnecessary.
 * Removed powerpc64*-*-* target selector, added -mvsx option, removed extra
   braces from dg-options directive.
 * Added __attribute__ ((noinline)) to test_s*di_4 functions.
 * Corrected typoed function names.
 * Changed test case names.
 * Added vec-sld-modulo.c as requested (works both before and after this patch).

Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no regressions.  Is
this okay for trunk, and for GCC 8.3 if there is no fallout by the end of the
week?

Thanks,
Bill


[gcc]

2019-02-11  Bill Schmidt  <wschmidt@linux.ibm.com>

	* config/rs6000/rs6000.c (rs6000_gimple_fold_builtin): Shift-right
	and shift-left vector built-ins need to include a TRUNC_MOD_EXPR
	for correct semantics.

[gcc/testsuite]

2019-02-11  Bill Schmidt  <wschmidt@linux.ibm.com>

	* gcc.target/powerpc/vec-sld-modulo.c: New.
	* gcc.target/powerpc/vec-srad-modulo.c: New.
	* gcc.target/powerpc/vec-srd-modulo.c: New.


Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 268707)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -15735,13 +15735,37 @@ rs6000_gimple_fold_builtin (gimple_stmt_iterator *
     case ALTIVEC_BUILTIN_VSRAH:
     case ALTIVEC_BUILTIN_VSRAW:
     case P8V_BUILTIN_VSRAD:
-      arg0 = gimple_call_arg (stmt, 0);
-      arg1 = gimple_call_arg (stmt, 1);
-      lhs = gimple_call_lhs (stmt);
-      g = gimple_build_assign (lhs, RSHIFT_EXPR, arg0, arg1);
-      gimple_set_location (g, gimple_location (stmt));
-      gsi_replace (gsi, g, true);
-      return true;
+      {
+	arg0 = gimple_call_arg (stmt, 0);
+	arg1 = gimple_call_arg (stmt, 1);
+	lhs = gimple_call_lhs (stmt);
+	tree arg1_type = TREE_TYPE (arg1);
+	tree unsigned_arg1_type = unsigned_type_for (TREE_TYPE (arg1));
+	tree unsigned_element_type = unsigned_type_for (TREE_TYPE (arg1_type));
+	location_t loc = gimple_location (stmt);
+	/* Force arg1 into the range valid matching the arg0 type.  */
+	/* Build a vector consisting of the max valid bit-size values.  */
+	int n_elts = VECTOR_CST_NELTS (arg1);
+	tree element_size = build_int_cst (unsigned_element_type,
+					   128 / n_elts);
+	tree_vector_builder elts (unsigned_arg1_type, n_elts, 1);
+	for (int i = 0; i < n_elts; i++)
+	  elts.safe_push (element_size);
+	tree modulo_tree = elts.build ();
+	/* Modulo the provided shift value against that vector.  */
+	gimple_seq stmts = NULL;
+	tree unsigned_arg1 = gimple_build (&stmts, VIEW_CONVERT_EXPR,
+					   unsigned_arg1_type, arg1);
+	tree new_arg1 = gimple_build (&stmts, loc, TRUNC_MOD_EXPR,
+				      unsigned_arg1_type, unsigned_arg1,
+				      modulo_tree);
+	gsi_insert_seq_before (gsi, stmts, GSI_SAME_STMT);
+	/* And finally, do the shift.  */
+	g = gimple_build_assign (lhs, RSHIFT_EXPR, arg0, new_arg1);
+	gimple_set_location (g, loc);
+	gsi_replace (gsi, g, true);
+	return true;
+      }
    /* Flavors of vector shift left.
       builtin_altivec_vsl{b,h,w} -> vsl{b,h,w}.  */
     case ALTIVEC_BUILTIN_VSLB:
@@ -15795,14 +15819,34 @@ rs6000_gimple_fold_builtin (gimple_stmt_iterator *
 	arg0 = gimple_call_arg (stmt, 0);
 	arg1 = gimple_call_arg (stmt, 1);
 	lhs = gimple_call_lhs (stmt);
+	tree arg1_type = TREE_TYPE (arg1);
+	tree unsigned_arg1_type = unsigned_type_for (TREE_TYPE (arg1));
+	tree unsigned_element_type = unsigned_type_for (TREE_TYPE (arg1_type));
+	location_t loc = gimple_location (stmt);
 	gimple_seq stmts = NULL;
 	/* Convert arg0 to unsigned.  */
 	tree arg0_unsigned
 	  = gimple_build (&stmts, VIEW_CONVERT_EXPR,
 			  unsigned_type_for (TREE_TYPE (arg0)), arg0);
+	/* Force arg1 into the range valid matching the arg0 type.  */
+	/* Build a vector consisting of the max valid bit-size values.  */
+	int n_elts = VECTOR_CST_NELTS (arg1);
+	tree element_size = build_int_cst (unsigned_element_type,
+					   128 / n_elts);
+	tree_vector_builder elts (unsigned_arg1_type, n_elts, 1);
+	for (int i = 0; i < n_elts; i++)
+	  elts.safe_push (element_size);
+	tree modulo_tree = elts.build ();
+	/* Modulo the provided shift value against that vector.  */
+	tree unsigned_arg1 = gimple_build (&stmts, VIEW_CONVERT_EXPR,
+					   unsigned_arg1_type, arg1);
+	tree new_arg1 = gimple_build (&stmts, loc, TRUNC_MOD_EXPR,
+				      unsigned_arg1_type, unsigned_arg1,
+				      modulo_tree);
+	/* Do the shift.  */
 	tree res
 	  = gimple_build (&stmts, RSHIFT_EXPR,
-			  TREE_TYPE (arg0_unsigned), arg0_unsigned, arg1);
+			  TREE_TYPE (arg0_unsigned), arg0_unsigned, new_arg1);
 	/* Convert result back to the lhs type.  */
 	res = gimple_build (&stmts, VIEW_CONVERT_EXPR, TREE_TYPE (lhs), res);
 	gsi_insert_seq_before (gsi, stmts, GSI_SAME_STMT);
Index: gcc/testsuite/gcc.target/powerpc/vec-sld-modulo.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/vec-sld-modulo.c	(nonexistent)
+++ gcc/testsuite/gcc.target/powerpc/vec-sld-modulo.c	(working copy)
@@ -0,0 +1,42 @@
+/* Test that using a character splat to set up a shift-left
+   for a doubleword vector works correctly after gimple folding.  */
+
+/* { dg-do run { target { vsx_hw } } } */
+/* { dg-options "-O2 -mvsx" } */
+
+#include <altivec.h>
+
+typedef __vector unsigned long long vui64_t;
+
+static inline vui64_t
+vec_sldi (vui64_t vra, const unsigned int shb)
+{
+  vui64_t lshift;
+  vui64_t result;
+
+  /* Note legitimate use of wrong-type splat due to expectation that only
+     lower 6-bits are read.  */
+  lshift = (vui64_t) vec_splat_s8((const unsigned char)shb);
+
+  /* Vector Shift Left Doublewords based on the lower 6-bits
+     of corresponding element of lshift.  */
+  result = vec_vsld (vra, lshift);
+
+  return (vui64_t) result;
+}
+
+__attribute__ ((noinline)) vui64_t
+test_sldi_4 (vui64_t a)
+{
+  return vec_sldi (a, 4);
+}
+
+int
+main ()
+{
+  vui64_t x = {-256, 1025};
+  x = test_sldi_4 (x);
+  if (x[0] != -4096 || x[1] != 16400)
+    __builtin_abort ();
+  return 0;
+}
Index: gcc/testsuite/gcc.target/powerpc/vec-srad-modulo.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/vec-srad-modulo.c	(nonexistent)
+++ gcc/testsuite/gcc.target/powerpc/vec-srad-modulo.c	(working copy)
@@ -0,0 +1,43 @@
+/* Test that using a character splat to set up a shift-right algebraic
+   for a doubleword vector works correctly after gimple folding.  */
+
+/* { dg-do run { target { vsx_hw } } } */
+/* { dg-options "-O2 -mvsx" } */
+
+#include <altivec.h>
+
+typedef __vector unsigned long long vui64_t;
+typedef __vector long long vi64_t;
+
+static inline vi64_t
+vec_sradi (vi64_t vra, const unsigned int shb)
+{
+  vui64_t rshift;
+  vi64_t result;
+
+  /* Note legitimate use of wrong-type splat due to expectation that only
+     lower 6-bits are read.  */
+  rshift = (vui64_t) vec_splat_s8((const unsigned char)shb);
+
+  /* Vector Shift Right Algebraic Doublewords based on the lower 6-bits
+     of corresponding element of rshift.  */
+  result = vec_vsrad (vra, rshift);
+
+  return (vi64_t) result;
+}
+
+__attribute__ ((noinline)) vi64_t
+test_sradi_4 (vi64_t a)
+{
+  return vec_sradi (a, 4);
+}
+
+int
+main ()
+{
+  vi64_t x = {-256, 1025};
+  x = test_sradi_4 (x);
+  if (x[0] != -16 || x[1] != 64)
+    __builtin_abort ();
+  return 0;
+}
Index: gcc/testsuite/gcc.target/powerpc/vec-srd-modulo.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/vec-srd-modulo.c	(nonexistent)
+++ gcc/testsuite/gcc.target/powerpc/vec-srd-modulo.c	(working copy)
@@ -0,0 +1,42 @@
+/* Test that using a character splat to set up a shift-right logical
+   for a doubleword vector works correctly after gimple folding.  */
+
+/* { dg-do run { target { vsx_hw } } } */
+/* { dg-options "-O2 -mvsx" } */
+
+#include <altivec.h>
+
+typedef __vector unsigned long long vui64_t;
+
+static inline vui64_t
+vec_srdi (vui64_t vra, const unsigned int shb)
+{
+  vui64_t rshift;
+  vui64_t result;
+
+  /* Note legitimate use of wrong-type splat due to expectation that only
+     lower 6-bits are read.  */
+  rshift = (vui64_t) vec_splat_s8((const unsigned char)shb);
+
+  /* Vector Shift Right [Logical] Doublewords based on the lower 6-bits
+     of corresponding element of rshift.  */
+  result = vec_vsrd (vra, rshift);
+
+  return (vui64_t) result;
+}
+
+__attribute__ ((noinline)) vui64_t
+test_srdi_4 (vui64_t a)
+{
+  return vec_srdi (a, 4);
+}
+
+int
+main ()
+{
+  vui64_t x = {1992357, 1025};
+  x = test_srdi_4 (x);
+  if (x[0] != 124522 || x[1] != 64)
+    __builtin_abort ();
+  return 0;
+}

Re: [PATCH, v2] rs6000: Vector shift-right should honor modulo semantics

[Segher Boessenkool]

Hi Bill,

On Mon, Feb 11, 2019 at 07:36:11AM -0600, Bill Schmidt wrote:
> 2019-02-11  Bill Schmidt  <wschmidt@linux.ibm.com>
> 
> 	* config/rs6000/rs6000.c (rs6000_gimple_fold_builtin): Shift-right
> 	and shift-left vector built-ins need to include a TRUNC_MOD_EXPR
> 	for correct semantics.
> 
> [gcc/testsuite]
> 
> 2019-02-11  Bill Schmidt  <wschmidt@linux.ibm.com>
> 
> 	* gcc.target/powerpc/vec-sld-modulo.c: New.
> 	* gcc.target/powerpc/vec-srad-modulo.c: New.
> 	* gcc.target/powerpc/vec-srd-modulo.c: New.

This is okay for trunk and backports.  Thanks!

One comment:

> +vec_sldi (vui64_t vra, const unsigned int shb)
> +{
> +  vui64_t lshift;
> +  vui64_t result;
> +
> +  /* Note legitimate use of wrong-type splat due to expectation that only
> +     lower 6-bits are read.  */
> +  lshift = (vui64_t) vec_splat_s8((const unsigned char)shb);
> +
> +  /* Vector Shift Left Doublewords based on the lower 6-bits
> +     of corresponding element of lshift.  */
> +  result = vec_vsld (vra, lshift);
> +
> +  return (vui64_t) result;
> +}

I realise this is a testcase, and in one frame of mind it is good to test
all different styles and bad habits.  But please never use casts that do not
do anything in correct programs: the only thing such casts do is they shut
up warnings in incorrect programs (including the same program after a wrong
change).  </petpeeve>


Segher

Re: [PATCH, v2] rs6000: Vector shift-right should honor modulo semantics

[Bill Schmidt]

On 2/11/19 10:01 AM, Segher Boessenkool wrote:

> Hi Bill,
>
> On Mon, Feb 11, 2019 at 07:36:11AM -0600, Bill Schmidt wrote:
>> 2019-02-11  Bill Schmidt  <wschmidt@linux.ibm.com>
>>
>> 	* config/rs6000/rs6000.c (rs6000_gimple_fold_builtin): Shift-right
>> 	and shift-left vector built-ins need to include a TRUNC_MOD_EXPR
>> 	for correct semantics.
>>
>> [gcc/testsuite]
>>
>> 2019-02-11  Bill Schmidt  <wschmidt@linux.ibm.com>
>>
>> 	* gcc.target/powerpc/vec-sld-modulo.c: New.
>> 	* gcc.target/powerpc/vec-srad-modulo.c: New.
>> 	* gcc.target/powerpc/vec-srd-modulo.c: New.
> This is okay for trunk and backports.  Thanks!
>
> One comment:
>
>> +vec_sldi (vui64_t vra, const unsigned int shb)
>> +{
>> +  vui64_t lshift;
>> +  vui64_t result;
>> +
>> +  /* Note legitimate use of wrong-type splat due to expectation that only
>> +     lower 6-bits are read.  */
>> +  lshift = (vui64_t) vec_splat_s8((const unsigned char)shb);
>> +
>> +  /* Vector Shift Left Doublewords based on the lower 6-bits
>> +     of corresponding element of lshift.  */
>> +  result = vec_vsld (vra, lshift);
>> +
>> +  return (vui64_t) result;
>> +}
> I realise this is a testcase, and in one frame of mind it is good to test
> all different styles and bad habits.  But please never use casts that do not
> do anything in correct programs: the only thing such casts do is they shut
> up warnings in incorrect programs (including the same program after a wrong
> change).  </petpeeve>

Agreed!  Thanks.  I wasn't careful to remove these as I modified the original
test where they were pertinent.  Will fix before committing.

Thanks,
Bill

>
>
> Segher
>