[v2,rs6000] Fix implementation of vec_pack (vector double, vector double) built-in function

This patch fixes an error in the code generation for vec_pack (vector double, vector double).  As previously implemented, this built-in function translates to the vpkudum instruction.

This patch causes vec_pack (vector double, vector double) to behave the same as vec_float2 for the same type signature, producing the vmrgow instruction on little-endian targets and the vmrgew instruction on big-endian targets.

This revision differs from the initial path submission in that it combines all of the new testing into two test programs, using target qualifiers on the dg scan-assembler-times directives.

This patch has bootstrapped and tested without regressions on powerpc64le-unknown-linux (both P8 and P9) and on powerpc-linux (P8 big-endian, both -m32 and -m64).

Is this ok for the trunk?

gcc/ChangeLog:

2018-06-19  Kelvin Nilsen  <kelvin@gcc.gnu.org>

	* config/rs6000/rs6000-c.c (altivec_overloaded_builtins): Change
	behavior of vec_pack (vector double, vector double) to match
	behavior of vec_float2 (vector double, vector double).

gcc/testsuite/ChangeLog:

2018-06-19  Kelvin Nilsen  <kelvin@gcc.gnu.org>

	* gcc.target/powerpc/builtins-3-p8.c (test_pack_float): Remove
	this test.
	* gcc.target/powerpc/builtins-9.c: New test.
	* gcc.target/powerpc/fold-vec-pack-double.c: Modify dg directives
	to expect different code generation on big-endian
	vs. little-endian targets.

Hi!

On Tue, Jun 19, 2018 at 01:37:51PM -0500, Kelvin Nilsen wrote:
> --- gcc/testsuite/gcc.target/powerpc/builtins-9.c	(nonexistent)
> +++ gcc/testsuite/gcc.target/powerpc/builtins-9.c	(working copy)
> @@ -0,0 +1,21 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target powerpc_p8vector_ok } */
> +/* Expect same instruction selecton on p8 and above.  Fix if future
> +   targets behave differently.  */
> +/* { dg-options "-O3 -maltivec" } */

But this doesn't use -mcpu=power8 or similar.  Does it need it anyway?
Both xxpermdi and xvcvdpsp are Power7 (ISA 2.06) and the rest is AltiVec?
So maybe just powerpc_vsx_ok?

> +/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } } */

You do not use -mcpu= so you don't need this.

Same issues in the next test.  Rest looks good though :-)


Segher

Thanks for feedback.  It turns out that the vmrgew and vmrgow instructions require power 8.

After coordinating with Segher on minor refinements to the test cases, I have committed the patch as quoted below to the trunk.

On 6/19/18 5:37 PM, Segher Boessenkool wrote:
> Hi!
> 
> On Tue, Jun 19, 2018 at 01:37:51PM -0500, Kelvin Nilsen wrote:
>> --- gcc/testsuite/gcc.target/powerpc/builtins-9.c	(nonexistent)
>> +++ gcc/testsuite/gcc.target/powerpc/builtins-9.c	(working copy)
>> @@ -0,0 +1,21 @@
>> +/* { dg-do compile } */
>> +/* { dg-require-effective-target powerpc_p8vector_ok } */
>> +/* Expect same instruction selecton on p8 and above.  Fix if future
>> +   targets behave differently.  */
>> +/* { dg-options "-O3 -maltivec" } */
> 
> But this doesn't use -mcpu=power8 or similar.  Does it need it anyway?
> Both xxpermdi and xvcvdpsp are Power7 (ISA 2.06) and the rest is AltiVec?
> So maybe just powerpc_vsx_ok?
> 
>> +/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } } */
> 
> You do not use -mcpu= so you don't need this.
> 
> Same issues in the next test.  Rest looks good though :-)
> 
> 
> Segher
> 
> 

gcc/ChangeLog:

2018-06-22  Kelvin Nilsen  <kelvin@gcc.gnu.org>

	* config/rs6000/rs6000-c.c (altivec_overloaded_builtins): Change
	behavior of vec_pack (vector double, vector double) to match
	behavior of vec_float2 (vector double, vector double).

gcc/testsuite/ChangeLog:

2018-06-22  Kelvin Nilsen  <kelvin@gcc.gnu.org>

	* gcc.target/powerpc/builtins-3-p8.c (test_pack_float): Remove
	this test.
	* gcc.target/powerpc/builtins-9.c: New test.
	* gcc.target/powerpc/fold-vec-pack-double.c: Modify dg directives
	to expect different code generation on big-endian vs.
	little-endian targets.

Index: gcc/config/rs6000/rs6000-c.c
===================================================================
--- gcc/config/rs6000/rs6000-c.c	(revision 261775)
+++ gcc/config/rs6000/rs6000-c.c	(working copy)
@@ -2425,7 +2425,7 @@ const struct altivec_builtin_types altivec_overloa
     RS6000_BTI_unsigned_V4SI, RS6000_BTI_unsigned_V2DI, RS6000_BTI_unsigned_V2DI, 0 },
   { ALTIVEC_BUILTIN_VEC_PACK, P8V_BUILTIN_VPKUDUM,
     RS6000_BTI_bool_V4SI, RS6000_BTI_bool_V2DI, RS6000_BTI_bool_V2DI, 0 },
-  { ALTIVEC_BUILTIN_VEC_PACK, P8V_BUILTIN_VPKUDUM,
+  { ALTIVEC_BUILTIN_VEC_PACK, P8V_BUILTIN_FLOAT2_V2DF,
     RS6000_BTI_V4SF, RS6000_BTI_V2DF, RS6000_BTI_V2DF, 0 },
 
   { P8V_BUILTIN_VEC_NEG, P8V_BUILTIN_NEG_V16QI,
Index: gcc/testsuite/gcc.target/powerpc/builtins-3-p8.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/builtins-3-p8.c	(revision 261775)
+++ gcc/testsuite/gcc.target/powerpc/builtins-3-p8.c	(working copy)
@@ -11,12 +11,6 @@ test_eq_long_long (vector bool long long x, vector
 	return vec_cmpeq (x, y);
 }
 
-vector float
-test_pack_float (vector double x, vector double y)
-{
-  return vec_pack (x, y);
-}
-
 vector unsigned char
 test_vsi_packs_vusi_vusi (vector unsigned short x,
                           vector unsigned short y)
@@ -214,7 +208,6 @@ test_neg_double (vector double x)
 /* Expected test results:
 
      test_eq_long_long                         1 vcmpequd inst
-     test_pack_float                           1 vpkudum inst
      test_vsi_packs_vsll_vsll                  1 vpksdss
      test_vui_packs_vull_vull                  1 vpkudus
      test_vui_packs_vssi_vssi                  1 vpkshss
@@ -239,7 +232,6 @@ test_neg_double (vector double x)
  */
 
 /* { dg-final { scan-assembler-times "vcmpequd" 1 } } */
-/* { dg-final { scan-assembler-times "vpkudum"  1 } } */
 /* { dg-final { scan-assembler-times "vpksdss"  1 } } */
 /* { dg-final { scan-assembler-times "vpkudus"  1 } } */  
 /* { dg-final { scan-assembler-times "vpkuhus"  2 } } */
Index: gcc/testsuite/gcc.target/powerpc/builtins-9.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/builtins-9.c	(nonexistent)
+++ gcc/testsuite/gcc.target/powerpc/builtins-9.c	(working copy)
@@ -0,0 +1,19 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target powerpc_p8vector_ok } */
+/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=power8" } } */
+/* { dg-options "-maltivec -mcpu=power8 -O3" } */
+
+#include <altivec.h>
+
+vector float
+test_pack_float (vector double x, vector double y)
+{
+  return vec_pack (x, y);
+}
+
+/* { dg-final { scan-assembler-times "vmrgew" 1 { target be } } } */
+/* { dg-final { scan-assembler-times "vmrgow"  1 { target le } } } */
+
+/* { dg-final { scan-assembler-times "xvcvdpsp"  2 } } */
+/* { dg-final { scan-assembler-times "xxpermdi"  2 } } */
+
Index: gcc/testsuite/gcc.target/powerpc/fold-vec-pack-double.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/fold-vec-pack-double.c	(revision 261775)
+++ gcc/testsuite/gcc.target/powerpc/fold-vec-pack-double.c	(working copy)
@@ -3,6 +3,7 @@
 
 /* { dg-do compile } */
 /* { dg-require-effective-target powerpc_p8vector_ok } */
+/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=power8" } } */
 /* { dg-options "-mvsx -mpower8-vector -O2" } */
 
 #include <altivec.h>
@@ -15,4 +16,5 @@ test_pack (vector double vd2, vector double vd3)
   return vec_pack (vd2, vd3);
 }
 
-/* { dg-final { scan-assembler-times "vpkudum" 1 } } */
+/* { dg-final { scan-assembler-times "vmrgew" 1 { target be } } } */
+/* { dg-final { scan-assembler-times "vmrgow"  1 { target le } } } */

Hi Segher,

After waiting a few days for this newly committed patch to settle, is it ok to backport to gcc 6, gcc 7, and gcc 8?

Thanks.


On 6/22/18 5:34 PM, Kelvin Nilsen wrote:
> Thanks for feedback.  It turns out that the vmrgew and vmrgow instructions require power 8.
> 
> After coordinating with Segher on minor refinements to the test cases, I have committed the patch as quoted below to the trunk.
> 
> On 6/19/18 5:37 PM, Segher Boessenkool wrote:
>> Hi!
>>
>> On Tue, Jun 19, 2018 at 01:37:51PM -0500, Kelvin Nilsen wrote:
>>> --- gcc/testsuite/gcc.target/powerpc/builtins-9.c	(nonexistent)
>>> +++ gcc/testsuite/gcc.target/powerpc/builtins-9.c	(working copy)
>>> @@ -0,0 +1,21 @@
>>> +/* { dg-do compile } */
>>> +/* { dg-require-effective-target powerpc_p8vector_ok } */
>>> +/* Expect same instruction selecton on p8 and above.  Fix if future
>>> +   targets behave differently.  */
>>> +/* { dg-options "-O3 -maltivec" } */
>>
>> But this doesn't use -mcpu=power8 or similar.  Does it need it anyway?
>> Both xxpermdi and xvcvdpsp are Power7 (ISA 2.06) and the rest is AltiVec?
>> So maybe just powerpc_vsx_ok?
>>
>>> +/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } } */
>>
>> You do not use -mcpu= so you don't need this.
>>
>> Same issues in the next test.  Rest looks good though :-)
>>
>>
>> Segher
>>
>>
> 
> gcc/ChangeLog:
> 
> 2018-06-22  Kelvin Nilsen  <kelvin@gcc.gnu.org>
> 
> 	* config/rs6000/rs6000-c.c (altivec_overloaded_builtins): Change
> 	behavior of vec_pack (vector double, vector double) to match
> 	behavior of vec_float2 (vector double, vector double).
> 
> gcc/testsuite/ChangeLog:
> 
> 2018-06-22  Kelvin Nilsen  <kelvin@gcc.gnu.org>
> 
> 	* gcc.target/powerpc/builtins-3-p8.c (test_pack_float): Remove
> 	this test.
> 	* gcc.target/powerpc/builtins-9.c: New test.
> 	* gcc.target/powerpc/fold-vec-pack-double.c: Modify dg directives
> 	to expect different code generation on big-endian vs.
> 	little-endian targets.
> 
> Index: gcc/config/rs6000/rs6000-c.c
> ===================================================================
> --- gcc/config/rs6000/rs6000-c.c	(revision 261775)
> +++ gcc/config/rs6000/rs6000-c.c	(working copy)
> @@ -2425,7 +2425,7 @@ const struct altivec_builtin_types altivec_overloa
>      RS6000_BTI_unsigned_V4SI, RS6000_BTI_unsigned_V2DI, RS6000_BTI_unsigned_V2DI, 0 },
>    { ALTIVEC_BUILTIN_VEC_PACK, P8V_BUILTIN_VPKUDUM,
>      RS6000_BTI_bool_V4SI, RS6000_BTI_bool_V2DI, RS6000_BTI_bool_V2DI, 0 },
> -  { ALTIVEC_BUILTIN_VEC_PACK, P8V_BUILTIN_VPKUDUM,
> +  { ALTIVEC_BUILTIN_VEC_PACK, P8V_BUILTIN_FLOAT2_V2DF,
>      RS6000_BTI_V4SF, RS6000_BTI_V2DF, RS6000_BTI_V2DF, 0 },
> 
>    { P8V_BUILTIN_VEC_NEG, P8V_BUILTIN_NEG_V16QI,
> Index: gcc/testsuite/gcc.target/powerpc/builtins-3-p8.c
> ===================================================================
> --- gcc/testsuite/gcc.target/powerpc/builtins-3-p8.c	(revision 261775)
> +++ gcc/testsuite/gcc.target/powerpc/builtins-3-p8.c	(working copy)
> @@ -11,12 +11,6 @@ test_eq_long_long (vector bool long long x, vector
>  	return vec_cmpeq (x, y);
>  }
> 
> -vector float
> -test_pack_float (vector double x, vector double y)
> -{
> -  return vec_pack (x, y);
> -}
> -
>  vector unsigned char
>  test_vsi_packs_vusi_vusi (vector unsigned short x,
>                            vector unsigned short y)
> @@ -214,7 +208,6 @@ test_neg_double (vector double x)
>  /* Expected test results:
> 
>       test_eq_long_long                         1 vcmpequd inst
> -     test_pack_float                           1 vpkudum inst
>       test_vsi_packs_vsll_vsll                  1 vpksdss
>       test_vui_packs_vull_vull                  1 vpkudus
>       test_vui_packs_vssi_vssi                  1 vpkshss
> @@ -239,7 +232,6 @@ test_neg_double (vector double x)
>   */
> 
>  /* { dg-final { scan-assembler-times "vcmpequd" 1 } } */
> -/* { dg-final { scan-assembler-times "vpkudum"  1 } } */
>  /* { dg-final { scan-assembler-times "vpksdss"  1 } } */
>  /* { dg-final { scan-assembler-times "vpkudus"  1 } } */  
>  /* { dg-final { scan-assembler-times "vpkuhus"  2 } } */
> Index: gcc/testsuite/gcc.target/powerpc/builtins-9.c
> ===================================================================
> --- gcc/testsuite/gcc.target/powerpc/builtins-9.c	(nonexistent)
> +++ gcc/testsuite/gcc.target/powerpc/builtins-9.c	(working copy)
> @@ -0,0 +1,19 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target powerpc_p8vector_ok } */
> +/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=power8" } } */
> +/* { dg-options "-maltivec -mcpu=power8 -O3" } */
> +
> +#include <altivec.h>
> +
> +vector float
> +test_pack_float (vector double x, vector double y)
> +{
> +  return vec_pack (x, y);
> +}
> +
> +/* { dg-final { scan-assembler-times "vmrgew" 1 { target be } } } */
> +/* { dg-final { scan-assembler-times "vmrgow"  1 { target le } } } */
> +
> +/* { dg-final { scan-assembler-times "xvcvdpsp"  2 } } */
> +/* { dg-final { scan-assembler-times "xxpermdi"  2 } } */
> +
> Index: gcc/testsuite/gcc.target/powerpc/fold-vec-pack-double.c
> ===================================================================
> --- gcc/testsuite/gcc.target/powerpc/fold-vec-pack-double.c	(revision 261775)
> +++ gcc/testsuite/gcc.target/powerpc/fold-vec-pack-double.c	(working copy)
> @@ -3,6 +3,7 @@
> 
>  /* { dg-do compile } */
>  /* { dg-require-effective-target powerpc_p8vector_ok } */
> +/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=power8" } } */
>  /* { dg-options "-mvsx -mpower8-vector -O2" } */
> 
>  #include <altivec.h>
> @@ -15,4 +16,5 @@ test_pack (vector double vd2, vector double vd3)
>    return vec_pack (vd2, vd3);
>  }
> 
> -/* { dg-final { scan-assembler-times "vpkudum" 1 } } */
> +/* { dg-final { scan-assembler-times "vmrgew" 1 { target be } } } */
> +/* { dg-final { scan-assembler-times "vmrgow"  1 { target le } } } */
> 
>

On Fri, Jun 22, 2018 at 05:36:24PM -0500, Kelvin Nilsen wrote:
> After waiting a few days for this newly committed patch to settle, is it ok to backport to gcc 6, gcc 7, and gcc 8?

Sure.  Thanks!


Segher