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

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

Kewen:

On Mon, 2023-06-19 at 14:08 +0800, Kewen.Lin wrote:
> > 

<snip>

> Hi Carl,
> 
> on 2023/6/17 01:57, Carl Love wrote:
> > 	overloaded instance. Update comments.
> > 	* 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
> > 	overloaded definitions.
> > 	* config/vsx.md (V2DI_DI): New mode iterator.
> 
> Missing an entry for DI_to_TI.

Opps, missed that.  Sorry, fixed.

> > 

<snip>

> 
> >  
> >    const signed long long __builtin_vsx_scalar_extract_expq
> > (_Float128);
> > -    VSEEQP xsxexpqp_kf {}
> > +    VSEEQP xsxexpqp_kf_di {}
> > +
> > +  vull __builtin_vsx_scalar_extract_exp_to_vec (_Float128);
> > +    VSEEXPKF xsxexpqp_kf_v2di {}
> 
> As I pointed out previously, the related id is VSEEQP, since both of
> them

Oops, I guess I forgot to change that.  Sorry.

> have kf in their names, having KF in its id doesn't look good IMHO.
> How about VSEEQPV instead of VSEEXPKF?  It's also consistent with
> what
> we use for VSIEQP.

Yup, makes sense, changed to VSEEQPV.
> 
> >  
> >    const signed __int128 __builtin_vsx_scalar_extract_sigq
> > (_Float128);
> > -    VSESQP xsxsigqp_kf {}
> > +    VSESQP xsxsigqp_kf_ti {}
> > +
> > +  vuq __builtin_vsx_scalar_extract_sig_to_vec (_Float128);
> > +    VSESIGKF xsxsigqp_kf_v1ti {}
> 
> Similar to the above, s/VSESIGKF/VSESQPV/
 
Changed to VSESQPV.
> 
> >  
> >    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..05a5ca6a04d 100644
> > --- a/gcc/config/rs6000/rs6000-overload.def
> > +++ b/gcc/config/rs6000/rs6000-overload.def
> > @@ -4515,6 +4515,18 @@
> >      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_vec_scalar_extract_exp_to_vector]
> > +  vull __builtin_vec_scalar_extract_exp_to_vector (_Float128);
> > +    VSEEXPKF
> > +
> 
> Need to update if the above changes.

changed 
> 
> > +[VEC_VSESV, scalar_extract_sig_to_vec, \
> > +            __builtin_vec_scalar_extract_sig_to_vector]
> > +  vuq __builtin_vec_scalar_extract_sig_to_vector (_Float128);
> > +    VSESIGKF
> >  
> 
> Ditto.

changed

> 

<snip>

> > 
> > diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-
> > exp-8.c b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-
> > 8.c
> > new file mode 100644
> > index 00000000000..e24e09012d9
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-8.c
> > @@ -0,0 +1,58 @@
> > +/* { dg-do run { target { powerpc*-*-* } } } */
> > +/* { dg-require-effective-target lp64 } */
> > +/* { dg-require-effective-target p9vector_hw } */
> > +/* { dg-options "-mdejagnu-cpu=power9 -save-temps" } */
> > +
> > +#include <altivec.h>
> > +#include <stdlib.h>
> > +
> > +#if DEBUG
> > +#include <stdio.h>
> > +#endif
> > +
> > +vector unsigned long long int
> > +get_exponents (__ieee128 *p)
> > +{
> > +  __ieee128 source = *p;
> > +
> > +  return scalar_extract_exp_to_vec (source);
> > +}
> > +
> > +int
> > +main ()
> > +{
> > +  vector unsigned long long int result, exp_result;
> > +  union conv128_t
> > +   {
> > +     __ieee128 val_ieee128;
> > +     __int128  val_int128;
> > +  } source;
> > +  
> > +#ifdef _BIG_ENDIAN
> > +  exp_result[1] = 0x0ULL;
> > +  exp_result[0] = 0x1234ULL;
> > +#else
> > +  exp_result[0] = 0x0ULL;
> > +  exp_result[1] = 0x1234ULL;
> > +#endif
> > +  source.val_int128 = 0x923456789ABCDEF0ULL;
> > +  source.val_int128 = (source.val_int128 << 64) |
> > 0x123456789ABCDEFULL;
> > +
> > +  result = get_exponents (&source.val_ieee128);
> > +
> > +  if ((result[0] != exp_result[0]) || (result[1] !=
> > exp_result[1]))
> > +#if DEBUG
> > +    {
> > +      printf("result[0] = 0x%llx; exp_result[0] = 0x%llx\n",
> > +	     result[0], exp_result[0]);
> > +      printf("result[1] = 0x%llx; exp_result[1] = 0x%llx\n",
> > +	     result[1], exp_result[1]);
> > +    }
> > +#else
> > +    abort();
> > +#endif
> > +  return 0;
> > +}
> > +
> > +/* check that the expected extract exponent instruction is
> > generated. */
> 
> Nit nit: it's test case, so nobody cares it, but it's better to use
> "Check"
> instead of "check", if you don't mind, could you update this along
> with the
> other updates?  (and for the other two cases)

Fixed in all three test program files.
            
                  Carl