Re: [PATCH v5, rs6000] Change mode and insn condition for VSX scalar extract/insert instructions

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

Hi!

On Fri, Dec 02, 2022 at 03:03:46PM +0800, HAO CHEN GUI wrote:
>   For scalar extract/insert instructions, exponent field can be stored in a
> 32-bit register. So this patch changes the mode of exponent field from DI to
> SI so that these instructions can be generated in a 32-bit environment. Also
> it removes TARGET_64BIT check for these instructions.

>   This patch also changes prototypes and catagories of relevant built-ins and
> effective target checks of test cases.

You can not change the types of builtins, unless they were completely
broken before or something serious like that.  Existing user code should
keep doing the same as it did before.  So any change like this needs a
lot more justification.

> --- a/gcc/config/rs6000/rs6000-builtins.def
> +++ b/gcc/config/rs6000/rs6000-builtins.def
> @@ -2833,6 +2833,11 @@
>    const signed int __builtin_dtstsfi_ov_td (const int<6>, _Decimal128);
>      TSTSFI_OV_TD dfptstsfi_unordered_td {}
> 
> +  const unsigned int __builtin_vsx_scalar_extract_exp (double);
> +    VSEEDP xsxexpdp {}
> +
> +  const double __builtin_vsx_scalar_insert_exp_dp (double, unsigned int);
> +    VSIEDPF xsiexpdpf {}

vec_insert_exp etc. are externally documented (in the PVIPR), these
(internal) builtins should behave similarly.

You can change the *internal* stuff fine of course.

> --- a/gcc/config/rs6000/rs6000-overload.def
> +++ b/gcc/config/rs6000/rs6000-overload.def
> @@ -4507,9 +4507,9 @@
>      VSESQP
> 
>  [VEC_VSIE, scalar_insert_exp, __builtin_vec_scalar_insert_exp]
> -  double __builtin_vec_scalar_insert_exp (unsigned long long, unsigned long long);
> +  double __builtin_vec_scalar_insert_exp (unsigned long long, unsigned int);
>      VSIEDP
> -  double __builtin_vec_scalar_insert_exp (double, unsigned long long);
> +  double __builtin_vec_scalar_insert_exp (double, unsigned int);
>      VSIEDPF

NAK on this as well.

>  ;; VSX Scalar Extract Exponent Double-Precision
>  (define_insn "xsxexpdp"
> -  [(set (match_operand:DI 0 "register_operand" "=r")
> -	(unspec:DI [(match_operand:DF 1 "vsx_register_operand" "wa")]
> +  [(set (match_operand:SI 0 "register_operand" "=r")
> +	(unspec:SI [(match_operand:DF 1 "vsx_register_operand" "wa")]
>  	 UNSPEC_VSX_SXEXPDP))]
> -  "TARGET_P9_VECTOR && TARGET_64BIT"
> +  "TARGET_P9_VECTOR"
>    "xsxexpdp %0,%x1"
>    [(set_attr "type" "integer")])

You need to keep a version that returns DImode (where it did before,
64-bit).  But you can use :GPR perhaps?  That also allows :SI on 64-bit,
so things can be optimised to that where it makes sense.

And :SI on 32-bit is just new, no problems there :-)

> @@ -5116,7 +5116,7 @@ (define_insn "xsxsigdp"
>    [(set (match_operand:DI 0 "register_operand" "=r")
>  	(unspec:DI [(match_operand:DF 1 "vsx_register_operand" "wa")]
>  	 UNSPEC_VSX_SXSIG))]
> -  "TARGET_P9_VECTOR && TARGET_64BIT"
> +  "TARGET_P9_VECTOR && TARGET_POWERPC64"

That is a good fix, separate from the rest though, so could you do it
first please?


Segher