Re: [PATCH 4/13] rs6000, extend the current vec_{un,}signed{e,o} built-ins

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

Kewen:

On 5/14/24 00:53, Kewen.Lin wrote:
> Hi,
> 
> on 2024/4/20 05:17, Carl Love wrote:
>> rs6000, extend the current vec_{un,}signed{e,o} built-ins
>>
>> The built-ins __builtin_vsx_xvcvspsxds and __builtin_vsx_xvcvspuxds
>> convert a vector of floats to signed/unsigned long long ints.  Extend the
>> existing vec_{un,}signed{e,o} built-ins to handle the argument
>> vector of floats to return the even/odd signed/unsigned integers.
>>
>> Add testcases and update documentation.
>>
>> gcc/ChangeLog:
>>         * config/rs6000/rs6000-builtins.def (__builtin_vsx_xvcvspsxds_low,
>>         __builtin_vsx_xvcvspuxds_low): New built-in definitions.
>>         * config/rs6000/rs6000-overload.def (vec_signede, vec_signedo):
>>         Add new overloaded specifications.
>>         * config/rs6000/vsx.md (vsx_xvcvsp<su>xds_low): New define_expand.
>>         * doc/extend.texi (vec_signedo, vec_signede): Add documentation.
>>
>> gcc/testsuite/ChangeLog:
>>         * gcc.target/powerpc/builtins-3-runnable: New tests for the added
>>         overloaded built-ins.
> 
> This part is missing, there are no test case changes in this patch.

Yes, the new tests are missing.  Not sure what happened to them.  Fixed.

> 
>> ---
>>  gcc/config/rs6000/rs6000-builtins.def |  6 ++++++
>>  gcc/config/rs6000/rs6000-overload.def |  8 ++++++++
>>  gcc/config/rs6000/vsx.md              | 23 +++++++++++++++++++++++
>>  gcc/doc/extend.texi                   | 13 +++++++++++++
>>  4 files changed, 50 insertions(+)
>>
>> diff --git a/gcc/config/rs6000/rs6000-builtins.def b/gcc/config/rs6000/rs6000-builtins.def
>> index bf9a0ae22fc..5b7237a2327 100644
>> --- a/gcc/config/rs6000/rs6000-builtins.def
>> +++ b/gcc/config/rs6000/rs6000-builtins.def
>> @@ -1709,9 +1709,15 @@
>>    const vsll __builtin_vsx_xvcvspsxds (vf);
>>      XVCVSPSXDS vsx_xvcvspsxds {}
>>  
>> +  const vsll __builtin_vsx_xvcvspsxds_low (vf);
>> +    XVCVSPSXDSO vsx_xvcvspsxds_low {}
>> +
>>    const vsll __builtin_vsx_xvcvspuxds (vf);
>>      XVCVSPUXDS vsx_xvcvspuxds {}
> 
> This existing should return with type vull, ...

Fixed.

> 
>>  
>> +  const vsll __builtin_vsx_xvcvspuxds_low (vf);
>> +    XVCVSPUXDSO vsx_xvcvspuxds_low {}
> 
> ... so this copied one should be vull too.

Fixed.

> 
> As the existing instances for vec_signed and vec_unsigned are with
> names like VEC_V{UN,}SIGNED{O,E}_V2DF, I prefer these are updated
> with similar style, maybe something like:
> 
> VEC_V{UN,}SIGNED{E,O}_V4SF v{un,}signed{e,o}_v4sf

Yes, sounds reasonable.  Changed XVCVSPUXDS -> VEC_VUNSIGNEDE_V4SF
                                 XVCVSPUXDSO -> VEC_VUNSIGNEDO_V4SF
				 XVCVSPSXDS  -> VEC_VSIGNEDE_V4SF
				 XVCVSPSXDSO  -> VEC_VSIGNEDO_V4SF

NEED TO ADDRESS RESPONSE TO QUESTION I ASKED.

> 
>>    const vsi __builtin_vsx_xvcvspuxws (vf);
>>      XVCVSPUXWS vsx_fixuns_truncv4sfv4si2 {}
>>  > diff --git a/gcc/config/rs6000/rs6000-overload.def b/gcc/config/rs6000/rs6000-overload.def
>> index 84bd9ae6554..68501c05289 100644
>> --- a/gcc/config/rs6000/rs6000-overload.def
>> +++ b/gcc/config/rs6000/rs6000-overload.def
>> @@ -3307,10 +3307,14 @@
>>  [VEC_SIGNEDE, vec_signede, __builtin_vec_vsignede]
>>    vsi __builtin_vec_vsignede (vd);
>>      VEC_VSIGNEDE_V2DF
>> +  vsll __builtin_vec_vsignede (vf);
>> +    XVCVSPSXDS
>>  
>>  [VEC_SIGNEDO, vec_signedo, __builtin_vec_vsignedo]
>>    vsi __builtin_vec_vsignedo (vd);
>>      VEC_VSIGNEDO_V2DF
>> +  vsll __builtin_vec_vsignedo (vf);
>> +    XVCVSPSXDSO
>>  
>>  [VEC_SIGNEXTI, vec_signexti, __builtin_vec_signexti]
>>    vsi __builtin_vec_signexti (vsc);
>> @@ -4433,10 +4437,14 @@
>>  [VEC_UNSIGNEDE, vec_unsignede, __builtin_vec_vunsignede]
>>    vui __builtin_vec_vunsignede (vd);
>>      VEC_VUNSIGNEDE_V2DF
>> +  vull __builtin_vec_vunsignede (vf);
>> +    XVCVSPUXDS
>>  
>>  [VEC_UNSIGNEDO, vec_unsignedo, __builtin_vec_vunsignedo]
>>    vui __builtin_vec_vunsignedo (vd);
>>      VEC_VUNSIGNEDO_V2DF
>> +  vull __builtin_vec_vunsignedo (vf);
>> +    XVCVSPUXDSO
>>  
> As above, the name can be tweaked.

Fixed.

> 
>>  [VEC_VEE, vec_extract_exp, __builtin_vec_extract_exp]
>>    vui __builtin_vec_extract_exp (vf);
>> diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md
>> index f135fa079bd..3d39ae7995f 100644
>> --- a/gcc/config/rs6000/vsx.md
>> +++ b/gcc/config/rs6000/vsx.md
>> @@ -2704,6 +2704,29 @@
>>    DONE;
>>  })
>>  
>> +;; Convert low vector elements of 32-bit floating point numbers to vector of
>> +;; 64-bit signed/unsigned integers.
>> +(define_expand "vsx_xvcvsp<su>xds_low"
>> +  [(match_operand:V2DI 0 "vsx_register_operand")
>> +   (match_operand:V4SF 1 "vsx_register_operand")
>> +   (any_fix (pc))]
>> +  "VECTOR_UNIT_VSX_P (V2DFmode)"
>> +{
>> +  /* Shift left one word to put even word in correct location */
>> +  rtx rtx_tmp;
>> +  rtx rtx_val = GEN_INT (4);
>> +  rtx_tmp = gen_reg_rtx (V4SFmode);
>> +  emit_insn (gen_altivec_vsldoi_v4sf (rtx_tmp, operands[1], operands[1],
>> +                                      rtx_val));
>> +
> 
> I think this shift is only needed for LE, see the existing handlings on 
> float/signed int to double conversions, like:
> 
> (define_expand "doublee<mode>2"
> (define_expand "doubleo<mode>2
Yes, I tested builtins-3-runnable.c on a BE machine and it fails.  I moved the shift to the LE stanza only.  The patch now passes on LE and BE.  I think the fact that the tests cases got lost is why this didnt' get caught the first time.

> 
>> +  if (BYTES_BIG_ENDIAN)
>> +    emit_insn (gen_vsx_xvcvsp<su>xds_be (operands[0], rtx_tmp));
>> +  else
>> +    emit_insn (gen_vsx_xvcvsp<su>xds_le (operands[0], rtx_tmp));
>> +
>> +  DONE;
>> +})
>> +
>>  ;; Generate float2 double
>>  ;; convert two double to float
>>  (define_expand "float2_v2df"
>> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
>> index 7b54a241a7b..64a43b55e2d 100644
>> --- a/gcc/doc/extend.texi
>> +++ b/gcc/doc/extend.texi
>> @@ -22552,6 +22552,19 @@ can use @var{vector long} instead of @var{vector long long},
>>  @var{vector bool long} instead of @var{vector bool long long}, and
>>  @var{vector unsigned long} instead of @var{vector unsigned long long}.
>>  
>> +@smallexample
>> +vector signed signed long long vec_signedo (vector float);
>> +vector signed signed long long vec_signede (vector float);
>> +vector unsigned signed long long vec_signedo (vector float);
>> +vector unsigned signed long long vec_signede (vector float);
>> +@end smallexample
>> +
>> +The overloaded built-ins @code{vec_signedo} and @code{vec_signede} convert the
>> +even/odd input vector elements to signed/unsigned long long integer values in
>> +addition to the supported arguments and return types documented in the PVIPR.
>> +Negative input values are returned as zero for the unsigned long long return
>> +values.
> 
> These functions are placed in stanza [vsx] instead of [power8-vector], so it should
> be in the section "PowerPC AltiVec Built-in Functions Available on ISA 2.06".
> 
> As mentioned in the replies to other patches, since it's an extension from the
> existing PVIPR bifs and there is nothing special from them, I prefer to just mention
> them but omit the description on them.

Moved, description now just says they are new extensions of the exiting built-ins documented in the
PVIPR.

             Carl 
> 
> BR,
> Kewen
> 
>> +
>>  Only functions excluded from the PVIPR are listed here.
>>  
>>  @smallexample