[PATCH] rs6000: Correct function prototypes for vec_replace_unaligned

[PATCH] rs6000: Correct function prototypes for vec_replace_unaligned

[Bill Schmidt]

Hi!

Due to a pasto error in the documentation, vec_replace_unaligned was
implemented with the same function prototypes as vec_replace_elt.  It was
intended that vec_replace_unaligned always specify output vectors as having
type vector unsigned char, to emphasize that elements are potentially
misaligned by this built-in function.  This patch corrects the
misimplementation.

Bootstrapped and tested on powerpc64le-linux-gnu with no regressions.
Is this okay for trunk?  Eventually I would also like to backport it
to GCC 11, after burn-in.

Thanks!
Bill


2022-02-04  Bill Schmidt  <wschmidt@linux.ibm.com>

gcc/
	* config/rs6000/rs6000-builtins.def (VREPLACE_UN_UV2DI): Change
	function prototype.
	(VREPLACE_UN_UV4SI): Likewise.
	(VREPLACE_UN_V2DF): Likewise.
	(VREPLACE_UN_V2DI): Likewise.
	(VREPLACE_UN_V4SF): Likewise.
	(VREPLACE_UN_V4SI): Likewise.
	* config/rs6000/rs6000-overload.def (VEC_REPLACE_UN): Change all
	function prototypes.
	* config/rs6000/vsx.md (vreplace_un_<mode>): Remove define_expand.
	(vreplace_un_<mode>): New define_insn.

gcc/testsuite/
	* gcc.target/powerpc/vec-replace-word-runnable.c: Handle expected
	prototypes for each call to vec_replace_unaligned.
---
 gcc/config/rs6000/rs6000-builtins.def         | 16 ++++++------
 gcc/config/rs6000/rs6000-overload.def         | 12 ++++-----
 gcc/config/rs6000/vsx.md                      | 25 ++++++++-----------
 .../powerpc/vec-replace-word-runnable.c       | 20 ++++++++++-----
 4 files changed, 38 insertions(+), 35 deletions(-)

diff --git a/gcc/config/rs6000/rs6000-builtins.def b/gcc/config/rs6000/rs6000-builtins.def
index 5c988cc1152..846c0bafd45 100644
--- a/gcc/config/rs6000/rs6000-builtins.def
+++ b/gcc/config/rs6000/rs6000-builtins.def
@@ -3387,25 +3387,25 @@
   const vull __builtin_altivec_vpextd (vull, vull);
     VPEXTD vpextd {}
 
-  const vull __builtin_altivec_vreplace_un_uv2di (vull, unsigned long long, \
-                                                  const int<4>);
+  const vuc __builtin_altivec_vreplace_un_uv2di (vull, unsigned long long, \
+                                                 const int<4>);
     VREPLACE_UN_UV2DI vreplace_un_v2di {}
 
-  const vui __builtin_altivec_vreplace_un_uv4si (vui, unsigned int, \
+  const vuc __builtin_altivec_vreplace_un_uv4si (vui, unsigned int, \
                                                  const int<4>);
     VREPLACE_UN_UV4SI vreplace_un_v4si {}
 
-  const vd __builtin_altivec_vreplace_un_v2df (vd, double, const int<4>);
+  const vuc __builtin_altivec_vreplace_un_v2df (vd, double, const int<4>);
     VREPLACE_UN_V2DF vreplace_un_v2df {}
 
-  const vsll __builtin_altivec_vreplace_un_v2di (vsll, signed long long, \
-                                                 const int<4>);
+  const vuc __builtin_altivec_vreplace_un_v2di (vsll, signed long long, \
+                                                const int<4>);
     VREPLACE_UN_V2DI vreplace_un_v2di {}
 
-  const vf __builtin_altivec_vreplace_un_v4sf (vf, float, const int<4>);
+  const vuc __builtin_altivec_vreplace_un_v4sf (vf, float, const int<4>);
     VREPLACE_UN_V4SF vreplace_un_v4sf {}
 
-  const vsi __builtin_altivec_vreplace_un_v4si (vsi, signed int, const int<4>);
+  const vuc __builtin_altivec_vreplace_un_v4si (vsi, signed int, const int<4>);
     VREPLACE_UN_V4SI vreplace_un_v4si {}
 
   const vull __builtin_altivec_vreplace_uv2di (vull, unsigned long long, \
diff --git a/gcc/config/rs6000/rs6000-overload.def b/gcc/config/rs6000/rs6000-overload.def
index 49a6104ddd2..44e2945aaa0 100644
--- a/gcc/config/rs6000/rs6000-overload.def
+++ b/gcc/config/rs6000/rs6000-overload.def
@@ -3059,17 +3059,17 @@
     VREPLACE_ELT_V2DF
 
 [VEC_REPLACE_UN, vec_replace_unaligned, __builtin_vec_replace_un]
-  vui __builtin_vec_replace_un (vui, unsigned int, const int);
+  vuc __builtin_vec_replace_un (vui, unsigned int, const int);
     VREPLACE_UN_UV4SI
-  vsi __builtin_vec_replace_un (vsi, signed int, const int);
+  vuc __builtin_vec_replace_un (vsi, signed int, const int);
     VREPLACE_UN_V4SI
-  vull __builtin_vec_replace_un (vull, unsigned long long, const int);
+  vuc __builtin_vec_replace_un (vull, unsigned long long, const int);
     VREPLACE_UN_UV2DI
-  vsll __builtin_vec_replace_un (vsll, signed long long, const int);
+  vuc __builtin_vec_replace_un (vsll, signed long long, const int);
     VREPLACE_UN_V2DI
-  vf __builtin_vec_replace_un (vf, float, const int);
+  vuc __builtin_vec_replace_un (vf, float, const int);
     VREPLACE_UN_V4SF
-  vd __builtin_vec_replace_un (vd, double, const int);
+  vuc __builtin_vec_replace_un (vd, double, const int);
     VREPLACE_UN_V2DF
 
 [VEC_REVB, vec_revb, __builtin_vec_revb]
diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md
index 2f5a2f7828d..b53de103872 100644
--- a/gcc/config/rs6000/vsx.md
+++ b/gcc/config/rs6000/vsx.md
@@ -4197,21 +4197,6 @@ (define_expand "vreplace_elt_<mode>"
  }
 [(set_attr "type" "vecsimple")])
 
-(define_expand "vreplace_un_<mode>"
- [(set (match_operand:REPLACE_ELT 0 "register_operand")
- (unspec:REPLACE_ELT [(match_operand:REPLACE_ELT 1 "register_operand")
-		      (match_operand:<VS_scalar> 2 "register_operand")
-		      (match_operand:QI 3 "const_0_to_12_operand")]
-		     UNSPEC_REPLACE_UN))]
- "TARGET_POWER10"
-{
-   /* Immediate value is the byte index Big Endian numbering.  */
-   emit_insn (gen_vreplace_elt_<mode>_inst (operands[0], operands[1],
-					    operands[2], operands[3]));
-   DONE;
- }
-[(set_attr "type" "vecsimple")])
-
 (define_insn "vreplace_elt_<mode>_inst"
  [(set (match_operand:REPLACE_ELT 0 "register_operand" "=v")
   (unspec:REPLACE_ELT [(match_operand:REPLACE_ELT 1 "register_operand" "0")
@@ -4222,6 +4207,16 @@ (define_insn "vreplace_elt_<mode>_inst"
  "vins<REPLACE_ELT_char> %0,%2,%3"
  [(set_attr "type" "vecsimple")])
 
+(define_insn "vreplace_un_<mode>"
+ [(set (match_operand:V16QI 0 "register_operand" "=v")
+  (unspec:V16QI [(match_operand:REPLACE_ELT 1 "register_operand" "0")
+                 (match_operand:<VS_scalar> 2 "register_operand" "r")
+		 (match_operand:QI 3 "const_0_to_12_operand" "n")]
+		UNSPEC_REPLACE_UN))]
+ "TARGET_POWER10"
+ "vins<REPLACE_ELT_char> %0,%2,%3"
+ [(set_attr "type" "vecsimple")])
+
 ;; VSX_EXTRACT optimizations
 ;; Optimize double d = (double) vec_extract (vi, <n>)
 ;; Get the element into the top position and use XVCVSWDP/XVCVUWDP
diff --git a/gcc/testsuite/gcc.target/powerpc/vec-replace-word-runnable.c b/gcc/testsuite/gcc.target/powerpc/vec-replace-word-runnable.c
index 9497cbf2ab0..27318822871 100644
--- a/gcc/testsuite/gcc.target/powerpc/vec-replace-word-runnable.c
+++ b/gcc/testsuite/gcc.target/powerpc/vec-replace-word-runnable.c
@@ -54,6 +54,8 @@ main (int argc, char *argv [])
   vector double src_va_double;
   double src_a_double;
 
+  vector unsigned char vresult_uchar;
+
   /* Vector replace 32-bit element */
   src_a_uint = 345;
   src_va_uint = (vector unsigned int) { 0, 1, 2, 3 };
@@ -172,7 +174,8 @@ main (int argc, char *argv [])
   /* Byte index 7 will overwrite part of elements 2 and 3 */
   expected_vresult_uint = (vector unsigned int) { 1, 2, 345*256, 0 };
 						 
-  vresult_uint = vec_replace_unaligned (src_va_uint, src_a_uint, 3);
+  vresult_uchar = vec_replace_unaligned (src_va_uint, src_a_uint, 3);
+  vresult_uint = (vector unsigned int) vresult_uchar;
 
   if (!vec_all_eq (vresult_uint,  expected_vresult_uint)) {
 #if DEBUG
@@ -191,7 +194,8 @@ main (int argc, char *argv [])
   /* Byte index 7 will over write part of elements 1 and 2 */
   expected_vresult_int = (vector int) { 1, 234*256, 0, 4 };
 						 
-  vresult_int = vec_replace_unaligned (src_va_int, src_a_int, 7);
+  vresult_uchar = vec_replace_unaligned (src_va_int, src_a_int, 7);
+  vresult_int = (vector signed int) vresult_uchar;
 
   if (!vec_all_eq (vresult_int,  expected_vresult_int)) {
 #if DEBUG
@@ -209,7 +213,8 @@ main (int argc, char *argv [])
   vresult_float = (vector float) { 0.0, 0.0, 0.0, 0.0 };
   expected_vresult_float = (vector float) { 0.0, 34.0, 20.0, 30.0 };
 						 
-  vresult_float = vec_replace_unaligned (src_va_float, src_a_float, 8);
+  vresult_uchar = vec_replace_unaligned (src_va_float, src_a_float, 8);
+  vresult_float = (vector float) vresult_uchar;
 
   if (!vec_all_eq (vresult_float,  expected_vresult_float)) {
 #if DEBUG
@@ -231,7 +236,8 @@ main (int argc, char *argv [])
 							      0x200 };
 						 
   /* Byte index 7 will over write least significant byte of  element 0  */
-  vresult_ullint = vec_replace_unaligned (src_va_ullint, src_a_ullint, 7);
+  vresult_uchar = vec_replace_unaligned (src_va_ullint, src_a_ullint, 7);
+  vresult_ullint = (vector unsigned long long) vresult_uchar;
 
   if (!vec_all_eq (vresult_ullint,  expected_vresult_ullint)) {
 #if DEBUG
@@ -251,7 +257,8 @@ main (int argc, char *argv [])
   /* Byte index 7 will over write least significant byte of  element 0  */
   expected_vresult_llint = (vector long long int) { 678*256, 0x100 };
 						 
-  vresult_llint = vec_replace_unaligned (src_va_llint, src_a_llint, 7);
+  vresult_uchar = vec_replace_unaligned (src_va_llint, src_a_llint, 7);
+  vresult_llint = (vector signed long long) vresult_uchar;
 
   if (!vec_all_eq (vresult_llint,  expected_vresult_llint)) {
 #if DEBUG
@@ -270,7 +277,8 @@ main (int argc, char *argv [])
   vresult_double = (vector double) { 0.0, 0.0 };
   expected_vresult_double = (vector double) { 0.0, 678.0 };
 						 
-  vresult_double = vec_replace_unaligned (src_va_double, src_a_double, 0);
+  vresult_uchar = vec_replace_unaligned (src_va_double, src_a_double, 0);
+  vresult_double = (vector double) vresult_uchar;
 
   if (!vec_all_eq (vresult_double,  expected_vresult_double)) {
 #if DEBUG
-- 
2.27.0

Re: [PATCH] rs6000: Correct function prototypes for vec_replace_unaligned

[Segher Boessenkool]

Hi!

On Tue, Feb 08, 2022 at 03:29:48PM -0600, Bill Schmidt wrote:
> Due to a pasto error in the documentation, vec_replace_unaligned was
> implemented with the same function prototypes as vec_replace_elt.  It was
> intended that vec_replace_unaligned always specify output vectors as having
> type vector unsigned char, to emphasize that elements are potentially
> misaligned by this built-in function.

In the documentation that isn't publically available in the first place,
heh.

I don't see how the result type should always be vector unsigned char
here, and not for every other builtin the same.  But whatever the
documentation says, we should do of course.

> This patch corrects the misimplementation.

> gcc/
> 	* config/rs6000/rs6000-builtins.def (VREPLACE_UN_UV2DI): Change
> 	function prototype.
> 	(VREPLACE_UN_UV4SI): Likewise.
> 	(VREPLACE_UN_V2DF): Likewise.
> 	(VREPLACE_UN_V2DI): Likewise.
> 	(VREPLACE_UN_V4SF): Likewise.
> 	(VREPLACE_UN_V4SI): Likewise.
> 	* config/rs6000/rs6000-overload.def (VEC_REPLACE_UN): Change all
> 	function prototypes.
> 	* config/rs6000/vsx.md (vreplace_un_<mode>): Remove define_expand.
> 	(vreplace_un_<mode>): New define_insn.
> 
> gcc/testsuite/
> 	* gcc.target/powerpc/vec-replace-word-runnable.c: Handle expected
> 	prototypes for each call to vec_replace_unaligned.

> +(define_insn "vreplace_un_<mode>"
> + [(set (match_operand:V16QI 0 "register_operand" "=v")
> +  (unspec:V16QI [(match_operand:REPLACE_ELT 1 "register_operand" "0")
> +                 (match_operand:<VS_scalar> 2 "register_operand" "r")
> +		 (match_operand:QI 3 "const_0_to_12_operand" "n")]
> +		UNSPEC_REPLACE_UN))]
> + "TARGET_POWER10"
> + "vins<REPLACE_ELT_char> %0,%2,%3"
> + [(set_attr "type" "vecsimple")])

const_0_to_12 operand is wrong for vinsd.  Not new after your patch, but
still broken.  You could use const_0_to_15_operand together with an insn
condition for example.  It seems all uses of 0_to_12 have similar
problems?  Removing it completely also saves use from having to fix its
documentation (in predicates.md) :-)

The patch is okay for trunk.  Thanks!


Segher