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
