Re: [PATCH ver 4] rs6000: Add builtins for IEEE 128-bit floating point values

[Carl Love <cel@us.ibm.com>]

On Thu, 2023-06-15 at 14:23 +0800, Kewen.Lin wrote:
> Hi Carl,
> 
> on 2023/6/15 04:37, Carl Love wrote:
> > Kewen, GCC maintainers:
> > 
> > Version 4, added missing cases for new xxexpqp, xsxexpdp and
> > xsxsigqp
> > cases to rs6000_expand_builtin.  Merged the new define_insn
> > definitions
> > with the existing definitions.  Renamed the builtins by removing
> > the
> > __builtin_ prefix from the names.  Fixed the documentation for the
> > builtins.  Updated the test files to check the desired instructions
> > were generated.  Retested patch on Power 10 with no regressions.
> > 
> > Version 3, was able to get the overloaded version of
> > scalar_insert_exp
> > to work and the change to xsxexpqp_f128_<mode> define instruction
> > to
> > work with the suggestions from Kewen.  
> > 
> > Version 2, I have addressed the various comments from Kewen.  I had
> > issues with adding an additional overloaded version of
> > scalar_insert_exp with vector arguments.  The overload
> > infrastructure
> > didn't work with a mix of scalar and vector arguments.  I did
> > rename
> > the __builtin_insertf128_exp to __builtin_vsx_scalar_insert_exp_qp
> > make
> > it similar to the existing builtin.  I also wasn't able to get the
> > suggested merge of xsxexpqp_f128_<mode> with xsxexpqp_<mode> to
> > work so
> > I left the two simpler definitiions.
> > 
> > The patch add three new builtins to extract the significand and
> > exponent of an IEEE float 128-bit value where the builtin argument
> > is a
> > vector.  Additionally, a builtin to insert the exponent into an
> > IEEE
> > float 128-bit vector argument is added.  These builtins were
> > requested
> > since there is no clean and optimal way to transfer between a
> > vector
> > and a scalar IEEE 128 bit value.
> > 
> > The patch has been tested on Power 10 with no regressions.  Please
> > let
> > me know if the patch is acceptable or not.  Thanks.
> 
> I'd suggest you to test this on P9 BE as well to ensure the test case
> to work well on BE too.

Tested on P9 BE.  Updated test cases for the correct expected BE and LE
results.

> 
> >                Carl
> > 
> > 
> > ----------------------------------------
> > rs6000: Add builtins for IEEE 128-bit floating point values
> > 
> > Add support for the following builtins:
> > 
> >  __vector unsigned long long int scalar_extract_exp_to_vec
> > (__ieee128);
> >  __vector unsigned __int128 scalar_extract_sig_to_vec (__ieee128);
> >  __ieee128 scalar_insert_exp (__vector unsigned __int128,
> >  			      __vector unsigned long long);
> > 
> > These builtins were requesed since there is no clean and performant
> > way to
> 
> s/requesed/requested/

Fixed.

> 
> > transfer a value from a vector type and scalar type, despite the
> > fact
> 
> Describe it oppositely?  As the related existing bifs returns scalar
> type,
> the users want them in vector type, so it's "from scalar type to
> vector
> type"?

Updated the description.

> 
> > that they both reside in vector registers.
> 
> the fact is the related hardware insns have vsx registers
> destination.
> 
> > gcc/
> > 	* config/rs6000/rs6000-builtin.cc (rs6000_expand_builtin):
> > 	Rename CCDE_FOR_xsxexpqp_tf to CODE_FOR_xsxexpqp_tf_di.
> > 	Rename CODE_FOR_xsxexpqp_kf to CODE_FOR_xsxexpqp_kf_di.
> > 	(CODE_FOR_xsxexpqp_kf_v2di, CODE_FOR_xsxsigqp_kf_v1ti,
> > 	CODE_FOR_xsiexpqp_kf_v2di	): Add case statements.
> 
> unnecessary tab.

Fixed.

> 
> > 	* config/rs6000/rs6000-buildin.def (__builtin_extractf128_exp,
> > 	 __builtin_extractf128_sig, __builtin_insertf128_exp): Add new
> > 	builtin definitions.
> > 	Rename xsxexpqp_kf, xsxsigqp_kf, xxsiexpqp_kf to xsexpqp_kf_di,
> 
> typo, xxsiexpqp_kf => xsiexpqp_kf
> 
> > 	xsxsigqp_kf_ti, xsiexpqp_kf_di respectively.
> > 	* config/rs6000/rs6000-c.cc
> > (altivec_resolve_overloaded_builtin):
> > 	Add else if for MODE_VECTOR_INT. Update comments.
> 
> May be better with "Update RS6000_OVLD_VEC_VSIE handling for
> MODE_VECTOR_INT
> which is used for newly added overloaded instance"?

Changed.

> 
> > 	* config/rs6000/rs6000-overload.def
> > 	(__builtin_vec_scalar_insert_exp): Add new overload definition
> > with
> > 	vector arguments.
> > 	(scalar_extract_exp_to_vec, scalar_extract_sig_to_vec): New
> > 	odverloaded definitions.
> 
> s/odverloaded/overloaded/

Fixed.

> 
> > 	* config/vsx.md (VSEEQP_DI, VSESQP_TI): New mode iterators.
> > 	(VSEEQP_DI_base): New mode attribute definition.
> > 	Rename xsxexpqp_<mode> to
> > sxexpqp_<IEEE128:mode>_<VSEEQP_DI:mode>.
> > 	Rename xsxsigqp_<mode> to
> > xsxsigqp_<IEEE128:mode>_<VSESQP_TI:mode>.
> > 	Rename xsiexpqp_<mode> to
> > xsiexpqp_<IEEE128:mode>_<VSEEQP_DI:mode>.
> > 	(xsxsigqp_f128_<mode>, xsiexpqpf_f128_<mode>): Add define_insn
> > for
> > 	new builtins.
> > 	* doc/extend.texi (__builtin_extractf128_exp,
> > 	__builtin_extractf128_sig): Add documentation for new builtins.
> > 	(scalar_insert_exp): Add new overloaded builtin definition.
> > 
> > gcc/testsuite/
> > 	* gcc.target/powerpc/bfp/extract-exp-1.c: New test case.
> > 	* gcc.target/powerpc/bfp/extract-sig-1.c: New test case.
> > 	* gcc.target/powerpc/bfp/insert-exp-1.c: New test case.
> 
> May be better just with the existing base name and increasing number:

Renamed test files.

> 
>   extract-exp-1.c => scalar-extract-exp-8.c
>   extract-sig-1.c => scalar-extract-sig-8.c
>   insert-exp-1.c  => scalar-insert-exp-16.c
> 
> > ---
> >  gcc/config/rs6000/rs6000-builtin.cc           | 21 +++--
> >  gcc/config/rs6000/rs6000-builtins.def         | 15 ++-
> >  gcc/config/rs6000/rs6000-c.cc                 | 10 +-
> >  gcc/config/rs6000/rs6000-overload.def         | 10 ++
> >  gcc/config/rs6000/vsx.md                      | 26 +++--
> >  gcc/doc/extend.texi                           | 21 ++++-
> >  .../gcc.target/powerpc/bfp/extract-exp-1.c    | 53 +++++++++++
> >  .../gcc.target/powerpc/bfp/extract-sig-1.c    | 60 ++++++++++++
> >  .../gcc.target/powerpc/bfp/insert-exp-1.c     | 94
> > +++++++++++++++++++
> >  9 files changed, 284 insertions(+), 26 deletions(-)
> >  create mode 100644 gcc/testsuite/gcc.target/powerpc/bfp/extract-
> > exp-1.c
> >  create mode 100644 gcc/testsuite/gcc.target/powerpc/bfp/extract-
> > sig-1.c
> >  create mode 100644 gcc/testsuite/gcc.target/powerpc/bfp/insert-
> > exp-1.c
> > 
> > diff --git a/gcc/config/rs6000/rs6000-builtin.cc
> > b/gcc/config/rs6000/rs6000-builtin.cc
> > index 534698e7d3e..a8f291c6a72 100644
> > --- a/gcc/config/rs6000/rs6000-builtin.cc
> > +++ b/gcc/config/rs6000/rs6000-builtin.cc
> > @@ -3326,17 +3326,26 @@ rs6000_expand_builtin (tree exp, rtx
> > target, rtx /* subtarget */,
> >        case CODE_FOR_fmakf4_odd:
> >  	icode = CODE_FOR_fmatf4_odd;
> >  	break;
> > -      case CODE_FOR_xsxexpqp_kf:
> > -	icode = CODE_FOR_xsxexpqp_tf;
> > +      case CODE_FOR_xsxexpqp_kf_di:
> > +	icode = CODE_FOR_xsxexpqp_tf_di;
> >  	break;
> > -      case CODE_FOR_xsxsigqp_kf:
> > -	icode = CODE_FOR_xsxsigqp_tf;
> > +      case CODE_FOR_xsxexpqp_kf_v2di:
> > +	icode = CODE_FOR_xsxexpqp_tf_v2di;
> > +	break;
> > +      case CODE_FOR_xsxsigqp_kf_ti:
> > +	icode = CODE_FOR_xsxsigqp_tf_ti;
> > +	break;
> > +      case CODE_FOR_xsxsigqp_kf_v1ti:
> > +	icode = CODE_FOR_xsxsigqp_tf_v1ti;
> >  	break;
> >        case CODE_FOR_xststdcnegqp_kf:
> >  	icode = CODE_FOR_xststdcnegqp_tf;
> >  	break;
> > -      case CODE_FOR_xsiexpqp_kf:
> > -	icode = CODE_FOR_xsiexpqp_tf;
> > +      case CODE_FOR_xsiexpqp_kf_di:
> > +	icode = CODE_FOR_xsiexpqp_tf_di;
> > +	break;
> > +      case CODE_FOR_xsiexpqp_kf_v2di:
> > +	icode = CODE_FOR_xsiexpqp_tf_v2di;
> >  	break;
> >        case CODE_FOR_xsiexpqpf_kf:
> >  	icode = CODE_FOR_xsiexpqpf_tf;
> > diff --git a/gcc/config/rs6000/rs6000-builtins.def
> > b/gcc/config/rs6000/rs6000-builtins.def
> > index 638d0bc72ca..6623cb8195d 100644
> > --- a/gcc/config/rs6000/rs6000-builtins.def
> > +++ b/gcc/config/rs6000/rs6000-builtins.def
> > @@ -2901,20 +2901,29 @@
> >    fpmath double __builtin_truncf128_round_to_odd (_Float128);
> >      TRUNCF128_ODD trunckfdf2_odd {}
> >  
> > +  vull __builtin_scalar_extract_exp_to_vec (_Float128);
> > +    EEXPKF xsxexpqp_kf_v2di {}
> 
> Since you have added one entry in overload.def for this, I think
> it's better to name this as
> __builtin_**vsx**_scalar_extract_exp_to_vec
> just like the others.  And sorry for nit-picking, the related bif
> has the id VSEEQP, could we use VSEEQPV for the id here?
> 
> And move this entry just after its related
> __builtin_vsx_scalar_extract_expq.
> 

Renamed and moved as requested.

> > +
> > +  vuq __builtin_scalar_extract_sig_to_vec (_Float128);
> > +    ESIGKF xsxsigqp_kf_v1ti {}
> > +
> 
> Similar comments like above applied for this one.

Also fixed.

> 
> 
> >    const signed long long __builtin_vsx_scalar_extract_expq
> > (_Float128);
> > -    VSEEQP xsxexpqp_kf {}
> > +    VSEEQP xsxexpqp_kf_di {}
> >  
> >    const signed __int128 __builtin_vsx_scalar_extract_sigq
> > (_Float128);
> > -    VSESQP xsxsigqp_kf {}
> > +    VSESQP xsxsigqp_kf_ti {}
> >  
> >    const _Float128 __builtin_vsx_scalar_insert_exp_q (unsigned
> > __int128, \
> >                                                       unsigned long
> > long);
> > -    VSIEQP xsiexpqp_kf {}
> > +    VSIEQP xsiexpqp_kf_di {}
> >  
> >    const _Float128 __builtin_vsx_scalar_insert_exp_qp (_Float128, \
> >                                                        unsigned
> > long long);
> >      VSIEQPF xsiexpqpf_kf {}
> >  
> > +  const _Float128 __builtin_vsx_scalar_insert_exp_vqp (vuq, vull);
> > +    VSIEQPV xsiexpqp_kf_v2di {}
> > +
> >    const signed int __builtin_vsx_scalar_test_data_class_qp
> > (_Float128, \
> >                                                              const
> > int<7>);
> >      VSTDCQP xststdcqp_kf {}
> > diff --git a/gcc/config/rs6000/rs6000-c.cc
> > b/gcc/config/rs6000/rs6000-c.cc
> > index 8555174d36e..11060f697db 100644
> > --- a/gcc/config/rs6000/rs6000-c.cc
> > +++ b/gcc/config/rs6000/rs6000-c.cc
> > @@ -1929,11 +1929,15 @@ altivec_resolve_overloaded_builtin
> > (location_t loc, tree fndecl,
> >  	   128-bit variant of built-in function.  */
> >  	if (GET_MODE_PRECISION (arg1_mode) > 64)
> >  	  {
> > -	    /* If first argument is of float variety, choose variant
> > -	       that expects __ieee128 argument.  Otherwise, expect
> > -	       __int128 argument.  */
> > +	    /* If first argument is of float variety, choose the
> > variant that
> > +	       expects __ieee128 argument.  If the first argument is
> > vector
> > +	       int, choose the variant that expects vector unsigned
> > +	       __int128 argument.  Otherwise, expect scalar __int128
> > argument.
> > +	    */
> >  	    if (GET_MODE_CLASS (arg1_mode) == MODE_FLOAT)
> >  	      instance_code = RS6000_BIF_VSIEQPF;
> > +	    else if (GET_MODE_CLASS (arg1_mode) == MODE_VECTOR_INT)
> > +	      instance_code = RS6000_BIF_VSIEQPV;
> >  	    else
> >  	      instance_code = RS6000_BIF_VSIEQP;
> >  	  }
> > diff --git a/gcc/config/rs6000/rs6000-overload.def
> > b/gcc/config/rs6000/rs6000-overload.def
> > index c582490c084..1c503a4b3d3 100644
> > --- a/gcc/config/rs6000/rs6000-overload.def
> > +++ b/gcc/config/rs6000/rs6000-overload.def
> > @@ -4515,6 +4515,16 @@
> >      VSIEQP
> >    _Float128 __builtin_vec_scalar_insert_exp (_Float128, unsigned
> > long long);
> >      VSIEQPF
> > +  _Float128 __builtin_vec_scalar_insert_exp (vuq, vull);
> > +    VSIEQPV
> > +
> > +[VEC_VSEEV, scalar_extract_exp_to_vec,
> > __builtin_scalar_extract_exp_to_vector]
> 
> Thanks for adding this, to make it more consistent with the existing
> others,
> I think the latter name would be better with
> __builtin_**vec**_scalar_extract_exp_to_vector.

Renamed. 

> 
> > +  vull __builtin_scalar_extract_exp_to_vector (_Float128);
> > +    EEXPKF
> 
> Need to update this bif id accordingly if the one in rs6000-
> buildin.def changes.

Changed. 

> 
> > +
> > +[VEC_VSESV, scalar_extract_sig_to_vec,
> > __builtin_scalar_extract_sig_to_vector]
> > +  vuq __builtin_scalar_extract_sig_to_vector (_Float128);
> > +    ESIGKF
> >  
> 
> Ditto.
> 

Fixed.


> >  [VEC_VSTDC, scalar_test_data_class,
> > __builtin_vec_scalar_test_data_class]
> >    unsigned int __builtin_vec_scalar_test_data_class (float, const
> > int);
> > diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md
> > index 7d845df5c2d..bb19b5ef170 100644
> > --- a/gcc/config/rs6000/vsx.md
> > +++ b/gcc/config/rs6000/vsx.md
> > @@ -396,6 +396,10 @@
> >  			   V4SF
> >  			   V2DF
> >  			   V2DI])
> > +(define_mode_iterator VSEEQP_DI  [V2DI DI])
> 
> Since you hoist this to one centralized place, I guess it may be
> better to name
> it more general, like: V2DI_DI.

Changed name as suggested.

> 
> > +(define_mode_iterator VSESQP_TI  [V1TI TI])
> 
> VEC_TI matches the requirement here.

Missed that definition.  Replaced VSESQP_TI with VEC_TI.

> 
> > +(define_mode_attr VSEEQP_DI_base [(V2DI "V1TI")
> > +				  (DI   "TI")])
> 
> As above, may be better with name DI_to_TI.

Renamed.

> 
> >  
> >  (define_mode_attr VM3_char [(V2DI "d")
> >  			   (V4SI "w")
> > @@ -5008,9 +5012,10 @@
> >  ;; ISA 3.0 Binary Floating-Point Support
> >  
> >  ;; VSX Scalar Extract Exponent Quad-Precision
> > -(define_insn "xsxexpqp_<mode>"
> > -  [(set (match_operand:DI 0 "altivec_register_operand" "=v")
> > -	(unspec:DI [(match_operand:IEEE128 1 "altivec_register_operand"
> > "v")]
> > +(define_insn "xsxexpqp_<IEEE128:mode>_<VSEEQP_DI:mode>"
> > +  [(set (match_operand:VSEEQP_DI 0 "altivec_register_operand"
> > "=v")
> > +	(unspec:VSEEQP_DI
> > +	  [(match_operand:IEEE128 1 "altivec_register_operand" "v")]
> >  	 UNSPEC_VSX_SXEXPDP))]
> >    "TARGET_P9_VECTOR"
> >    "xsxexpqp %0,%1"
> > @@ -5026,9 +5031,10 @@
> >    [(set_attr "type" "integer")])
> >  
> >  ;; VSX Scalar Extract Significand Quad-Precision
> > -(define_insn "xsxsigqp_<mode>"
> > -  [(set (match_operand:TI 0 "altivec_register_operand" "=v")
> > -	(unspec:TI [(match_operand:IEEE128 1 "altivec_register_operand"
> > "v")]
> > +(define_insn "xsxsigqp_<IEEE128:mode>_<VSESQP_TI:mode>"
> > +  [(set (match_operand:VSESQP_TI 0 "altivec_register_operand"
> > "=v")
> > +	(unspec:VSESQP_TI [(match_operand:IEEE128 1
> > +			    "altivec_register_operand" "v")]
> >  	 UNSPEC_VSX_SXSIG))]
> >    "TARGET_P9_VECTOR"
> >    "xsxsigqp %0,%1"
> > @@ -5055,10 +5061,12 @@
> >    [(set_attr "type" "vecmove")])
> >  
> >  ;; VSX Scalar Insert Exponent Quad-Precision
> > -(define_insn "xsiexpqp_<mode>"
> > +(define_insn "xsiexpqp_<IEEE128:mode>_<VSEEQP_DI:mode>"
> >    [(set (match_operand:IEEE128 0 "altivec_register_operand" "=v")
> > -	(unspec:IEEE128 [(match_operand:TI 1 "altivec_register_operand"
> > "v")
> > -			 (match_operand:DI 2 "altivec_register_operand"
> > "v")]
> > +	(unspec:IEEE128 [(match_operand:<VSEEQP_DI_base> 1
> > +			  "altivec_register_operand" "v")
> > +			 (match_operand:VSEEQP_DI 2
> > +			  "altivec_register_operand" "v")]
> >  	 UNSPEC_VSX_SIEXPQP))]
> >    "TARGET_P9_VECTOR"
> >    "xsiexpqp %0,%1,%2"
> > diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> > index e426a2eb7d8..6da7ae9d94c 100644
> > --- a/gcc/doc/extend.texi
> > +++ b/gcc/doc/extend.texi
> > @@ -19724,6 +19724,10 @@ double scalar_insert_exp (double
> > significand, unsigned long long int exponent);
> >  ieee_128 scalar_insert_exp (unsigned __int128 significand,
> >                              unsigned long long int exponent);
> >  ieee_128 scalar_insert_exp (ieee_128 significand, unsigned long
> > long int exponent);
> > +vector ieee_128 scalar_insert_exp (vector unsigned __int128,
> > +                                   vector unsigned long long);
> > +vector unsigned long long scalar_extract_exp_to_vec (ieee_128);
> > +vector unsigned __int128  scalar_extract_sig_to_vec (ieee_128);
> >  
> >  int scalar_cmp_exp_gt (double arg1, double arg2);
> >  int scalar_cmp_exp_lt (double arg1, double arg2);
> > @@ -19771,11 +19775,18 @@ of the result are composed of the least
> > significant 11 bits of the
> >  
> >  When supplied with a 128-bit first argument, the
> >  @code{scalar_insert_exp} built-in function returns a quad-
> > precision
> > -ieee floating point value.  The sign bit of the result is copied
> > from
> > -the most significant bit of the @code{significand} argument.
> > -The significand and exponent components of the result are composed
> > of
> > -the least significant 15 bits of the @code{exponent} argument and
> > the
> > -least significant 112 bits of the @code{significand} argument
> > respectively.
> > +ieee floating point value if the two arguments were scalar.  If
> > the two
> > +arguments are vectors, the retun value is a vector ieee floating
> > point value.
> 
> s/retun/return/

fixed.

> 
> > +The sign bit of the result is copied from the most significant bit
> > of the
> > +@code{significand} argument.  The significand and exponent
> > components of the
> > +result are composed of the least significant 15 bits of the
> > @code{exponent}
> > +argument and the least significant 112 bits of the
> > @code{significand} argument
> > +respectively.
> 
> The @code{exponent} now is a vector and has two elements, you should
> note
> which element matters here, element zero on big-endian while element
> one
> on little-endian will be used.
> 
> Since this document quoted both @code{significand} and
> @code{significand},
> the above prototype should have the argument name as well.  :)
> 
> +vector ieee_128 scalar_insert_exp (vector unsigned __int128
> significand,
> +                                   vector unsigned long long
> exponent);
> 

Ah, didn't notice that I missed adding the argument names.  Fixed.

               Carl