Re: [PATCH 2/2 ver 5] rs6000, fix vec_replace_unaligned built-in arguments

["Kewen.Lin" <linkw@linux.ibm.com>]

Hi Carl,

on 2023/7/22 07:38, Carl Love wrote:
> GCC maintainers:
> 
> Version 5, Fixed patch description, the first argument should be of
> type vector.  Fixed comment in vsx.md to say "Vector and scalar
> extract_elt iterator/attr ....".  Removed a few of the changes in
> version 4.  Specifically, reverted the names of REPLACE_ELT_V_sh back
> to REPLACE_ELT_sh and REPLACE_ELT_V_max back to REPLACE_ELT_V_max. 
> Combined the REPLACE_ELT_char and REPLACE_ELT_V_char mode attributes
> into REPLACE_ELT_char.  Put the "dg-do link" directive back into the
> vec-replace-word-runnable_1.c test file.  The patch was tested with the
> updated patch 1 in the series on Power 8 LE/BE, Power 9 LE/BE and Power
> 10 with no regressions.
> 

snip...

> --------------------------------
> rs6000, fix vec_replace_unaligned built-in arguments
> 
> The first argument of the vec_replace_unaligned built-in should always be
> of type vector unsigned char, as specified in gcc/doc/extend.texi.
> 
> This patch fixes the builtin definitions and updates the test cases to use
> the correct arguments.  The original test file is renamed and a second test
> file is added for a new test case.
> 
> gcc/ChangeLog:
> 	* config/rs6000/rs6000-builtins.def: Rename
> 	__builtin_altivec_vreplace_un_uv2di as __builtin_altivec_vreplace_un_udi
> 	__builtin_altivec_vreplace_un_uv4si as __builtin_altivec_vreplace_un_usi
> 	__builtin_altivec_vreplace_un_v2df as __builtin_altivec_vreplace_un_df
> 	__builtin_altivec_vreplace_un_v2di as __builtin_altivec_vreplace_un_di
> 	__builtin_altivec_vreplace_un_v4sf as __builtin_altivec_vreplace_un_sf
> 	__builtin_altivec_vreplace_un_v4si as __builtin_altivec_vreplace_un_si.
> 	Rename VREPLACE_UN_UV2DI as VREPLACE_UN_UDI, VREPLACE_UN_UV4SI as
> 	VREPLACE_UN_USI, VREPLACE_UN_V2DF as VREPLACE_UN_DF,
> 	VREPLACE_UN_V2DI as VREPLACE_UN_DI, VREPLACE_UN_V4SF as
> 	VREPLACE_UN_SF, VREPLACE_UN_V4SI as VREPLACE_UN_SI.
> 	Rename vreplace_un_v2di as vreplace_un_di, vreplace_un_v4si as
> 	vreplace_un_si, vreplace_un_v2df as vreplace_un_df,
> 	vreplace_un_v2di as vreplace_un_di, vreplace_un_v4sf as
> 	vreplace_un_sf, vreplace_un_v4si as vreplace_un_si.
> 	* config/rs6000/rs6000-c.cc (find_instance): Add case
> 	RS6000_OVLD_VEC_REPLACE_UN.
> 	* config/rs6000/rs6000-overload.def (__builtin_vec_replace_un):
> 	Fix first argument type.  Rename VREPLACE_UN_UV4SI as
> 	VREPLACE_UN_USI, VREPLACE_UN_V4SI as VREPLACE_UN_SI,
> 	VREPLACE_UN_UV2DI as VREPLACE_UN_UDI, VREPLACE_UN_V2DI as
> 	VREPLACE_UN_DI, VREPLACE_UN_V4SF as VREPLACE_UN_SF,
> 	VREPLACE_UN_V2DF as VREPLACE_UN_DF.
> 	* config/rs6000/vsx.md (REPLACE_ELT): Renamed the mode_iterator

Nit: s/Renamed/Rename/

> 	REPLACE_ELT_V for vector modes.
> 	(REPLACE_ELT): New scalar mode iterator.
> 	(REPLACE_ELT_char): Add scalar attributes.
> 	(vreplace_un_<mode>): Change iterator and mode attribute.
> 
> gcc/testsuite/ChangeLog:
> 	* gcc.target/powerpc/vec-replace-word-runnable.c: Renamed
> 	vec-replace-word-runnable_1.c.

Ditto.

> 	* gcc.target/powerpc/vec-replace-word-runnable_1.c
> 	(dg-options): add -flax-vector-conversions.
> 	(vec_replace_unaligned) Fix first argument type.
> 	(vresult_uchar): Fix expected results.
> 	(vec_replace_unaligned): Update for loop to check uchar results.
> 	Remove extra spaces in if statements. Insert missing spaces in
> 	for statements.
> 	* gcc.target/powerpc/vec-replace-word-runnable_2.c: New test file.
> ---

[snip...]

>  
>  [VEC_REVB, vec_revb, __builtin_vec_revb]
>    vss __builtin_vec_revb (vss);
> diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md
> index 0c269e4e8d9..7a4cf492ea5 100644
> --- a/gcc/config/rs6000/vsx.md
> +++ b/gcc/config/rs6000/vsx.md
> @@ -380,10 +380,13 @@ (define_int_attr xvcvbf16       [(UNSPEC_VSX_XVCVSPBF16 "xvcvspbf16")
>  ;; Like VI, defined in vector.md, but add ISA 2.07 integer vector ops
>  (define_mode_iterator VI2 [V4SI V8HI V16QI V2DI])
>  
> -;; Vector extract_elt iterator/attr for 32-bit and 64-bit elements
> -(define_mode_iterator REPLACE_ELT [V4SI V4SF V2DI V2DF])
> +;; Vector and scalar extract_elt iterator/attr for 32-bit and 64-bit elements

Nit: Since you touched this comment line, extract_elt is wrong before.
Maybe s/extract_elt/vector replace/?

> +(define_mode_iterator REPLACE_ELT_V [V4SI V4SF V2DI V2DF])
> +(define_mode_iterator REPLACE_ELT [SI SF DI DF])
>  (define_mode_attr REPLACE_ELT_char [(V4SI "w") (V4SF "w")
> -				    (V2DI  "d") (V2DF "d")])
> +				    (V2DI "d") (V2DF "d")
> +				    (SI "w") (SF "w")
> +				    (DI "d") (DF "d")])
>  (define_mode_attr REPLACE_ELT_sh [(V4SI "2") (V4SF "2")
>  				  (V2DI  "3") (V2DF "3")])
>  (define_mode_attr REPLACE_ELT_max [(V4SI "12") (V4SF "12")
> @@ -4183,11 +4186,11 @@ (define_insn "vinsertgr_internal_<mode>"
>   [(set_attr "type" "vecsimple")])
>  
>  (define_expand "vreplace_elt_<mode>"
> -  [(set (match_operand:REPLACE_ELT 0 "register_operand")
> -  (unspec:REPLACE_ELT [(match_operand:REPLACE_ELT 1 "register_operand")
> -		       (match_operand:<VEC_base> 2 "register_operand")
> -		       (match_operand:QI 3 "const_0_to_3_operand")]
> -		      UNSPEC_REPLACE_ELT))]
> +  [(set (match_operand:REPLACE_ELT_V 0 "register_operand")
> +  (unspec:REPLACE_ELT_V [(match_operand:REPLACE_ELT_V 1 "register_operand")
> +			 (match_operand:<VEC_base> 2 "register_operand")
> +			 (match_operand:QI 3 "const_0_to_3_operand")]
> +			UNSPEC_REPLACE_ELT))]
>   "TARGET_POWER10"
>  {
>     int index;
> @@ -4207,19 +4210,19 @@ (define_expand "vreplace_elt_<mode>"
>  [(set_attr "type" "vecsimple")])
>  
>  (define_insn "vreplace_elt_<mode>_inst"
> - [(set (match_operand:REPLACE_ELT 0 "register_operand" "=v")
> -  (unspec:REPLACE_ELT [(match_operand:REPLACE_ELT 1 "register_operand" "0")
> -		       (match_operand:<VEC_base> 2 "register_operand" "r")
> -		       (match_operand:QI 3 "const_0_to_12_operand" "n")]
> -		      UNSPEC_REPLACE_ELT))]
> + [(set (match_operand:REPLACE_ELT_V 0 "register_operand" "=v")
> +  (unspec:REPLACE_ELT_V [(match_operand:REPLACE_ELT_V 1 "register_operand" "0")
> +			 (match_operand:<VEC_base> 2 "register_operand" "r")
> +			 (match_operand:QI 3 "const_0_to_12_operand" "n")]
> +			UNSPEC_REPLACE_ELT))]
>   "TARGET_POWER10"
>   "vins<REPLACE_ELT_char> %0,%2,%3"
>   [(set_attr "type" "vecsimple")])
>  
>  (define_insn "vreplace_un_<mode>"
>   [(set (match_operand:V16QI 0 "register_operand" "=v")
> -  (unspec:V16QI [(match_operand:REPLACE_ELT 1 "register_operand" "0")
> -                 (match_operand:<VEC_base> 2 "register_operand" "r")
> +  (unspec:V16QI [(match_operand:V16QI 1 "register_operand" "0")
> +		 (match_operand:REPLACE_ELT 2 "register_operand" "r")
>  		 (match_operand:QI 3 "const_0_to_12_operand" "n")]
>  		UNSPEC_REPLACE_UN))]
>   "TARGET_POWER10"

[snip...]

>  }
>  
>  /* { dg-final { scan-assembler-times {\mvinsw\M} 6 } } */
>  /* { dg-final { scan-assembler-times {\mvinsd\M} 6 } } */
> -
> -
> diff --git a/gcc/testsuite/gcc.target/powerpc/vec-replace-word-runnable_2.c b/gcc/testsuite/gcc.target/powerpc/vec-replace-word-runnable_2.c
> new file mode 100644
> index 00000000000..bf0952c029f
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/vec-replace-word-runnable_2.c
> @@ -0,0 +1,50 @@
> +/* { dg-do run { target { power10_hw } } } */
> +/* { dg-require-effective-target power10_ok } */

Nit: I think power10_ok isn't needed, if power10_ok fails, power10_hw would never succeed.

This patch is okay for trunk with some nits above fixed.  Thanks!


BR,
Kewen