[rs6000] Fix vec_permxor builtin support, fix test cases for vec_permxor and vec_insert4b

Message ID 1528232272.4981.4.camel@us.ibm.com
State New
Headers show
Series
  • [rs6000] Fix vec_permxor builtin support, fix test cases for vec_permxor and vec_insert4b
Related show

Commit Message

Carl Love June 5, 2018, 8:57 p.m.
GCC Maintainers:

The following patch fixes the return type for the existing vec_permxor
builtin to match the ABI specification.  The test case for the builtin
is also updates.

Secondly, the first argument of the vec_insert4b() builtin test case is
fixed to match the ABI specification for the builtin.

The patch was retested on:

    powerpc64le-unknown-linux-gnu (Power 8 LE)   
    powerpc64le-unknown-linux-gnu (Power 9 LE)
    powerpc64-unknown-linux-gnu (Power 8 BE)

With no regressions.

Please let me know if the patch looks OK for GCC mainline.

                         Carl Love

---------------------------------------------------------------------

gcc/ChangeLog:

2018-06-05  Carl Love  <cel@us.ibm.com>
	* gcc/config/rs6000/rs6000-c.c: Fix return type for
	P8V_BUILTIN_VPERMXOR with V16QI arguments.

gcc/testsuite/ChangeLog:

2018-06-05  Carl Love  <cel@us.ibm.com>
	* gcc.target/powerpc/builtins-7-p9-runnable.c: Change first
	argument to vui_arg.
	* gcc.target/powerpc/builtins-7-runnable.c: Change result
	to vec_uc_result1.
---
 gcc/config/rs6000/rs6000-c.c                             |  3 ++-
 .../gcc.target/powerpc/builtins-7-p9-runnable.c          |  7 +++++--
 gcc/testsuite/gcc.target/powerpc/builtins-7-runnable.c   | 16 ++++++++--------
 3 files changed, 15 insertions(+), 11 deletions(-)

-- 
2.7.4

Comments

Segher Boessenkool June 5, 2018, 9:32 p.m. | #1
Hi Carl,

On Tue, Jun 05, 2018 at 01:57:52PM -0700, Carl Love wrote:
> The following patch fixes the return type for the existing vec_permxor

> builtin to match the ABI specification.  The test case for the builtin

> is also updates.


Hrm, is that a bug in the ABI doc though?  Bill?  Most older builtins
return their source type; some newer ones always return unsigned?  Is this
on purpose?

> Secondly, the first argument of the vec_insert4b() builtin test case is

> fixed to match the ABI specification for the builtin.


The patch is fine for trunk if the ABI doc is correct.  Thanks,


Segher
Bill Schmidt June 5, 2018, 9:45 p.m. | #2
Hi Carl,

That looks like a typo in the ABI document to me.  The return type should match the
argument types like it does for the other variants.  Sorry -- I'll open a bug against
the ABI doc.

Thanks!  Good catch, Segher.

-- Bill

Bill Schmidt, Ph.D.
STSM, GCC Architect for Linux on Power
IBM Linux Technology Center
wschmidt@linux.vnet.ibm.com

> On Jun 5, 2018, at 4:32 PM, Segher Boessenkool <segher@kernel.crashing.org> wrote:

> 

> Hi Carl,

> 

> On Tue, Jun 05, 2018 at 01:57:52PM -0700, Carl Love wrote:

>> The following patch fixes the return type for the existing vec_permxor

>> builtin to match the ABI specification.  The test case for the builtin

>> is also updates.

> 

> Hrm, is that a bug in the ABI doc though?  Bill?  Most older builtins

> return their source type; some newer ones always return unsigned?  Is this

> on purpose?

> 

>> Secondly, the first argument of the vec_insert4b() builtin test case is

>> fixed to match the ABI specification for the builtin.

> 

> The patch is fine for trunk if the ABI doc is correct.  Thanks,

> 

> 

> Segher

>
Carl Love June 5, 2018, 10:04 p.m. | #3
On Tue, 2018-06-05 at 16:45 -0500, Bill Schmidt wrote:
> Hi Carl,

> 

> That looks like a typo in the ABI document to me.  The return type

> should match the

> argument types like it does for the other variants.  Sorry -- I'll

> open a bug against

> the ABI doc.


So, the ABI doc currently says:

    vector unsigned char vec_permxor (vector signed char, vector signed char, vector signed char);
    vector unsigned char vec_permxor (vector unsigned char, vector unsigned char, vector unsigned char);
     
and we want it to read:

    vector signed char vec_permxor (vector signed char, vector signed char, vector signed char);
    vector unsigned char vec_permxor (vector unsigned char, vector unsigned char, vector unsigned char);

If so, we only want the changes to vec_insert4b in builtins-7-p9-runnable.c.

I will re-spin the patch.  Thanks.

                  Carl Love

> vec_insert4b

> Thanks!  Good catch, Segher.

> 

> -- Bill

> 

> Bill Schmidt, Ph.D.

> STSM, GCC Architect for Linux on Power

> IBM Linux Technology Center

> wschmidt@linux.vnet.ibm.com

> 

> > On Jun 5, 2018, at 4:32 PM, Segher Boessenkool <segher@kernel.crash

> > ing.org> wrote:

> > 

> > Hi Carl,

> > 

> > On Tue, Jun 05, 2018 at 01:57:52PM -0700, Carl Love wrote:

> > > The following patch fixes the return type for the existing

> > > vec_permxor

> > > builtin to match the ABI specification.  The test case for the

> > > builtin

> > > is also updates.

> > 

> > Hrm, is that a bug in the ABI doc though?  Bill?  Most older

> > builtins

> > return their source type; some newer ones always return

> > unsigned?  Is this

> > on purpose?

> > 

> > > Secondly, the first argument of the vec_insert4b() builtin test

> > > case is

> > > fixed to match the ABI specification for the builtin.

> > 

> > The patch is fine for trunk if the ABI doc is correct.  Thanks,

> > 

> > 

> > Segher

> > 

> 

>
Bill Schmidt June 5, 2018, 10:05 p.m. | #4
> On Jun 5, 2018, at 5:04 PM, Carl Love <cel@us.ibm.com> wrote:

> 

> On Tue, 2018-06-05 at 16:45 -0500, Bill Schmidt wrote:

>> Hi Carl,

>> 

>> That looks like a typo in the ABI document to me.  The return type

>> should match the

>> argument types like it does for the other variants.  Sorry -- I'll

>> open a bug against

>> the ABI doc.

> 

> So, the ABI doc currently says:

> 

>    vector unsigned char vec_permxor (vector signed char, vector signed char, vector signed char);

>    vector unsigned char vec_permxor (vector unsigned char, vector unsigned char, vector unsigned char);

> 

> and we want it to read:

> 

>    vector signed char vec_permxor (vector signed char, vector signed char, vector signed char);

>    vector unsigned char vec_permxor (vector unsigned char, vector unsigned char, vector unsigned char);


Yep, correct!
Bill
> 

> If so, we only want the changes to vec_insert4b in builtins-7-p9-runnable.c.

> 

> I will re-spin the patch.  Thanks.

> 

>                  Carl Love

> 

>> vec_insert4b

>> Thanks!  Good catch, Segher.

>> 

>> -- Bill

>> 

>> Bill Schmidt, Ph.D.

>> STSM, GCC Architect for Linux on Power

>> IBM Linux Technology Center

>> wschmidt@linux.vnet.ibm.com

>> 

>>> On Jun 5, 2018, at 4:32 PM, Segher Boessenkool <segher@kernel.crash

>>> ing.org> wrote:

>>> 

>>> Hi Carl,

>>> 

>>> On Tue, Jun 05, 2018 at 01:57:52PM -0700, Carl Love wrote:

>>>> The following patch fixes the return type for the existing

>>>> vec_permxor

>>>> builtin to match the ABI specification.  The test case for the

>>>> builtin

>>>> is also updates.

>>> 

>>> Hrm, is that a bug in the ABI doc though?  Bill?  Most older

>>> builtins

>>> return their source type; some newer ones always return

>>> unsigned?  Is this

>>> on purpose?

>>> 

>>>> Secondly, the first argument of the vec_insert4b() builtin test

>>>> case is

>>>> fixed to match the ABI specification for the builtin.

>>> 

>>> The patch is fine for trunk if the ABI doc is correct.  Thanks,

>>> 

>>> 

>>> Segher

>>> 

>> 

>>
Carl Love June 6, 2018, 3:29 p.m. | #5
GCC maintainers:

Per the comment from Segher, there is a typo in the ABI.  

On Tue, 2018-06-05 at 16:45 -0500, Bill Schmidt wrote:
> Hi Carl,

> 

> That looks like a typo in the ABI document to me.  The return type

> should match the

> argument types like it does for the other variants.  Sorry -- I'll

> open a bug against

> the ABI doc.

> 

> Thanks!  Good catch, Segher.

> 


>> So, the ABI doc currently says:

> > 

> >    vector unsigned char vec_permxor (vector signed char, vector

> > signed char, vector signed char);

> >    vector unsigned char vec_permxor (vector unsigned char, vector

> > unsigned char, vector unsigned char);

> > 

> > and we want it to read:

> > 

> >    vector signed char vec_permxor (vector signed char, vector

> > signed char, vector signed char);

> >    vector unsigned char vec_permxor (vector unsigned char, vector

> > unsigned char, vector unsigned char);> 

> Yep, correct!

> Bill


Given that, the changes to match the ABI in gcc/config/rs6000/rs6000-
c.c and gcc/testsuite/gcc.target/powerpc/builtins-7-runnable.c were
removed from the patch leaving just the changes to file
gcc/testsuite/gcc.target/powerpc/builtins-7-p9-runnable.c.

The patch was retested on:

    powerpc64le-unknown-linux-gnu (Power 8 LE)   
    powerpc64le-unknown-linux-gnu (Power 9 LE)
    powerpc64-unknown-linux-gnu (Power 8 BE)

With no regressions.

Please let me know if the patch looks OK for GCC mainline.

                         Carl Love

---------------------------------------------------------------------

gcc/testsuite/ChangeLog:

2018-06-06  Carl Love  <cel@us.ibm.com>
	* gcc.target/powerpc/builtins-7-p9-runnable.c: Change first
	argument to vui_arg.
---
 gcc/testsuite/gcc.target/powerpc/builtins-7-p9-runnable.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/gcc/testsuite/gcc.target/powerpc/builtins-7-p9-runnable.c b/gcc/testsuite/gcc.target/powerpc/builtins-7-p9-runnable.c
index 137b46b..f277344 100644
--- a/gcc/testsuite/gcc.target/powerpc/builtins-7-p9-runnable.c
+++ b/gcc/testsuite/gcc.target/powerpc/builtins-7-p9-runnable.c
@@ -82,6 +82,7 @@ vext (vector unsigned char *vc)
 int main()
 {
    vector signed int vsi_arg;
+   vector unsigned int vui_arg;
    vector unsigned char vec_uc_arg, vec_uc_result, vec_uc_expected;
    vector unsigned long long vec_ull_result, vec_ull_expected;
    unsigned long long ull_result, ull_expected;
@@ -113,10 +114,12 @@ int main()
 
    /* insert into char 4 location */
    vec_uc_expected = (vector unsigned char){1, 2, 3, 4,
-					    0xC, 0, 0, 0,
+					    2, 0, 0, 0,
 					    9, 10, 11, 12,
 					    13, 14, 15, 16};
-   vec_uc_result = vec_insert4b (vsi_arg, vec_uc_arg, 4);
+   vui_arg = (vector unsigned int){0x4, 0x3, 0x2, 0x1};
+
+   vec_uc_result = vec_insert4b (vui_arg, vec_uc_arg, 4);
 
    if (result_wrong_uc(vec_uc_expected, vec_uc_result))
      {
-- 
2.7.4
Segher Boessenkool June 6, 2018, 8:53 p.m. | #6
On Wed, Jun 06, 2018 at 08:29:47AM -0700, Carl Love wrote:
> 2018-06-06  Carl Love  <cel@us.ibm.com>

> 	* gcc.target/powerpc/builtins-7-p9-runnable.c: Change first

> 	argument to vui_arg.


This is fine, please go ahead.  Thanks!


Segher

Patch

diff --git a/gcc/config/rs6000/rs6000-c.c b/gcc/config/rs6000/rs6000-c.c
index 6cf5537..ea0c82a 100644
--- a/gcc/config/rs6000/rs6000-c.c
+++ b/gcc/config/rs6000/rs6000-c.c
@@ -3614,7 +3614,8 @@  const struct altivec_builtin_types altivec_overloaded_builtins[] = {
     RS6000_BTI_bool_V16QI, RS6000_BTI_bool_V16QI, RS6000_BTI_bool_V16QI,
     RS6000_BTI_bool_V16QI },
   { P8V_BUILTIN_VEC_VPERMXOR, P8V_BUILTIN_VPERMXOR,
-    RS6000_BTI_V16QI, RS6000_BTI_V16QI, RS6000_BTI_V16QI, RS6000_BTI_V16QI },
+    RS6000_BTI_unsigned_V16QI, RS6000_BTI_V16QI,
+    RS6000_BTI_V16QI, RS6000_BTI_V16QI },
   { P8V_BUILTIN_VEC_VPERMXOR, P8V_BUILTIN_VPERMXOR,
     RS6000_BTI_unsigned_V16QI, RS6000_BTI_unsigned_V16QI,
     RS6000_BTI_unsigned_V16QI, RS6000_BTI_unsigned_V16QI },
diff --git a/gcc/testsuite/gcc.target/powerpc/builtins-7-p9-runnable.c b/gcc/testsuite/gcc.target/powerpc/builtins-7-p9-runnable.c
index 137b46b..f277344 100644
--- a/gcc/testsuite/gcc.target/powerpc/builtins-7-p9-runnable.c
+++ b/gcc/testsuite/gcc.target/powerpc/builtins-7-p9-runnable.c
@@ -82,6 +82,7 @@  vext (vector unsigned char *vc)
 int main()
 {
    vector signed int vsi_arg;
+   vector unsigned int vui_arg;
    vector unsigned char vec_uc_arg, vec_uc_result, vec_uc_expected;
    vector unsigned long long vec_ull_result, vec_ull_expected;
    unsigned long long ull_result, ull_expected;
@@ -113,10 +114,12 @@  int main()
 
    /* insert into char 4 location */
    vec_uc_expected = (vector unsigned char){1, 2, 3, 4,
-					    0xC, 0, 0, 0,
+					    2, 0, 0, 0,
 					    9, 10, 11, 12,
 					    13, 14, 15, 16};
-   vec_uc_result = vec_insert4b (vsi_arg, vec_uc_arg, 4);
+   vui_arg = (vector unsigned int){0x4, 0x3, 0x2, 0x1};
+
+   vec_uc_result = vec_insert4b (vui_arg, vec_uc_arg, 4);
 
    if (result_wrong_uc(vec_uc_expected, vec_uc_result))
      {
diff --git a/gcc/testsuite/gcc.target/powerpc/builtins-7-runnable.c b/gcc/testsuite/gcc.target/powerpc/builtins-7-runnable.c
index 965b1cd..05e7441 100644
--- a/gcc/testsuite/gcc.target/powerpc/builtins-7-runnable.c
+++ b/gcc/testsuite/gcc.target/powerpc/builtins-7-runnable.c
@@ -65,17 +65,17 @@  int main() {
 				0x4D, 0x5C, 0x6D, 0x7E,
 				0x8F, 0x90, 0xA1, 0xB2,
 				0xC3, 0xD4, 0xE5, 0xF6};
-  vec_sc_expected1 = (vector signed char){0xC, 0xC, 0xC, 0x4,
-					  0x7, 0x7, 0x5, 0xB,
-					  0xD, 0x15, 0xF, 0xC,
-					  0x4, 0x4, 0x4, 0x4};
-  vec_sc_result1 = vec_permxor (sc_arg1, sc_arg2, sc_arg3);
+  vec_uc_expected1 = (vector unsigned char){0xC, 0xC, 0xC, 0x4,
+					    0x7, 0x7, 0x5, 0xB,
+					    0xD, 0x15, 0xF, 0xC,
+					    0x4, 0x4, 0x4, 0x4};
+  vec_uc_result1 = vec_permxor (sc_arg1, sc_arg2, sc_arg3);
 
   for (i = 0; i < 16; i++) {
-    if (vec_sc_expected1[i] != vec_sc_result1[i])
+    if (vec_uc_expected1[i] != vec_uc_result1[i])
 #ifdef DEBUG
-      printf("ERROR vec_permxor (sc, sc, sc) result[%d]=0x%x != expected[%d]=0x%x\n",
-	     i, vec_sc_result1[i],  i, vec_sc_expected1[i]);
+      printf("ERROR vec_permxor (uc, sc, sc) result[%d]=0x%x != expected[%d]=0x%x\n",
+	     i, vec_uc_result1[i],  i, vec_uc_expected1[i]);
 #else
       abort();
 #endif