Re: [PATCH, rs6000] Add builtin support for vec_insert4b, vec_extract4b

[Segher Boessenkool <segher@kernel.crashing.org>]

Hi  Carl,

On Thu, Feb 01, 2018 at 11:31:55AM -0800, Carl Love wrote:
> The following patch contains fixes for the builtins to add and extract
> a word from a char vector.  The existing names for the builtins that do
> this are not consistent with the ABI.  Additionally, the supported
> arguments are not all consistent with the ABI.  This patch fixes the
> support for the insert and extract word builtins so they are consistent
> with the "64-Bit ELF V2 ABI Specification".

The patch is hard to review because it does multiple things at once.
Would have been easier if you first add the new builtins and then delete
the old one.  But I'll try :-)

> Let me know if the patch looks OK or not.  Let me know if you want to
> include it in stage 4 or wait for the next release.  Thanks.

It should also go to GCC 7, right?

> 2018-01-31  Carl Love  <cel@us.ibm.com>
> 
> 	* config/rs6000/altivec.h: Change vec_vextract4b to	vec_extract4b

You have a tab before vec_extract4b there.

> 	and vec_vinsert4b to vec_insert4b.
> 	* config/rs6000/rs6000-builtin.def: Remove VEXTRACT4B, VINSERT4B
> 	and VINSERT4B_DI definitions. Add INSERT4B and	EXTRACT4B definitions.

Two spaces after dot (numerous times, this is the first one).  You have
a tab before EXTRACT4B.

	* config/rs6000/rs6000-builtin.def (VEXTRACT4B, VINSERT4B): Delete.
	(EXTRACT4B, INSERT4B): New.

(And similar to all below).

> 	*doc/extend.texi: Remove documentation for the vec_vextract4b. Fix
> 	documentation for vec_insert4b.  Add documentation for vec_extract4b.

Space after *.

> 2018-01-31  Carl Love  <cel@us.ibm.com>
> 	* gcc.target/powerpc/builtins-7-p9-runnable.c: New runnable test file.
> 	* gcc.target/powerpc/p9_vinsert4b-1.c: Remove file.
> 	* gcc.target/powerpc/p9_vinsert4b-2.c: Remove file.

The files are called p9-vinsert* in fact.  Is all what they tested now in
builtins-7-p9-runnable.c ?

> +(define_insn "extract4b"
> +  [(set (match_operand:V2DI 0 "vsx_register_operand")
> +	(unspec:V2DI [(match_operand:V16QI 1 "vsx_register_operand" "wa")
> +		      (match_operand:QI 2 "const_0_to_12_operand" "n")]
> +		     UNSPEC_XXEXTRACTUW))]
>    "TARGET_P9_VECTOR"
>  {
>    if (!VECTOR_ELT_ORDER_BIG)
>      operands[2] = GEN_INT (12 - INTVAL (operands[2]));
>  
> +  return "xxextractuw %0,%1,%2";
> +})

You need %x0 and %x1 here I think?

> -vector signed char vec_insert4b (vector int, vector signed char, const int);
> +vector unsigned char vec_insert4b (vector int, vector unsigned char,
> +                                   const signed int);

Maybe just write "int" instead of "signed int"?

> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/builtins-7-p9-runnable.c
> @@ -0,0 +1,168 @@
> +/* { dg-do run { target { powerpc64*-*-* && p9vector_hw } } } */

As always: why powerpc64* instead of powerpc*?


Segher