Re: [PATCH 3/13] rs6000, fix error in unsigned vector float to unsigned int built-in definitions

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

Keewn:

On 5/14/24 00:00, Kewen.Lin wrote:
> Hi,
> 
> on 2024/4/20 05:17, Carl Love wrote:
>> rs6000, fix error in unsigned vector float to unsigned  int built-in definitions
>>
>> The built-ins __builtin_vsx_vunsigned_v2df and__builtin_vsx_vunsigned_v4sf
>> are supposed to take a vector of floats and return a vector of unsigned
>> long long ints.  The definitions are using the signed version of the
> 
> Sorry for nitpicking, here __builtin_vsx_vunsigned_v2df takes vector of doubles
> and returns vector of unsigned long long ints while __builtin_vsx_vunsigned_v4sf
> takes vector of floats and returns vector of unsigned ints.

That is not nitpicking, the description is wrong.  Changed float to double.
> 
>> instructions not the unsigned version of the instruction.  The results
>> should also be unsigned.  The builtins are used by the overloaded
>> vec_unsigned builtin which has an unsigned result.
>>
>> Similarly the built-ins __builtin_vsx_vunsignede_v2df and
>> __builtin_vsx_vunsignedo_v2df are supposed to retun an unsigned result.
> 
> Nit: s/retun/return/

Fixed.

> 
>> If the floating point argument is negative, the unsigned result is zero.
>> The built-ins are used in the overloaded built-in vec_unsignede and
>> vec_unsignedo respectively.
>>
>> Add a test cases for a negative floating point arguments for each of the
>> above built-ins.
>>
>> gcc/ChangeLog:
>> 	* config/rs6000/rs6000-builtins.def (__builtin_vsx_vunsigned_v2df,
>> 	__builtin_vsx_vunsigned_v4sf, __builtin_vsx_vunsignede_v2df,
>> 	__builtin_vsx_vunsignedo_v2df): Change the result type to unsigned.
>>
>> gcc/testsuite/ChangeLog:
>> 	* gcc.target/powerpc/builtins-3-runnable.c: Add tests for
>> 	vec_unsignede and vec_unsignedo with negative arguments.
>> ---
>>  gcc/config/rs6000/rs6000-builtins.def         | 12 +++++-----
>>  .../gcc.target/powerpc/builtins-3-runnable.c  | 23 ++++++++++++++++---
>>  2 files changed, 26 insertions(+), 9 deletions(-)
>>
>> diff --git a/gcc/config/rs6000/rs6000-builtins.def b/gcc/config/rs6000/rs6000-builtins.def
>> index c6d2ea1bc39..bf9a0ae22fc 100644
>> --- a/gcc/config/rs6000/rs6000-builtins.def
>> +++ b/gcc/config/rs6000/rs6000-builtins.def
>> @@ -1580,16 +1580,16 @@
>>    const vsi __builtin_vsx_vsignedo_v2df (vd);
>>      VEC_VSIGNEDO_V2DF vsignedo_v2df {}
>>  
>> -  const vsll __builtin_vsx_vunsigned_v2df (vd);
>> -    VEC_VUNSIGNED_V2DF vsx_xvcvdpsxds {}
>> +  const vull __builtin_vsx_vunsigned_v2df (vd);
>> +    VEC_VUNSIGNED_V2DF vsx_xvcvdpuxds {}
>>  
>> -  const vsi __builtin_vsx_vunsigned_v4sf (vf);
>> -    VEC_VUNSIGNED_V4SF vsx_xvcvspsxws {}
>> +  const vui __builtin_vsx_vunsigned_v4sf (vf);
>> +    VEC_VUNSIGNED_V4SF vsx_xvcvspuxws {}
>>  
>> -  const vsi __builtin_vsx_vunsignede_v2df (vd);
>> +  const vui __builtin_vsx_vunsignede_v2df (vd);
>>      VEC_VUNSIGNEDE_V2DF vunsignede_v2df {}
>>  
>> -  const vsi __builtin_vsx_vunsignedo_v2df (vd);
>> +  const vui __builtin_vsx_vunsignedo_v2df (vd);
>>      VEC_VUNSIGNEDO_V2DF vunsignedo_v2df {}
>>  
>>    const vf __builtin_vsx_xscvdpsp (double);
>> diff --git a/gcc/testsuite/gcc.target/powerpc/builtins-3-runnable.c b/gcc/testsuite/gcc.target/powerpc/builtins-3-runnable.c
>> index 0231a1fd086..6d4fe84c8a1 100644
>> --- a/gcc/testsuite/gcc.target/powerpc/builtins-3-runnable.c
>> +++ b/gcc/testsuite/gcc.target/powerpc/builtins-3-runnable.c
>> @@ -313,6 +313,15 @@ int main()
>>  	test_unsigned_int_result (ALL, vec_uns_int_result,
>>  				  vec_uns_int_expected);
>>  
>> +	/* Convert single precision float to  unsigned int.  Negative
>> +	   arguments
>> +	 */
>> +	vec_flt0 = (vector float){-14.930, -834.49, -3.3, -5.4};
>> +	vec_uns_int_expected = (vector unsigned int){0, 0, 0, 0};
>> +	vec_uns_int_result = vec_unsigned (vec_flt0);
>> +	test_unsigned_int_result (ALL, vec_uns_int_result,
>> +				  vec_uns_int_expected);
>> +
>>  	/* Convert double precision float to long long unsigned int */
>>  	vec_dble0 = (vector double){124.930, 8134.49};
>>  	vec_ll_uns_int_expected = (vector long long unsigned int){124, 8134};
>> @@ -321,9 +330,9 @@ int main()
>>  				     vec_ll_uns_int_expected);
> 
> Nit: Similar coverage on negative for vector double can be added here.

Added.

                  Carl