Re: [PATCH] rs6000, Add new overloaded vector shift builtin int128, varients

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

Kewen:

On 7/25/24 1:21 AM, Kewen.Lin wrote:
> Hi Carl,
>
> Some minor comments are inlined on top of Segher's and Peter's comments.
>
> on 2024/7/20 04:04, Carl Love wrote:
>> GCC developers:
>>
>> The following patch adds the int128 varients to the existing overloaded built-ins vec_sld, vec_sldb, vec_sldw, vec_sll, vec_slo, vec_srdb, vec_srl, vec_sro.  These varients were requested by Steve Munroe.
>>
>> The patch has been tested on a Power 10 system with no regressions.
>>
>> Please let me know if the patch is acceptable for mainline.
>>
>>                                     Carl
>>
>>
>> -------------------------------------------------------------------------------------------------------
>>   rs6000, Add new overloaded vector shift builtin int128 varients
>>
>> Add the signed __int128 and unsigned __int128 argument types for the
>> overloaded built-ins vec_sld, vec_sldb, vec_sldw, vec_sll, vec_slo,
>> vec_srdb, vec_srl, vec_sro.  For each of the new argument types add a
>> testcase and update the documentation for the built-in.
>>
>> Add the missing internal names for the float and double types for
>> overloaded builtin vec_sld for the float and double types.
> This isn't needed, see below explanation.

OK, per comments below, removed the additional internal names.

<snip>
>> diff --git a/gcc/config/rs6000/rs6000-overload.def b/gcc/config/rs6000/rs6000-overload.def
>> index c4ecafc6f7e..302e0232533 100644
>> --- a/gcc/config/rs6000/rs6000-overload.def
>> +++ b/gcc/config/rs6000/rs6000-overload.def
>> @@ -3396,9 +3396,13 @@
>>     vull __builtin_vec_sld (vull, vull, const int);
>>       VSLDOI_2DI  VSLDOI_VULL
>>     vf __builtin_vec_sld (vf, vf, const int);
>> -    VSLDOI_4SF
>> +    VSLDOI_4SF VSLDOI_VF
>>     vd __builtin_vec_sld (vd, vd, const int);
>> -    VSLDOI_2DF
>> +    VSLDOI_2DF VSLDOI_VD
> The other instances for vector integer type have multiple uses of 1st token,
> such as:
>
>    vsll __builtin_vec_sld (vsll, vsll, const int);
>      VSLDOI_2DI  VSLDOI_VSLL
>    vbll __builtin_vec_sld (vbll, vbll, const int);
>      VSLDOI_2DI  VSLDOI_VBLL
>    vull __builtin_vec_sld (vull, vull, const int);
>      VSLDOI_2DI  VSLDOI_VULL
>
> , it's unable to use the 1st token VSLDOI_2DI for the overload id (otherwise
> it can be ambiguous), but for vector float/double they don't have multiple
> variants, VSLDOI_4SF and VSLDOI_2DF are used once respectively so they are
> fine here.  I think the existing code is intentional so let's keep them
> unchanged (creating more unnecessary ids is slightly worse than before).

OK, removed the addtional tokens VSLDOI_VF and VSLDOI_VD.
>
>> +  vsq __builtin_vec_sld (vsq, vsq, const int);
>> +    VSLDOI_V1TI  VSLDOI_VSQ
>> +  vuq __builtin_vec_sld (vuq, vuq, const int);
>> +    VSLDOI_V1TI  VSLDOI_VUQ
>>
>>   [VEC_SLDB, vec_sldb, __builtin_vec_sldb]
>>     vsc __builtin_vec_sldb (vsc, vsc, const int);
>> @@ -3417,6 +3421,10 @@
>>       VSLDB_V2DI  VSLDB_VSLL
>>     vull __builtin_vec_sldb (vull, vull, const int);
>>       VSLDB_V2DI  VSLDB_VULL
>> +  vsq __builtin_vec_sldb (vsq, vsq, const int);
>> +    VSLDB_V1TI  VSLDB_VSQ
>> +  vuq __builtin_vec_sldb (vuq, vuq, const int);
>> +    VSLDB_V1TI  VSLDB_VUQ
>>
>>   [VEC_SLDW, vec_sldw, __builtin_vec_sldw]
>>     vsc __builtin_vec_sldw (vsc, vsc, const int);
>> @@ -3439,6 +3447,10 @@
>>       XXSLDWI_4SF  XXSLDWI_VF
>>     vd __builtin_vec_sldw (vd, vd, const int);
>>       XXSLDWI_2DF  XXSLDWI_VD
>> +  vsq __builtin_vec_sldw (vsq, vsq, const int);
>> +    XXSLDWI_Q  XXSLDWI_VSQ
>> +  vuq __builtin_vec_sldw (vuq, vuq, const int);
>> +    XXSLDWI_Q  XXSLDWI_VUQ
> Nit: s/XXSLDWI_Q/XXSLDWI_1TI/ to keep consistent with the
> other XXSLDWI_* as 1st token (XXSLDWI_16QI etc. are used
> above rather than XXSLDWI_{SC,UC} etc.)

OK, changed to:

   vsq __builtin_vec_sldw (vsq, vsq, const int);
     XXSLDWI_1TI  XXSLDWI_VSQ
   vuq __builtin_vec_sldw (vuq, vuq, const int);
     XXSLDWI_1TI  XXSLDWI_VUQ

<snip>

>>   [VEC_SRV, vec_srv, __builtin_vec_vsrv]
>>     vuc __builtin_vec_vsrv (vuc, vuc);
>> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
>> index 0b572afca72..5125a6d9def 100644
>> --- a/gcc/doc/extend.texi
>> +++ b/gcc/doc/extend.texi
>> @@ -23504,6 +23504,10 @@ const unsigned int);
>>   vector signed long long, const unsigned int);
>>   @exdent vector unsigned long long vec_sldb (vector unsigned long long,
>>   vector unsigned long long, const unsigned int);
>> +@exdent vector signed __int128 vec_sldb (vector signed __int128,
>> +vector signed __int128, const unsigned int);
>> +@exdent vector unsigned __int128 vec_sldb (vector unsigned __int128,
>> +vector unsigned __int128, const unsigned int);
>>   @end smallexample
>>
>>   Shift the combined input vectors left by the amount specified by the low-order
>> @@ -23531,6 +23535,10 @@ const unsigned int);
>>   vector signed long long, const unsigned int);
>>   @exdent vector unsigned long long vec_srdb (vector unsigned long long,
>>   vector unsigned long long, const unsigned int);
>> +@exdent vector signed __int128 vec_srdb (vector signed __int128,
>> +vector signed __int128, const unsigned int);
>> +@exdent vector unsigned __int128 vec_srdb (vector unsigned __int128,
>> +vector unsigned __int128, const unsigned int);
>>   @end smallexample
>>
>>   Shift the combined input vectors right by the amount specified by the low-order
>> @@ -24025,6 +24033,40 @@ int vec_any_le (vector signed __int128, vector signed __int128);
>>   int vec_any_le (vector unsigned __int128, vector unsigned __int128);
>>   @end smallexample
>>
> Personally I prefer to move the below paragraph for description here to avoid
> that two @smallexample sections are placed together, that is:
>
> "The following instances are extension of the existing overloaded built-ins
> @code{vec_sld}, @code{vec_sldw}, @code{vec_slo}, @code{vec_sro}, @code{vec_srl}
> that are documented in the PVIPR."
>
> @smallexample
> ....

OK, moved the changes for the extensions to the IPVR to right after the 
new vec_srb entries.

                        Carl