diff mbox series

[ver,2] rs6000, fix vec_replace_unaligned builtin arguments

Message ID 004d44c3139e2af8c70ed57a4cb813a6b1cf3d67.camel@us.ibm.com
State Accepted
Headers
Series [ver,2] rs6000, fix vec_replace_unaligned builtin arguments |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Carl Love June 15, 2023, 4 p.m. UTC 
  GCC maintainers:

Version 2, fixed various typos.  Updated the change log body to say the
instruction counts were updated.  The instruction counts changed as a
result of changing the first argument of the vec_replace_unaligned
builtin call from vector unsigned long long (vull) to vector unsigned
char (vuc).  When the first argument was vull the builtin call
generated the vinsd instruction for the two test cases.  The updated
call with vuc as the first argument generates two vinsw instructions
instead.  Patch was retested on Power 10 with no regressions.

The following patch fixes the first argument in the builtin definition
and the corresponding test cases.  Initially, the builtin specification
was wrong due to a cut and past error.  The documentation was fixed in:

   commit ed3fea09b18f67e757b5768b42cb6e816626f1db
   Author: Bill Schmidt <wschmidt@linux.ibm.com>
   Date:   Fri Feb 4 13:07:17 2022 -0600

       rs6000: Correct function prototypes for vec_replace_unaligned

       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.
    ....

This patch fixes the arguments in the definitions and updates the
testcases accordingly.  Additionally, a few minor spacing issues are
fixed.

The patch has been tested on Power 10 with no regressions.  Please let
me know if the patch is acceptable for mainline.  Thanks.

                 Carl 

--------------------------------------------------
rs6000, fix vec_replace_unaligned builtin arguments

The first argument of the vec_replace_unaligned builtin should always be
unsigned char, as specified in gcc/doc/extend.texi.

This patch fixes the builtin definitions and updates the testcases to use
the correct arguments.  The expected instruction counts for the testcase
are updated.

gcc/ChangeLog:
	* config/rs6000/rs6000-overload.def (__builtin_vec_replace_un):
	Fix first argument type.

gcc/testsuite/ChangeLog:
	* gcc.target/powerpc/ver-replace-word-runnable.c
	(vec_replace_unaligned) Fix first argument type.
	(vresult_uchar): Fix expected results.
	(vec_replace_unaligned): Update for loop to check uchar results.
	Remove extra spaces in if statements.
	Insert missing spaces in for statements.
	(dg-final): Update expected instruction counts.
---
 gcc/config/rs6000/rs6000-overload.def         |  12 +-
 .../powerpc/vec-replace-word-runnable.c       | 157 ++++++++++--------
 2 files changed, 92 insertions(+), 77 deletions(-)

Comments

Kewen.Lin June 19, 2023, 5:41 a.m. UTC | #1 
Hi Carl,

on 2023/6/16 00:00, Carl Love wrote:
> GCC maintainers:
> 
> Version 2, fixed various typos.  Updated the change log body to say the
> instruction counts were updated.  The instruction counts changed as a
> result of changing the first argument of the vec_replace_unaligned
> builtin call from vector unsigned long long (vull) to vector unsigned
> char (vuc).  When the first argument was vull the builtin call
> generated the vinsd instruction for the two test cases.  The updated
> call with vuc as the first argument generates two vinsw instructions
> instead.  Patch was retested on Power 10 with no regressions.
> 
> The following patch fixes the first argument in the builtin definition
> and the corresponding test cases.  Initially, the builtin specification
> was wrong due to a cut and past error.  The documentation was fixed in:
> 
>    commit ed3fea09b18f67e757b5768b42cb6e816626f1db
>    Author: Bill Schmidt <wschmidt@linux.ibm.com>
>    Date:   Fri Feb 4 13:07:17 2022 -0600
> 
>        rs6000: Correct function prototypes for vec_replace_unaligned
> 
>        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.
>     ....
> 
> This patch fixes the arguments in the definitions and updates the
> testcases accordingly.  Additionally, a few minor spacing issues are
> fixed.
> 
> The patch has been tested on Power 10 with no regressions.  Please let
> me know if the patch is acceptable for mainline.  Thanks.
> 
>                  Carl 
> 
> --------------------------------------------------
> rs6000, fix vec_replace_unaligned builtin arguments
> 
> The first argument of the vec_replace_unaligned builtin should always be
> unsigned char, as specified in gcc/doc/extend.texi.
> 
> This patch fixes the builtin definitions and updates the testcases to use
> the correct arguments.  The expected instruction counts for the testcase
> are updated.
> 
> gcc/ChangeLog:
> 	* config/rs6000/rs6000-overload.def (__builtin_vec_replace_un):
> 	Fix first argument type.
> 
> gcc/testsuite/ChangeLog:
> 	* gcc.target/powerpc/ver-replace-word-runnable.c

Wrong case name, s/ver/vec/

Sorry that I didn't catch this during the previous review.
How do you generate changelog?  I guess you don't manually type this
but use something like gcc-{commit}-mklog?  that should get this name
right, it's not perfect though. :)

Excepting for the vinsd/vinsw expected insn counts thing, that was
replied in another thread, the others look good to me.

BR,
Kewen
diff mbox series

Patch

diff --git a/gcc/config/rs6000/rs6000-overload.def b/gcc/config/rs6000/rs6000-overload.def
index c582490c084..26dc662b8fb 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]
-  vuc __builtin_vec_replace_un (vui, unsigned int, const int);
+  vuc __builtin_vec_replace_un (vuc, unsigned int, const int);
     VREPLACE_UN_UV4SI
-  vuc __builtin_vec_replace_un (vsi, signed int, const int);
+  vuc __builtin_vec_replace_un (vuc, signed int, const int);
     VREPLACE_UN_V4SI
-  vuc __builtin_vec_replace_un (vull, unsigned long long, const int);
+  vuc __builtin_vec_replace_un (vuc, unsigned long long, const int);
     VREPLACE_UN_UV2DI
-  vuc __builtin_vec_replace_un (vsll, signed long long, const int);
+  vuc __builtin_vec_replace_un (vuc, signed long long, const int);
     VREPLACE_UN_V2DI
-  vuc __builtin_vec_replace_un (vf, float, const int);
+  vuc __builtin_vec_replace_un (vuc, float, const int);
     VREPLACE_UN_V4SF
-  vuc __builtin_vec_replace_un (vd, double, const int);
+  vuc __builtin_vec_replace_un (vuc, double, const int);
     VREPLACE_UN_V2DF
 
 [VEC_REVB, vec_revb, __builtin_vec_revb]
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 27318822871..66b0ef58996 100644
--- a/gcc/testsuite/gcc.target/powerpc/vec-replace-word-runnable.c
+++ b/gcc/testsuite/gcc.target/powerpc/vec-replace-word-runnable.c
@@ -20,6 +20,9 @@  main (int argc, char *argv [])
   unsigned char ch;
   unsigned int index;
 
+  vector unsigned char src_va_uchar;
+  vector unsigned char expected_vresult_uchar;
+
   vector unsigned int vresult_uint;
   vector unsigned int expected_vresult_uint;
   vector unsigned int src_va_uint;
@@ -64,10 +67,10 @@  main (int argc, char *argv [])
 						 
   vresult_uint = vec_replace_elt (src_va_uint, src_a_uint, 2);
 
-  if (!vec_all_eq (vresult_uint,  expected_vresult_uint)) {
+  if (!vec_all_eq (vresult_uint, expected_vresult_uint)) {
 #if DEBUG
     printf("ERROR, vec_replace_elt (src_vb_uint, src_va_uint, index)\n");
-    for(i = 0; i < 4; i++)
+    for (i = 0; i < 4; i++)
       printf(" vresult_uint[%d] = %d, expected_vresult_uint[%d] = %d\n",
 	     i, vresult_uint[i], i, expected_vresult_uint[i]);
 #else
@@ -82,10 +85,10 @@  main (int argc, char *argv [])
 						 
   vresult_int = vec_replace_elt (src_va_int, src_a_int, 1);
 
-  if (!vec_all_eq (vresult_int,  expected_vresult_int)) {
+  if (!vec_all_eq (vresult_int, expected_vresult_int)) {
 #if DEBUG
     printf("ERROR, vec_replace_elt (src_vb_int, src_va_int, index)\n");
-    for(i = 0; i < 4; i++)
+    for (i = 0; i < 4; i++)
       printf(" vresult_int[%d] = %d, expected_vresult_int[%d] = %d\n",
 	     i, vresult_int[i], i, expected_vresult_int[i]);
 #else
@@ -100,10 +103,10 @@  main (int argc, char *argv [])
 						 
   vresult_float = vec_replace_elt (src_va_float, src_a_float, 1);
 
-  if (!vec_all_eq (vresult_float,  expected_vresult_float)) {
+  if (!vec_all_eq (vresult_float, expected_vresult_float)) {
 #if DEBUG
     printf("ERROR, vec_replace_elt (src_vb_float, src_va_float, index)\n");
-    for(i = 0; i < 4; i++)
+    for (i = 0; i < 4; i++)
       printf(" vresult_float[%d] = %f, expected_vresult_float[%d] = %f\n",
 	     i, vresult_float[i], i, expected_vresult_float[i]);
 #else
@@ -122,7 +125,7 @@  main (int argc, char *argv [])
   if (!vec_all_eq (vresult_ullint,  expected_vresult_ullint)) {
 #if DEBUG
     printf("ERROR, vec_replace_elt (src_vb_ullint, src_va_ullint, index)\n");
-    for(i = 0; i < 2; i++)
+    for (i = 0; i < 2; i++)
       printf(" vresult_ullint[%d] = %d, expected_vresult_ullint[%d] = %d\n",
 	     i, vresult_ullint[i], i, expected_vresult_ullint[i]);
 #else
@@ -137,10 +140,10 @@  main (int argc, char *argv [])
 						 
   vresult_llint = vec_replace_elt (src_va_llint, src_a_llint, 1);
 
-  if (!vec_all_eq (vresult_llint,  expected_vresult_llint)) {
+  if (!vec_all_eq (vresult_llint, expected_vresult_llint)) {
 #if DEBUG
     printf("ERROR, vec_replace_elt (src_vb_llint, src_va_llint, index)\n");
-    for(i = 0; i < 2; i++)
+    for (i = 0; i < 2; i++)
       printf(" vresult_llint[%d] = %d, expected_vresult_llint[%d] = %d\n",
 	     i, vresult_llint[i], i, expected_vresult_llint[i]);
 #else
@@ -155,10 +158,10 @@  main (int argc, char *argv [])
 						 
   vresult_double = vec_replace_elt (src_va_double, src_a_double, 1);
 
-  if (!vec_all_eq (vresult_double,  expected_vresult_double)) {
+  if (!vec_all_eq (vresult_double, expected_vresult_double)) {
 #if DEBUG
     printf("ERROR, vec_replace_elt (src_vb_double, src_va_double, index)\n");
-    for(i = 0; i < 2; i++)
+    for (i = 0; i < 2; i++)
       printf(" vresult_double[%d] = %f, expected_vresult_double[%d] = %f\n",
 	     i, vresult_double[i], i, expected_vresult_double[i]);
 #else
@@ -169,60 +172,68 @@  main (int argc, char *argv [])
 
   /* Vector replace 32-bit element, unaligned */
   src_a_uint = 345;
-  src_va_uint = (vector unsigned int) { 1, 2, 0, 0 };
-  vresult_uint = (vector unsigned int) { 0, 0, 0, 0 };
+  src_va_uchar = (vector unsigned char) { 1, 0, 0, 0, 2, 0, 0, 0,
+					  0, 0, 0, 0, 0, 0, 0, 0 };
+  vresult_uchar = (vector unsigned char) { 0, 0, 0, 0, 0, 0, 0, 0,
+					   0, 0, 0, 0, 0, 0, 0, 0 };
   /* Byte index 7 will overwrite part of elements 2 and 3 */
-  expected_vresult_uint = (vector unsigned int) { 1, 2, 345*256, 0 };
+  expected_vresult_uchar
+    = (vector unsigned char) { 1, 0, 0, 0, 2, 0, 0, 0,
+			       0, 0x59, 0x1, 0, 0, 0, 0, 0 };
 						 
-  vresult_uchar = vec_replace_unaligned (src_va_uint, src_a_uint, 3);
-  vresult_uint = (vector unsigned int) vresult_uchar;
+  vresult_uchar = vec_replace_unaligned (src_va_uchar, src_a_uint, 3);
 
-  if (!vec_all_eq (vresult_uint,  expected_vresult_uint)) {
+  if (!vec_all_eq (vresult_uchar, expected_vresult_uchar)) {
 #if DEBUG
-    printf("ERROR, vec_replace_unaligned (src_vb_uint, src_va_uint, index)\n");
-    for(i = 0; i < 4; i++)
-      printf(" vresult_uint[%d] = %d, expected_vresult_uint[%d] = %d\n",
-	     i, vresult_uint[i], i, expected_vresult_uint[i]);
+    printf("ERROR, vec_replace_unaligned (src_va_uchar, src_va_uint, index)\n");
+    for (i = 0; i < 16; i++)
+      printf(" vresult_uchar[%d] = 0x%x, expected_vresult_uint[%d] = 0%x\n",
+	     i, vresult_uchar[i], i, expected_vresult_uchar[i]);
 #else
     abort();
 #endif
   }
 
   src_a_int = 234;
-  src_va_int = (vector int) { 1, 0, 3, 4 };
-  vresult_int = (vector int) { 0, 0, 0, 0 };
+  src_va_uchar = (vector unsigned char) { 1, 0, 0, 0, 0, 0, 0, 0,
+					  3, 0, 0, 0, 4, 0, 0, 0 };
+  vresult_uchar = (vector unsigned char) { 0, 0, 0, 0, 0, 0, 0, 0,
+					   0, 0, 0, 0, 0, 0, 0, 0 };
   /* Byte index 7 will over write part of elements 1 and 2 */
-  expected_vresult_int = (vector int) { 1, 234*256, 0, 4 };
-						 
-  vresult_uchar = vec_replace_unaligned (src_va_int, src_a_int, 7);
-  vresult_int = (vector signed int) vresult_uchar;
+  expected_vresult_uchar = (vector unsigned char) { 1, 0, 0, 0, 0, 0xea, 0, 0,
+						    0, 0, 0, 0, 4, 0, 0, 0 };
 
-  if (!vec_all_eq (vresult_int,  expected_vresult_int)) {
+  vresult_uchar = vec_replace_unaligned (src_va_uchar, src_a_int, 7);
+
+  if (!vec_all_eq (vresult_uchar, expected_vresult_uchar)) {
 #if DEBUG
-    printf("ERROR, vec_replace_unaligned (src_vb_int, src_va_int, index)\n");
-    for(i = 0; i < 4; i++)
-      printf(" vresult_int[%d] = %d, expected_vresult_int[%d] = %d\n",
-	     i, vresult_int[i], i, expected_vresult_int[i]);
+    printf("ERROR, vec_replace_unaligned (src_va_uchar, src_va_int, index)\n");
+    for (i = 0; i < 16; i++)
+      printf(" vresult_int[%d] = 0x%x, expected_vresult_int[%d] = 0x%x\n",
+	     i, vresult_uchar[i], i, expected_vresult_uchar[i]);
 #else
     abort();
 #endif
   }
 
   src_a_float = 34.0;
-  src_va_float = (vector float) { 0.0, 10.0, 20.0, 30.0 };
-  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 };
+  src_va_uchar = (vector unsigned char) { 0, 0, 0, 0, 0, 0, 0x41, 0xa0,
+					  5, 6, 7, 8, 0x41, 0xf0, 0, 0};
+  vresult_uchar = (vector unsigned char) { 0, 0, 0, 0, 0, 0, 0, 0,
+					   0, 0, 0, 0, 0, 0, 0, 0 };
+  expected_vresult_uchar
+    = (vector unsigned char) { 0, 0, 0, 0, 0, 0, 8, 0x42,
+			       5, 6, 7, 8, 0x41, 0xf0, 0, 0 };
 						 
-  vresult_uchar = vec_replace_unaligned (src_va_float, src_a_float, 8);
-  vresult_float = (vector float) vresult_uchar;
+  vresult_uchar = vec_replace_unaligned (src_va_uchar, src_a_float, 8);
 
-  if (!vec_all_eq (vresult_float,  expected_vresult_float)) {
+  if (!vec_all_eq (vresult_uchar, expected_vresult_uchar)) {
 #if DEBUG
-    printf("ERROR, vec_replace_unaligned (src_vb_float, src_va_float, "
+    printf("ERROR, vec_replace_unaligned (src_va_uchar, src_va_float, "
 	   "index)\n");
-    for(i = 0; i < 4; i++)
-      printf(" vresult_float[%d] = %f, expected_vresult_float[%d] = %f\n",
-	     i, vresult_float[i], i, expected_vresult_float[i]);
+    for (i = 0; i < 16; i++)
+      printf(" vresult_uchar[%d] = 0x%x, expected_vresult_uchar[%d] = 0x%x\n",
+	     i, vresult_uchar[i], i, expected_vresult_uchar[i]);
 #else
     abort();
 #endif
@@ -230,61 +241,67 @@  main (int argc, char *argv [])
 
   /* Vector replace 64-bit element, unaligned  */
   src_a_ullint = 456;
-  src_va_ullint = (vector unsigned long long int) { 0, 0x222 };
-  vresult_ullint = (vector unsigned long long int) { 0, 0 };
-  expected_vresult_ullint = (vector unsigned long long int) { 456*256,
-							      0x200 };
+  src_va_uchar = (vector unsigned char) { 0, 0xc, 0x1, 0, 0, 0, 0, 0,
+					  0x22, 0x2, 0, 0, 0, 0, 0, 0 };
+  vresult_uchar = (vector unsigned char) { 0, 0, 0, 0, 0, 0, 0, 0,
+					   0, 0, 0, 0, 0, 0, 0, 0 };
+  expected_vresult_uchar
+    = (vector unsigned char) { 0, 0xc, 0x1, 0, 0, 0xc8, 1, 0,
+			       0, 2, 0, 0, 0, 0, 0, 0 };
 						 
   /* Byte index 7 will over write least significant byte of  element 0  */
-  vresult_uchar = vec_replace_unaligned (src_va_ullint, src_a_ullint, 7);
-  vresult_ullint = (vector unsigned long long) vresult_uchar;
+  vresult_uchar = vec_replace_unaligned (src_va_uchar, src_a_ullint, 7);
 
-  if (!vec_all_eq (vresult_ullint,  expected_vresult_ullint)) {
+  if (!vec_all_eq (vresult_uchar, expected_vresult_uchar)) {
 #if DEBUG
-    printf("ERROR, vec_replace_unaligned (src_vb_ullint, src_va_ullint, "
+    printf("ERROR, vec_replace_unaligned (src_va_uchar, src_va_ullint, "
 	   "index)\n");
-    for(i = 0; i < 2; i++)
-      printf(" vresult_ullint[%d] = %d, expected_vresult_ullint[%d] = %d\n",
-	     i, vresult_ullint[i], i, expected_vresult_ullint[i]);
+    for (i = 0; i < 15; i++)
+      printf(" vresult_uchar[%d] = 0x%x, expected_vresult_uchar[%d] = 0x%x\n",
+	     i, vresult_uchar[i], i, expected_vresult_uchar[i]);
 #else
     abort();
 #endif
   }
 
   src_a_llint = 678;
-  src_va_llint = (vector long long int) { 0, 0x101 };
+  src_va_uchar = (vector unsigned char) { 0, 0xa6, 0x2, 0, 0, 0, 0, 0,
+					  0x0, 0x1, 0, 0, 0, 0, 0, 0 };
   vresult_llint = (vector long long int) { 0, 0 };
   /* Byte index 7 will over write least significant byte of  element 0  */
-  expected_vresult_llint = (vector long long int) { 678*256, 0x100 };
+  expected_vresult_uchar
+    = (vector unsigned char) { 0x0, 0xa6, 0x2, 0x0, 0x0, 0xa6, 0x2, 0x0,
+			       0x0, 0x1, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0 };
 						 
-  vresult_uchar = vec_replace_unaligned (src_va_llint, src_a_llint, 7);
-  vresult_llint = (vector signed long long) vresult_uchar;
+  vresult_uchar = vec_replace_unaligned (src_va_uchar, src_a_llint, 7);
 
-  if (!vec_all_eq (vresult_llint,  expected_vresult_llint)) {
+  if (!vec_all_eq (vresult_uchar, expected_vresult_uchar)) {
 #if DEBUG
-    printf("ERROR, vec_replace_unaligned (src_vb_llint, src_va_llint, "
+    printf("ERROR, vec_replace_unaligned (src_va_uchar, src_va_llint, "
 	   "index)\n");
-    for(i = 0; i < 2; i++)
-      printf(" vresult_llint[%d] = %d, expected_vresult_llint[%d] = %d\n",
-	     i, vresult_llint[i], i, expected_vresult_llint[i]);
+    for (i = 0; i < 16; i++)
+      printf(" vresult_uchar[%d] = %d, expected_vresult_uchar[%d] = %d\n",
+	     i, vresult_uchar[i], i, expected_vresult_uchar[i]);
 #else
     abort();
 #endif
   }
   
   src_a_double = 678.0;
-  src_va_double = (vector double) { 0.0, 50.0 };
+  //  src_va_double = (vector double) { 0.0, 50.0 };
+  src_va_uchar = (vector unsigned char) { 0, 0, 0, 0, 0, 0, 0, 0,
+					  0, 0, 0, 0, 0, 0, 0, 0 };
   vresult_double = (vector double) { 0.0, 0.0 };
   expected_vresult_double = (vector double) { 0.0, 678.0 };
 						 
-  vresult_uchar = vec_replace_unaligned (src_va_double, src_a_double, 0);
+  vresult_uchar = vec_replace_unaligned (src_va_uchar, src_a_double, 0);
   vresult_double = (vector double) vresult_uchar;
 
-  if (!vec_all_eq (vresult_double,  expected_vresult_double)) {
+  if (!vec_all_eq (vresult_double, expected_vresult_double)) {
 #if DEBUG
-    printf("ERROR, vec_replace_unaligned (src_vb_double, src_va_double, "
+    printf("ERROR, vec_replace_unaligned (src_va_uchar, src_va_double, "
 	   "index)\n");
-    for(i = 0; i < 2; i++)
+    for (i = 0; i < 2; i++)
       printf(" vresult_double[%d] = %f, expected_vresult_double[%d] = %f\n",
 	     i, vresult_double[i], i, expected_vresult_double[i]);
 #else
@@ -295,7 +312,5 @@  main (int argc, char *argv [])
   return 0;
 }
 
-/* { dg-final { scan-assembler-times {\mvinsw\M} 6 } } */
-/* { dg-final { scan-assembler-times {\mvinsd\M} 6 } } */
-
-
+/* { dg-final { scan-assembler-times {\mvinsw\M} 8 } } */
+/* { dg-final { scan-assembler-times {\mvinsd\M} 4 } } */