[PATCH] rs6000: Fix vec_cpsgn parameter order (PR101985)

[PATCH] rs6000: Fix vec_cpsgn parameter order (PR101985)

[Bill Schmidt]

Hi!

This fixes a bug reported in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101985.

The vec_cpsgn built-in function API differs in argument order from the
copysign<mode>3 convention.  Currently that pattern is incorrctly used to
implement vec_cpsgn.  Fix that while leaving the existing pattern in place
to implement copysignf for vector modes.

Part of the fix when using the new built-in support requires an adjustment
to a pending patch that replaces much of altivec.h with an automatically
generated file.  So that adjustment will be coming later...

Also fix a bug in the new built-in overload infrastructure where we were
using the VSX form of the VEC_COPYSIGN built-in when we should default to
the VMX form.

Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no regressions.
Is this okay for trunk?

Thanks!
Bill


2021-09-24  Bill Schmidt  <wschmidt@linux.ibm.com>

gcc/
	PR target/101985
	* config/rs6000/altivec.h (vec_cpsgn): Adjust.
	* config/rs6000/rs6000-overload.def (VEC_COPYSIGN): Use SKIP to
	avoid generating an automatic #define of vec_cpsgn.  Use the
	correct built-in for V4SFmode that doesn't depend on VSX.

gcc/testsuite/
	PR target/101985
	* gcc.target/powerpc/pr101985.c: New.
---
 gcc/config/rs6000/altivec.h                   |  2 +-
 gcc/config/rs6000/rs6000-overload.def         |  4 ++--
 gcc/testsuite/gcc.target/powerpc/pr101985-1.c | 18 ++++++++++++++++++
 gcc/testsuite/gcc.target/powerpc/pr101985-2.c | 18 ++++++++++++++++++
 4 files changed, 39 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pr101985-1.c
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pr101985-2.c

diff --git a/gcc/config/rs6000/altivec.h b/gcc/config/rs6000/altivec.h
index 5b631c7ebaf..ea72c9c1789 100644
--- a/gcc/config/rs6000/altivec.h
+++ b/gcc/config/rs6000/altivec.h
@@ -129,7 +129,7 @@
 #define vec_vcfux __builtin_vec_vcfux
 #define vec_cts __builtin_vec_cts
 #define vec_ctu __builtin_vec_ctu
-#define vec_cpsgn __builtin_vec_copysign
+#define vec_cpsgn(x,y) __builtin_vec_copysign(y,x)
 #define vec_double __builtin_vec_double
 #define vec_doublee __builtin_vec_doublee
 #define vec_doubleo __builtin_vec_doubleo
diff --git a/gcc/config/rs6000/rs6000-overload.def b/gcc/config/rs6000/rs6000-overload.def
index 141f831e2c0..4f583312f36 100644
--- a/gcc/config/rs6000/rs6000-overload.def
+++ b/gcc/config/rs6000/rs6000-overload.def
@@ -1154,9 +1154,9 @@
   vus __builtin_vec_convert_4f32_8f16 (vf, vf);
     CONVERT_4F32_8F16
 
-[VEC_COPYSIGN, vec_cpsgn, __builtin_vec_copysign]
+[VEC_COPYSIGN, SKIP, __builtin_vec_copysign]
   vf __builtin_vec_copysign (vf, vf);
-    CPSGNSP
+    COPYSIGN_V4SF
   vd __builtin_vec_copysign (vd, vd);
     CPSGNDP
 
diff --git a/gcc/testsuite/gcc.target/powerpc/pr101985-1.c b/gcc/testsuite/gcc.target/powerpc/pr101985-1.c
new file mode 100644
index 00000000000..a1ec2d68d53
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr101985-1.c
@@ -0,0 +1,18 @@
+/* PR target/101985 */
+/* { dg-do run } */
+/* { dg-require-effective-target vsx_hw } */
+/* { dg-options "-O2" } */
+
+#include <altivec.h>
+
+int
+main (void)
+{
+  vector float a = {  1,  2, - 3, - 4};
+  vector float b = {-10, 20, -30,  40};
+  vector float c = { 10, 20, -30, -40};
+  a = vec_cpsgn (a, b);
+  if (! vec_all_eq (a, c))
+    __builtin_abort ();
+  return 0;
+}
diff --git a/gcc/testsuite/gcc.target/powerpc/pr101985-2.c b/gcc/testsuite/gcc.target/powerpc/pr101985-2.c
new file mode 100644
index 00000000000..71cc254c170
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr101985-2.c
@@ -0,0 +1,18 @@
+/* PR target/101985 */
+/* { dg-do run } */
+/* { dg-require-effective-target vsx_hw } */
+/* { dg-options "-O2" } */
+
+#include <altivec.h>
+
+int
+main (void)
+{
+  vector double a = {  1,  -4};
+  vector double b = { -10,  40};
+  vector double c = {  10, -40};
+  a = vec_cpsgn (a, b);
+  if (! vec_all_eq (a, c))
+    __builtin_abort ();
+  return 0;
+}
-- 
2.27.0

Re: [PATCH] rs6000: Fix vec_cpsgn parameter order (PR101985)

[Bill Schmidt]

Hi!  Ping, please. :)

Bill

On 9/24/21 10:20 AM, Bill Schmidt wrote:
> Hi!
>
> This fixes a bug reported in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101985.
>
> The vec_cpsgn built-in function API differs in argument order from the
> copysign<mode>3 convention.  Currently that pattern is incorrctly used to
> implement vec_cpsgn.  Fix that while leaving the existing pattern in place
> to implement copysignf for vector modes.
>
> Part of the fix when using the new built-in support requires an adjustment
> to a pending patch that replaces much of altivec.h with an automatically
> generated file.  So that adjustment will be coming later...
>
> Also fix a bug in the new built-in overload infrastructure where we were
> using the VSX form of the VEC_COPYSIGN built-in when we should default to
> the VMX form.
>
> Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no regressions.
> Is this okay for trunk?
>
> Thanks!
> Bill
>
>
> 2021-09-24  Bill Schmidt  <wschmidt@linux.ibm.com>
>
> gcc/
> 	PR target/101985
> 	* config/rs6000/altivec.h (vec_cpsgn): Adjust.
> 	* config/rs6000/rs6000-overload.def (VEC_COPYSIGN): Use SKIP to
> 	avoid generating an automatic #define of vec_cpsgn.  Use the
> 	correct built-in for V4SFmode that doesn't depend on VSX.
>
> gcc/testsuite/
> 	PR target/101985
> 	* gcc.target/powerpc/pr101985.c: New.
> ---
>  gcc/config/rs6000/altivec.h                   |  2 +-
>  gcc/config/rs6000/rs6000-overload.def         |  4 ++--
>  gcc/testsuite/gcc.target/powerpc/pr101985-1.c | 18 ++++++++++++++++++
>  gcc/testsuite/gcc.target/powerpc/pr101985-2.c | 18 ++++++++++++++++++
>  4 files changed, 39 insertions(+), 3 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/powerpc/pr101985-1.c
>  create mode 100644 gcc/testsuite/gcc.target/powerpc/pr101985-2.c
>
> diff --git a/gcc/config/rs6000/altivec.h b/gcc/config/rs6000/altivec.h
> index 5b631c7ebaf..ea72c9c1789 100644
> --- a/gcc/config/rs6000/altivec.h
> +++ b/gcc/config/rs6000/altivec.h
> @@ -129,7 +129,7 @@
>  #define vec_vcfux __builtin_vec_vcfux
>  #define vec_cts __builtin_vec_cts
>  #define vec_ctu __builtin_vec_ctu
> -#define vec_cpsgn __builtin_vec_copysign
> +#define vec_cpsgn(x,y) __builtin_vec_copysign(y,x)
>  #define vec_double __builtin_vec_double
>  #define vec_doublee __builtin_vec_doublee
>  #define vec_doubleo __builtin_vec_doubleo
> diff --git a/gcc/config/rs6000/rs6000-overload.def b/gcc/config/rs6000/rs6000-overload.def
> index 141f831e2c0..4f583312f36 100644
> --- a/gcc/config/rs6000/rs6000-overload.def
> +++ b/gcc/config/rs6000/rs6000-overload.def
> @@ -1154,9 +1154,9 @@
>    vus __builtin_vec_convert_4f32_8f16 (vf, vf);
>      CONVERT_4F32_8F16
>  
> -[VEC_COPYSIGN, vec_cpsgn, __builtin_vec_copysign]
> +[VEC_COPYSIGN, SKIP, __builtin_vec_copysign]
>    vf __builtin_vec_copysign (vf, vf);
> -    CPSGNSP
> +    COPYSIGN_V4SF
>    vd __builtin_vec_copysign (vd, vd);
>      CPSGNDP
>  
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr101985-1.c b/gcc/testsuite/gcc.target/powerpc/pr101985-1.c
> new file mode 100644
> index 00000000000..a1ec2d68d53
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr101985-1.c
> @@ -0,0 +1,18 @@
> +/* PR target/101985 */
> +/* { dg-do run } */
> +/* { dg-require-effective-target vsx_hw } */
> +/* { dg-options "-O2" } */
> +
> +#include <altivec.h>
> +
> +int
> +main (void)
> +{
> +  vector float a = {  1,  2, - 3, - 4};
> +  vector float b = {-10, 20, -30,  40};
> +  vector float c = { 10, 20, -30, -40};
> +  a = vec_cpsgn (a, b);
> +  if (! vec_all_eq (a, c))
> +    __builtin_abort ();
> +  return 0;
> +}
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr101985-2.c b/gcc/testsuite/gcc.target/powerpc/pr101985-2.c
> new file mode 100644
> index 00000000000..71cc254c170
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr101985-2.c
> @@ -0,0 +1,18 @@
> +/* PR target/101985 */
> +/* { dg-do run } */
> +/* { dg-require-effective-target vsx_hw } */
> +/* { dg-options "-O2" } */
> +
> +#include <altivec.h>
> +
> +int
> +main (void)
> +{
> +  vector double a = {  1,  -4};
> +  vector double b = { -10,  40};
> +  vector double c = {  10, -40};
> +  a = vec_cpsgn (a, b);
> +  if (! vec_all_eq (a, c))
> +    __builtin_abort ();
> +  return 0;
> +}

Re: [PATCH] rs6000: Fix vec_cpsgn parameter order (PR101985)

[David Edelsohn]

On Fri, Sep 24, 2021 at 11:20 AM Bill Schmidt <wschmidt@linux.ibm.com> wrote:
>
> Hi!
>
> This fixes a bug reported in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101985.
>
> The vec_cpsgn built-in function API differs in argument order from the
> copysign<mode>3 convention.  Currently that pattern is incorrectly used to
> implement vec_cpsgn.  Fix that while leaving the existing pattern in place
> to implement copysignf for vector modes.

It's a little confusing what "that" is.  Maybe clarify that the patch
is changing the PowerPC VSX function to invoke the GCC built-in with
the argument in the correct order.

>
> Part of the fix when using the new built-in support requires an adjustment
> to a pending patch that replaces much of altivec.h with an automatically
> generated file.  So that adjustment will be coming later...
>
> Also fix a bug in the new built-in overload infrastructure where we were
> using the VSX form of the VEC_COPYSIGN built-in when we should default to
> the VMX form.
>
> Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no regressions.
> Is this okay for trunk?
>
> Thanks!
> Bill
>
>
> 2021-09-24  Bill Schmidt  <wschmidt@linux.ibm.com>
>
> gcc/
>         PR target/101985
>         * config/rs6000/altivec.h (vec_cpsgn): Adjust.

Maybe a little more information than "Adjust"?  Swap arguments?

>         * config/rs6000/rs6000-overload.def (VEC_COPYSIGN): Use SKIP to
>         avoid generating an automatic #define of vec_cpsgn.  Use the
>         correct built-in for V4SFmode that doesn't depend on VSX.
>
> gcc/testsuite/
>         PR target/101985
>         * gcc.target/powerpc/pr101985.c: New.

Okay.

Thanks, David

Re: [PATCH] rs6000: Fix vec_cpsgn parameter order (PR101985)

[Segher Boessenkool]

On Fri, Sep 24, 2021 at 10:20:46AM -0500, Bill Schmidt wrote:
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr101985-1.c
> @@ -0,0 +1,18 @@
> +/* PR target/101985 */
> +/* { dg-do run } */
> +/* { dg-require-effective-target vsx_hw } */
> +/* { dg-options "-O2" } */

If you need vsx_hw (or vsx_ok), you need -mvsx in the options as well.
(Always, so in both testcases here).


Segher

Re: [PATCH] rs6000: Fix vec_cpsgn parameter order (PR101985)

[Bill Schmidt]

Hi!

On 10/12/21 4:37 PM, Segher Boessenkool wrote:
> On Fri, Sep 24, 2021 at 10:20:46AM -0500, Bill Schmidt wrote:
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/powerpc/pr101985-1.c
>> @@ -0,0 +1,18 @@
>> +/* PR target/101985 */
>> +/* { dg-do run } */
>> +/* { dg-require-effective-target vsx_hw } */
>> +/* { dg-options "-O2" } */
> If you need vsx_hw (or vsx_ok), you need -mvsx in the options as well.
> (Always, so in both testcases here).

Whoops.  Fixed, and adjusted ChangeLog/commit message per David.  Committed.

Thanks for the reviews!!
Bill
>
>
> Segher