[PATCH] [PATCH, rs6000] Fix pr79941 (v2)

[PATCH] [PATCH, rs6000] Fix pr79941 (v2)

[Will Schmidt]

Hi,
Per PR79941, we are folding the vec_mul{e,o}* operations improperly. Those
entries were added to the intrinsics-to-be-folded list where the generic
multiplies should have been instead.  Test coverage in place was for the
generic multiplies, and this was missed by my testing.

The mul[eo]* unsigned operations were missing entries in builtin_function_type()
to indicate the overloaded arguments were unsigned.  This is corrected here,
and those operations now fold accurately to the desired instruction.

Testcases have been added for the mule/mulo operations, as well as a dg-do
run test based on the testcase in the PR.  I'll note that the variables in that
test case are now marked as volatile so they are not entirely optimized out during
the compile.

OK for trunk?
regtest is currently running on ppc64*.


gcc:
2017-03-09  Will Schmidt <will_schmidt@vnet.ibm.com>

     PR target/79941
     * config/rs6000/rs6000.c (builtin_function_type): Add VMUL*UH
     entries to the case statement that marks unsigned arguments to
     overloaded functions.

testsuite:
2017-03-09  Will Schmidt <will_schmidt@vnet.ibm.com>

     PR target/79941
     * gcc.target/powerpc/fold-vec-mult-even_odd_misc.c: New test.
     * gcc.target/powerpc/fold-vec-mult-even_odd_char.c: New test.
     * gcc.target/powerpc/fold-vec-mult-even_odd_short.c: New test.
---
 gcc/config/rs6000/rs6000.c                         |    4 +
 .../gcc.target/powerpc/fold-vec-mule-char.c        |   38 ++++++++++++
 .../gcc.target/powerpc/fold-vec-mule-misc.c        |   61 ++++++++++++++++++++
 .../gcc.target/powerpc/fold-vec-mule-short.c       |   37 ++++++++++++
 4 files changed, 140 insertions(+)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/fold-vec-mule-char.c
 create mode 100644 gcc/testsuite/gcc.target/powerpc/fold-vec-mule-misc.c
 create mode 100644 gcc/testsuite/gcc.target/powerpc/fold-vec-mule-short.c

diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index 25b10f1..42109f5 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -18530,6 +18530,10 @@ builtin_function_type (machine_mode mode_ret, machine_mode mode_arg0,
     case ALTIVEC_BUILTIN_VMULEUH_UNS:
     case ALTIVEC_BUILTIN_VMULOUB_UNS:
     case ALTIVEC_BUILTIN_VMULOUH_UNS:
+    case ALTIVEC_BUILTIN_VMULEUB:
+    case ALTIVEC_BUILTIN_VMULEUH:
+    case ALTIVEC_BUILTIN_VMULOUB:
+    case ALTIVEC_BUILTIN_VMULOUH:
     case CRYPTO_BUILTIN_VCIPHER:
     case CRYPTO_BUILTIN_VCIPHERLAST:
     case CRYPTO_BUILTIN_VNCIPHER:
diff --git a/gcc/testsuite/gcc.target/powerpc/fold-vec-mule-char.c b/gcc/testsuite/gcc.target/powerpc/fold-vec-mule-char.c
new file mode 100644
index 0000000..88cd6cf
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/fold-vec-mule-char.c
@@ -0,0 +1,38 @@
+/* Verify that overloaded built-ins for vec_mule,vec_mulo with char
+   inputs produce the right results.  */
+
+/* { dg-do compile } */
+/* { dg-require-effective-target powerpc_altivec_ok } */
+/* { dg-options "-maltivec -O2 " } */
+
+#include <altivec.h>
+
+vector signed short
+test_even (vector signed char x, vector signed char y)
+{
+  return vec_mule (x, y);
+}
+
+vector unsigned short
+test_uns_even (vector unsigned char x, vector unsigned char y)
+{
+  return vec_mule (x, y);
+}
+
+vector signed short
+test_odd (vector signed char x, vector signed char y)
+{
+  return vec_mulo (x, y);
+}
+
+vector unsigned short
+test_uns_odd (vector unsigned char x, vector unsigned char y)
+{
+  return vec_mulo (x, y);
+}
+
+/* { dg-final { scan-assembler-times "vmuleub" 1 } } */
+/* { dg-final { scan-assembler-times "vmulesb" 1 } } */
+/* { dg-final { scan-assembler-times "vmuloub" 1 } } */
+/* { dg-final { scan-assembler-times "vmulosb" 1 } } */
+
diff --git a/gcc/testsuite/gcc.target/powerpc/fold-vec-mule-misc.c b/gcc/testsuite/gcc.target/powerpc/fold-vec-mule-misc.c
new file mode 100644
index 0000000..4bb6185
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/fold-vec-mule-misc.c
@@ -0,0 +1,61 @@
+/* PR target/79941 */
+
+/* { dg-do run } */
+/* { dg-require-effective-target powerpc_vsx_ok } */
+/* { dg-options "-mvsx -O2 -save-temps" } */
+
+#include <altivec.h>
+
+__attribute__((noinline)) void 
+test_eub_char ()
+{
+  volatile vector unsigned char v0 = {1, 0, 0, 0, 0, 0, 0, 0};
+  volatile vector unsigned char v1 = {0xff, 0, 0, 0, 0, 0, 0, 0};
+  vector unsigned short res = vec_vmuleub (v0, v1);
+  if (res[0] != (unsigned short)v0[0] * (unsigned short)v1[0])
+    __builtin_abort ();
+}
+
+__attribute__((noinline)) void 
+test_oub_char ()
+{
+  volatile vector unsigned char v0 = {0, 1, 0, 0, 0, 0, 0, 0};
+  volatile vector unsigned char v1 = {0, 0xff, 0, 0, 0, 0, 0, 0};
+  vector unsigned short res = vec_vmuloub (v0, v1);
+  if (res[0] != (unsigned short)v0[1] * (unsigned short)v1[1])
+    __builtin_abort ();
+}
+
+__attribute__((noinline)) void 
+test_euh_short ()
+{
+  volatile vector unsigned short v0 = {1, 0, 0, 0};
+  volatile vector unsigned short v1 = {0xff, 0, 0, 0};
+  vector unsigned int res = vec_vmuleuh (v0, v1);
+  if (res[0] != (unsigned int)v0[0] * (unsigned int)v1[0])
+    __builtin_abort ();
+}
+
+__attribute__((noinline)) void 
+test_ouh_short ()
+{
+  volatile vector unsigned short v0 = {0, 1, 0, 0};
+  volatile vector unsigned short v1 = {0, 0xff, 0, 0};
+  vector unsigned int res = vec_vmulouh (v0, v1);
+  if (res[0] != (unsigned int)v0[1] * (unsigned int)v1[1])
+    __builtin_abort ();
+}
+
+int main ()
+{
+  test_eub_char();
+  test_oub_char();
+  test_euh_short();
+  test_ouh_short();
+}
+
+/* { dg-final { scan-assembler-times "vmuleub" 1 } } */
+/* { dg-final { scan-assembler-times "vmuloub" 1 } } */
+/* { dg-final { scan-assembler-times "vmuleuh" 1 } } */
+/* { dg-final { scan-assembler-times "vmulouh" 1 } } */
+
diff --git a/gcc/testsuite/gcc.target/powerpc/fold-vec-mule-short.c b/gcc/testsuite/gcc.target/powerpc/fold-vec-mule-short.c
new file mode 100644
index 0000000..8c856aa
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/fold-vec-mule-short.c
@@ -0,0 +1,37 @@
+/* Verify that overloaded built-ins for vec_mule,vec_mulo with short
+   inputs produce the right results.  */
+
+/* { dg-do compile } */
+/* { dg-require-effective-target powerpc_altivec_ok } */
+/* { dg-options "-maltivec -O2 " } */
+
+#include <altivec.h>
+
+vector signed int
+test_even (vector signed short x, vector signed short y)
+{
+  return vec_mule (x, y);
+}
+
+vector unsigned int
+test_uns_even (vector unsigned short x, vector unsigned short y)
+{
+  return vec_mule (x, y);
+}
+
+vector signed int
+test_odd (vector signed short x, vector signed short y)
+{
+  return vec_mulo (x, y);
+}
+
+vector unsigned int
+test_uns_odd (vector unsigned short x, vector unsigned short y)
+{
+  return vec_mulo (x, y);
+}
+
+/* { dg-final { scan-assembler-times "vmuleuh" 1 } } */
+/* { dg-final { scan-assembler-times "vmulesh" 1 } } */
+/* { dg-final { scan-assembler-times "vmulouh" 1 } } */
+/* { dg-final { scan-assembler-times "vmulosh" 1 } } */

Re: [PATCH] [PATCH, rs6000] Fix pr79941 (v2)

[Jakub Jelinek]

On Thu, Mar 09, 2017 at 10:52:52AM -0600, Will Schmidt wrote:
> Per PR79941, we are folding the vec_mul{e,o}* operations improperly. Those
> entries were added to the intrinsics-to-be-folded list where the generic
> multiplies should have been instead.  Test coverage in place was for the
> generic multiplies, and this was missed by my testing.
> 
> The mul[eo]* unsigned operations were missing entries in builtin_function_type()
> to indicate the overloaded arguments were unsigned.  This is corrected here,
> and those operations now fold accurately to the desired instruction.

This looks good to me, but I'll defer the actual review to PowerPC
maintainers.  Perhaps there was some hidden reason (xlC compatibility,
whatever) that said that vmuleub etc. should have signed vector arguments
and result.

Also, I'd like to understand what those ALTIVEC_BUILTIN_VMULEUH_UNS etc.
codes are for (the builtin doesn't seem to be user accessible).

	Jakub

Re: [PATCH] [PATCH, rs6000] Fix pr79941 (v2)

[Segher Boessenkool]

On Thu, Mar 09, 2017 at 05:59:39PM +0100, Jakub Jelinek wrote:
> On Thu, Mar 09, 2017 at 10:52:52AM -0600, Will Schmidt wrote:
> > Per PR79941, we are folding the vec_mul{e,o}* operations improperly. Those
> > entries were added to the intrinsics-to-be-folded list where the generic
> > multiplies should have been instead.  Test coverage in place was for the
> > generic multiplies, and this was missed by my testing.
> > 
> > The mul[eo]* unsigned operations were missing entries in builtin_function_type()
> > to indicate the overloaded arguments were unsigned.  This is corrected here,
> > and those operations now fold accurately to the desired instruction.
> 
> This looks good to me, but I'll defer the actual review to PowerPC
> maintainers.  Perhaps there was some hidden reason (xlC compatibility,
> whatever) that said that vmuleub etc. should have signed vector arguments
> and result.
> 
> Also, I'd like to understand what those ALTIVEC_BUILTIN_VMULEUH_UNS etc.
> codes are for (the builtin doesn't seem to be user accessible).

It used to be, but that was removed when mult-even was removed (which
seems to be the only thing it was used for).  Mike, do you remember?


Segher

Re: [PATCH] [PATCH, rs6000] Fix pr79941 (v2)

[Segher Boessenkool]

Hi Will,

On Thu, Mar 09, 2017 at 10:52:52AM -0600, Will Schmidt wrote:
> Per PR79941, we are folding the vec_mul{e,o}* operations improperly. Those
> entries were added to the intrinsics-to-be-folded list where the generic
> multiplies should have been instead.  Test coverage in place was for the
> generic multiplies, and this was missed by my testing.
> 
> The mul[eo]* unsigned operations were missing entries in builtin_function_type()
> to indicate the overloaded arguments were unsigned.  This is corrected here,
> and those operations now fold accurately to the desired instruction.
> 
> Testcases have been added for the mule/mulo operations, as well as a dg-do
> run test based on the testcase in the PR.  I'll note that the variables in that
> test case are now marked as volatile so they are not entirely optimized out during
> the compile.
> 
> OK for trunk?
> regtest is currently running on ppc64*.



>      PR target/79941
>      * config/rs6000/rs6000.c (builtin_function_type): Add VMUL*UH
>      entries to the case statement that marks unsigned arguments to
>      overloaded functions.

UH and UB?

> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/fold-vec-mule-char.c
> @@ -0,0 +1,38 @@
> +/* Verify that overloaded built-ins for vec_mule,vec_mulo with char
> +   inputs produce the right results.  */
> +
> +/* { dg-do compile } */
> +/* { dg-require-effective-target powerpc_altivec_ok } */
> +/* { dg-options "-maltivec -O2 " } */

                                 ^ this space looks off

> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/fold-vec-mule-misc.c
> @@ -0,0 +1,61 @@
> +/* PR target/79941 */
> +
> +/* { dg-do run } */
> +/* { dg-require-effective-target powerpc_vsx_ok } */
> +/* { dg-options "-mvsx -O2 -save-temps" } */

I think that -save-temps is a leftover from testing?  It shouldn't be there?

> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/fold-vec-mule-short.c
> @@ -0,0 +1,37 @@
> +/* Verify that overloaded built-ins for vec_mule,vec_mulo with short
> +   inputs produce the right results.  */
> +
> +/* { dg-do compile } */
> +/* { dg-require-effective-target powerpc_altivec_ok } */
> +/* { dg-options "-maltivec -O2 " } */

                                 ^ another

Okay with those nits fixed, but let's hear if Mike remembers anything first.

Thanks,


Segher

Re: [PATCH] [PATCH, rs6000] Fix pr79941 (v2)

[Michael Meissner]

On Thu, Mar 09, 2017 at 05:15:40PM -0600, Segher Boessenkool wrote:
> On Thu, Mar 09, 2017 at 05:59:39PM +0100, Jakub Jelinek wrote:
> > On Thu, Mar 09, 2017 at 10:52:52AM -0600, Will Schmidt wrote:
> > > Per PR79941, we are folding the vec_mul{e,o}* operations improperly. Those
> > > entries were added to the intrinsics-to-be-folded list where the generic
> > > multiplies should have been instead.  Test coverage in place was for the
> > > generic multiplies, and this was missed by my testing.
> > > 
> > > The mul[eo]* unsigned operations were missing entries in builtin_function_type()
> > > to indicate the overloaded arguments were unsigned.  This is corrected here,
> > > and those operations now fold accurately to the desired instruction.
> > 
> > This looks good to me, but I'll defer the actual review to PowerPC
> > maintainers.  Perhaps there was some hidden reason (xlC compatibility,
> > whatever) that said that vmuleub etc. should have signed vector arguments
> > and result.
> > 
> > Also, I'd like to understand what those ALTIVEC_BUILTIN_VMULEUH_UNS etc.
> > codes are for (the builtin doesn't seem to be user accessible).
> 
> It used to be, but that was removed when mult-even was removed (which
> seems to be the only thing it was used for).  Mike, do you remember?

I don't recall.  Perhaps it is related to:

2016-12-19  Will Schmidt  <will_schmidt@vnet.ibm.com>

	* config/rs6000/rs6000.c (rs6000_gimple_fold_builtin): Add handling for
	early expansion of vector multiply and subtract builtins.

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.vnet.ibm.com, phone: +1 (978) 899-4797

Re: [PATCH] [PATCH, rs6000] Fix pr79941 (v2)

[Jakub Jelinek]

On Thu, Mar 09, 2017 at 07:01:06PM -0500, Michael Meissner wrote:
> > > This looks good to me, but I'll defer the actual review to PowerPC
> > > maintainers.  Perhaps there was some hidden reason (xlC compatibility,
> > > whatever) that said that vmuleub etc. should have signed vector arguments
> > > and result.
> > > 
> > > Also, I'd like to understand what those ALTIVEC_BUILTIN_VMULEUH_UNS etc.
> > > codes are for (the builtin doesn't seem to be user accessible).
> > 
> > It used to be, but that was removed when mult-even was removed (which
> > seems to be the only thing it was used for).  Mike, do you remember?
> 
> I don't recall.  Perhaps it is related to:
> 
> 2016-12-19  Will Schmidt  <will_schmidt@vnet.ibm.com>
> 
> 	* config/rs6000/rs6000.c (rs6000_gimple_fold_builtin): Add handling for
> 	early expansion of vector multiply and subtract builtins.

That added the folding.  The questions are:
1) if it is intentional that ALTIVEC_BUILTIN_VMULEUH etc. used signed rather
than unsigned vector types as arguments and return value (and if yes, why)?
BU_ALTIVEC_2 (VMULEUH,        "vmuleuh",        CONST,  vec_widen_umult_even_v8hi)
BU_ALTIVEC_2 (VMULEUH_UNS,    "vmuleuh_uns",    CONST,  vec_widen_umult_even_v8hi)
and builtin_function_type only mentioning
      /* unsigned 2 argument functions.  */
    case ALTIVEC_BUILTIN_VMULEUH_UNS:
Back e.g. in 4.6 UNS was used in targetm.vectorize.builtin_mul_widen_even.
Does the Altivec spec say that that vec_vmuleuh arguments should be
vector signed short?  For vec_mule it chooses {uh,ub,sh,sb} depending on
whether the arguments are signed/unsigned and short/char vectors.
2) can we now remove ALTIVEC_BUILTIN_VMULEUH_UNS etc.?

	Jakub

Re: [PATCH] [PATCH, rs6000] Fix pr79941 (v2)

[Will Schmidt]

On Thu, 2017-03-09 at 17:24 -0600, Segher Boessenkool wrote:
> Hi Will,
> 
> On Thu, Mar 09, 2017 at 10:52:52AM -0600, Will Schmidt wrote:
> > Per PR79941, we are folding the vec_mul{e,o}* operations improperly. Those
> > entries were added to the intrinsics-to-be-folded list where the generic
> > multiplies should have been instead.  Test coverage in place was for the
> > generic multiplies, and this was missed by my testing.
> > 
> > The mul[eo]* unsigned operations were missing entries in builtin_function_type()
> > to indicate the overloaded arguments were unsigned.  This is corrected here,
> > and those operations now fold accurately to the desired instruction.
> > 
> > Testcases have been added for the mule/mulo operations, as well as a dg-do
> > run test based on the testcase in the PR.  I'll note that the variables in that
> > test case are now marked as volatile so they are not entirely optimized out during
> > the compile.
> > 
> > OK for trunk?
> > regtest is currently running on ppc64*.
> 
> 
> 
> >      PR target/79941
> >      * config/rs6000/rs6000.c (builtin_function_type): Add VMUL*UH
> >      entries to the case statement that marks unsigned arguments to
> >      overloaded functions.
> 
> UH and UB?

Yes.  I've updated that to read "Add VMUL*U[HB]" in my local copy.

> 
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/powerpc/fold-vec-mule-char.c
> > @@ -0,0 +1,38 @@
> > +/* Verify that overloaded built-ins for vec_mule,vec_mulo with char
> > +   inputs produce the right results.  */
> > +
> > +/* { dg-do compile } */
> > +/* { dg-require-effective-target powerpc_altivec_ok } */
> > +/* { dg-options "-maltivec -O2 " } */
> 
>                                  ^ this space looks off

It is. fixed locally in both *mule-char.c and *mule-short.c testcases.


> 
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/powerpc/fold-vec-mule-misc.c
> > @@ -0,0 +1,61 @@
> > +/* PR target/79941 */
> > +
> > +/* { dg-do run } */
> > +/* { dg-require-effective-target powerpc_vsx_ok } */
> > +/* { dg-options "-mvsx -O2 -save-temps" } */
> 
> I think that -save-temps is a leftover from testing?  It shouldn't be there?

Because this is a "dg-do run", and I am also doing a scan-assembler
stanza at the end, I needed that to preserve the intermediate.   I can
rework things if this is not considered proper.. :-) 

> 
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/powerpc/fold-vec-mule-short.c
> > @@ -0,0 +1,37 @@
> > +/* Verify that overloaded built-ins for vec_mule,vec_mulo with short
> > +   inputs produce the right results.  */
> > +
> > +/* { dg-do compile } */
> > +/* { dg-require-effective-target powerpc_altivec_ok } */
> > +/* { dg-options "-maltivec -O2 " } */
> 
>                                  ^ another

yup, got it updated locally now, thanks.  

> 
> Okay with those nits fixed, but let's hear if Mike remembers anything first.

yup.

> 
> Thanks,
> 
> 
> Segher
> 

Re: [PATCH] [PATCH, rs6000] Fix pr79941 (v2)

[Bill Schmidt]

Jumping in so we can continue the Will/Bill confusion: ;)

The official prototypes from the original AltiVec PIM are:

vector unsigned short vec_mule (vector unsigned char, vector unsigned char);
vector signed short vec_mule (vector signed char, vector signed char);

vector unsigned int vec_mule (vector unsigned short, vector unsigned short);
vector signed int vec_mule (vector signed short, vector signed short);

These are still the only supported forms.  For POWER9, we are adding similar
prototypes for 32-bit multiplies with a 64-bit result.

I do not know why we have this _UNS variant, but it seems to be something
we can do without.  It does not appear in the built-in table in rs6000-c.c  All
the entries in that built-in table are type-correct with respect to the above
definitions.  We should delete that entry from rs6000-builtin.def.

-- Bill

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

> On Mar 9, 2017, at 6:15 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> 
> On Thu, Mar 09, 2017 at 07:01:06PM -0500, Michael Meissner wrote:
>>>> This looks good to me, but I'll defer the actual review to PowerPC
>>>> maintainers.  Perhaps there was some hidden reason (xlC compatibility,
>>>> whatever) that said that vmuleub etc. should have signed vector arguments
>>>> and result.
>>>> 
>>>> Also, I'd like to understand what those ALTIVEC_BUILTIN_VMULEUH_UNS etc.
>>>> codes are for (the builtin doesn't seem to be user accessible).
>>> 
>>> It used to be, but that was removed when mult-even was removed (which
>>> seems to be the only thing it was used for).  Mike, do you remember?
>> 
>> I don't recall.  Perhaps it is related to:
>> 
>> 2016-12-19  Will Schmidt  <will_schmidt@vnet.ibm.com>
>> 
>> 	* config/rs6000/rs6000.c (rs6000_gimple_fold_builtin): Add handling for
>> 	early expansion of vector multiply and subtract builtins.
> 
> That added the folding.  The questions are:
> 1) if it is intentional that ALTIVEC_BUILTIN_VMULEUH etc. used signed rather
> than unsigned vector types as arguments and return value (and if yes, why)?
> BU_ALTIVEC_2 (VMULEUH,        "vmuleuh",        CONST,  vec_widen_umult_even_v8hi)
> BU_ALTIVEC_2 (VMULEUH_UNS,    "vmuleuh_uns",    CONST,  vec_widen_umult_even_v8hi)
> and builtin_function_type only mentioning
>      /* unsigned 2 argument functions.  */
>    case ALTIVEC_BUILTIN_VMULEUH_UNS:
> Back e.g. in 4.6 UNS was used in targetm.vectorize.builtin_mul_widen_even.
> Does the Altivec spec say that that vec_vmuleuh arguments should be
> vector signed short?  For vec_mule it chooses {uh,ub,sh,sb} depending on
> whether the arguments are signed/unsigned and short/char vectors.
> 2) can we now remove ALTIVEC_BUILTIN_VMULEUH_UNS etc.?
> 
> 	Jakub
> 

Re: [PATCH] [PATCH, rs6000] Fix pr79941 (v2)

[Segher Boessenkool]

On Fri, Mar 10, 2017 at 08:08:06AM -0600, Will Schmidt wrote:
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.target/powerpc/fold-vec-mule-misc.c
> > > @@ -0,0 +1,61 @@
> > > +/* PR target/79941 */
> > > +
> > > +/* { dg-do run } */
> > > +/* { dg-require-effective-target powerpc_vsx_ok } */
> > > +/* { dg-options "-mvsx -O2 -save-temps" } */
> > 
> > I think that -save-temps is a leftover from testing?  It shouldn't be there?
> 
> Because this is a "dg-do run", and I am also doing a scan-assembler
> stanza at the end, I needed that to preserve the intermediate.   I can
> rework things if this is not considered proper.. :-) 

Oh I missed that.  No, it is fine then.


Segher

Re: [PATCH] [PATCH, rs6000] Fix pr79941 (v2)

[Bill Schmidt]

Just so there's no duplication of effort -- I'll follow up with a patch to remove the
outdated built-in.

-- Bill

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

> On Mar 10, 2017, at 8:24 AM, Bill Schmidt <wschmidt@linux.vnet.ibm.com> wrote:
> 
> Jumping in so we can continue the Will/Bill confusion: ;)
> 
> The official prototypes from the original AltiVec PIM are:
> 
> vector unsigned short vec_mule (vector unsigned char, vector unsigned char);
> vector signed short vec_mule (vector signed char, vector signed char);
> 
> vector unsigned int vec_mule (vector unsigned short, vector unsigned short);
> vector signed int vec_mule (vector signed short, vector signed short);
> 
> These are still the only supported forms.  For POWER9, we are adding similar
> prototypes for 32-bit multiplies with a 64-bit result.
> 
> I do not know why we have this _UNS variant, but it seems to be something
> we can do without.  It does not appear in the built-in table in rs6000-c.c  All
> the entries in that built-in table are type-correct with respect to the above
> definitions.  We should delete that entry from rs6000-builtin.def.
> 
> -- Bill
> 
> Bill Schmidt, Ph.D.
> GCC for Linux on Power
> Linux on Power Toolchain
> IBM Linux Technology Center
> wschmidt@linux.vnet.ibm.com
> 
>> On Mar 9, 2017, at 6:15 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> 
>> On Thu, Mar 09, 2017 at 07:01:06PM -0500, Michael Meissner wrote:
>>>>> This looks good to me, but I'll defer the actual review to PowerPC
>>>>> maintainers.  Perhaps there was some hidden reason (xlC compatibility,
>>>>> whatever) that said that vmuleub etc. should have signed vector arguments
>>>>> and result.
>>>>> 
>>>>> Also, I'd like to understand what those ALTIVEC_BUILTIN_VMULEUH_UNS etc.
>>>>> codes are for (the builtin doesn't seem to be user accessible).
>>>> 
>>>> It used to be, but that was removed when mult-even was removed (which
>>>> seems to be the only thing it was used for).  Mike, do you remember?
>>> 
>>> I don't recall.  Perhaps it is related to:
>>> 
>>> 2016-12-19  Will Schmidt  <will_schmidt@vnet.ibm.com>
>>> 
>>> 	* config/rs6000/rs6000.c (rs6000_gimple_fold_builtin): Add handling for
>>> 	early expansion of vector multiply and subtract builtins.
>> 
>> That added the folding.  The questions are:
>> 1) if it is intentional that ALTIVEC_BUILTIN_VMULEUH etc. used signed rather
>> than unsigned vector types as arguments and return value (and if yes, why)?
>> BU_ALTIVEC_2 (VMULEUH,        "vmuleuh",        CONST,  vec_widen_umult_even_v8hi)
>> BU_ALTIVEC_2 (VMULEUH_UNS,    "vmuleuh_uns",    CONST,  vec_widen_umult_even_v8hi)
>> and builtin_function_type only mentioning
>>     /* unsigned 2 argument functions.  */
>>   case ALTIVEC_BUILTIN_VMULEUH_UNS:
>> Back e.g. in 4.6 UNS was used in targetm.vectorize.builtin_mul_widen_even.
>> Does the Altivec spec say that that vec_vmuleuh arguments should be
>> vector signed short?  For vec_mule it chooses {uh,ub,sh,sb} depending on
>> whether the arguments are signed/unsigned and short/char vectors.
>> 2) can we now remove ALTIVEC_BUILTIN_VMULEUH_UNS etc.?
>> 
>> 	Jakub
>> 
> 

Re: [PATCH] [PATCH, rs6000] Fix pr79941 (v2)

[Andreas Schwab]

On Mär 09 2017, Will Schmidt <will_schmidt@vnet.ibm.com> wrote:

> diff --git a/gcc/testsuite/gcc.target/powerpc/fold-vec-mule-misc.c b/gcc/testsuite/gcc.target/powerpc/fold-vec-mule-misc.c
> new file mode 100644
> index 0000000..4bb6185
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/fold-vec-mule-misc.c
> @@ -0,0 +1,61 @@
> +/* PR target/79941 */
> +
> +/* { dg-do run } */
> +/* { dg-require-effective-target powerpc_vsx_ok } */
> +/* { dg-options "-mvsx -O2 -save-temps" } */

FAIL: gcc.target/powerpc/fold-vec-mule-misc.c execution test

$ ./fold-vec-mule-misc.exe
Illegal instruction

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] [PATCH, rs6000] Fix pr79941 (v2)

[Segher Boessenkool]

On Sun, Apr 02, 2017 at 09:26:24AM +0200, Andreas Schwab wrote:
> > diff --git a/gcc/testsuite/gcc.target/powerpc/fold-vec-mule-misc.c b/gcc/testsuite/gcc.target/powerpc/fold-vec-mule-misc.c
> > new file mode 100644
> > index 0000000..4bb6185
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/powerpc/fold-vec-mule-misc.c
> > @@ -0,0 +1,61 @@
> > +/* PR target/79941 */
> > +
> > +/* { dg-do run } */
> > +/* { dg-require-effective-target powerpc_vsx_ok } */
> > +/* { dg-options "-mvsx -O2 -save-temps" } */
> 
> FAIL: gcc.target/powerpc/fold-vec-mule-misc.c execution test
> 
> $ ./fold-vec-mule-misc.exe
> Illegal instruction

It should test vsx_hw, not just vsx_ok.  I'll handle it (and the other
problem you found).  Thanks, and sorry for not spotting the problems!


Segher

Re: [PATCH] [PATCH, rs6000] Fix pr79941 (v2)

[Segher Boessenkool]

On Sun, Apr 02, 2017 at 09:26:24AM +0200, Andreas Schwab wrote:
> > +/* PR target/79941 */
> > +
> > +/* { dg-do run } */
> > +/* { dg-require-effective-target powerpc_vsx_ok } */
> > +/* { dg-options "-mvsx -O2 -save-temps" } */
> 
> FAIL: gcc.target/powerpc/fold-vec-mule-misc.c execution test
> 
> $ ./fold-vec-mule-misc.exe
> Illegal instruction

I cannot get this one to fail, unless I explicitly use -mcpu=power8 or
similar (which will not run on a power7, direct move insns do not yet
exist on power7).

What is different about your setup?  (What *is* your setup?)


Segher

Re: [PATCH] [PATCH, rs6000] Fix pr79941 (v2)

[Andreas Schwab]

On Apr 02 2017, Segher Boessenkool <segher@kernel.crashing.org> wrote:

> On Sun, Apr 02, 2017 at 09:26:24AM +0200, Andreas Schwab wrote:
>> > +/* PR target/79941 */
>> > +
>> > +/* { dg-do run } */
>> > +/* { dg-require-effective-target powerpc_vsx_ok } */
>> > +/* { dg-options "-mvsx -O2 -save-temps" } */
>> 
>> FAIL: gcc.target/powerpc/fold-vec-mule-misc.c execution test
>> 
>> $ ./fold-vec-mule-misc.exe
>> Illegal instruction
>
> I cannot get this one to fail, unless I explicitly use -mcpu=power8 or
> similar (which will not run on a power7, direct move insns do not yet
> exist on power7).
>
> What is different about your setup?  (What *is* your setup?)

http://gcc.gnu.org/ml/gcc-testresults/2017-04/msg00126.html

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] [PATCH, rs6000] Fix pr79941 (v2)

[Segher Boessenkool]

On Sun, Apr 02, 2017 at 03:45:05PM +0200, Andreas Schwab wrote:
> > On Sun, Apr 02, 2017 at 09:26:24AM +0200, Andreas Schwab wrote:
> >> > +/* PR target/79941 */
> >> > +
> >> > +/* { dg-do run } */
> >> > +/* { dg-require-effective-target powerpc_vsx_ok } */
> >> > +/* { dg-options "-mvsx -O2 -save-temps" } */
> >> 
> >> FAIL: gcc.target/powerpc/fold-vec-mule-misc.c execution test
> >> 
> >> $ ./fold-vec-mule-misc.exe
> >> Illegal instruction
> >
> > I cannot get this one to fail, unless I explicitly use -mcpu=power8 or
> > similar (which will not run on a power7, direct move insns do not yet
> > exist on power7).
> >
> > What is different about your setup?  (What *is* your setup?)
> 
> http://gcc.gnu.org/ml/gcc-testresults/2017-04/msg00126.html

That only says it is powerpc64-linux.  I don't see these problems there
(or anywhere else).

The testcase should not run on anything older than a Power7.  Is your
system something older?  Why does powerpc_vsx_ok return true then, ugh.


Segher

Re: [PATCH] [PATCH, rs6000] Fix pr79941 (v2)

[Andreas Schwab]

On Apr 02 2017, Segher Boessenkool <segher@kernel.crashing.org> wrote:

> Why does powerpc_vsx_ok return true then, ugh.

Because the assembler is new enough.  That's all it checks.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."