Hi Carl,
on 2024/7/23 01:37, Carl Love wrote:
>
> Kewen:
>
> On 7/22/24 2:09 AM, Kewen.Lin wrote:
>> Hi Carl,
>>
>> on 2024/7/18 00:01, Carl Love wrote:
>>> GCC maintainers:
>>>
>>> This patch removes the __builtin_vec_set_v1ti, __builtin_vec_set_v2df and
>>> __builtin_vec_set_v2di built-ins. The users should just use normal C-code
>>> to update the various vector elements. This change was originally intended
>>> to be part of the earlier series of cleanup patches. It was initially
>>> thought that some additional work would be needed to do some gimple
>>> generation instead of these built-ins. However, the existing default code
>>> generation does produce the needed code. The code generated with normal
>>> C-code is as good or better than the code generated with these built-ins.
>> I think we need to expand this a bit:
>> - For vec_set bif, the equivalent C code is as good as or better than it.
>> - For vec_insert bif whose resolving makes use of vec_set bif previously
>> (now get removed),
>> it's as good as before with optimization.
>>> The patch has been tested on Power 10 LE with no regressions.
>>>
>>> Please let me know if the patch is acceptable for mainline. Thanks.
>>>
>>> Carl
>>>
>>> ---------------------------------------------------------------
>>> rs6000, Remove __builtin_vec_set_v1ti, __builtin_vec_set_v2df,
>>> __builtin_vec_set_v2di
>>>
>>> Remove the built-ins, use the default gimple generation instead.
>>>
>>> gcc/ChangeLog:
>>> * config/rs6000/rs6000-builtins.def (__builtin_vec_set_v1ti,
>>> __builtin_vec_set_v2df, __builtin_vec_set_v2di): Remove built-in
>>> definitions.
>>> * config/rs6000/rs6000-c.cc (resolve_vec_insert): Remove if
>>> statemnts for mode == V2DFmode, mode == V2DImode and
>> Nit: s/statemnts/statements/
>
> OK, fixed
>> Maybe a bit more meaningful like: Remove the handling for constant
>> vec_insert position
>> with VECTOR_UNIT_VSX_P V1TImode, V2DFmode and V2DImode modes.
> OK, changed
>>
>>
>>> mode == V1TImode that reference RS6000_BIF_VEC_SET_V2DF,
>>> RS6000_BIF_VEC_SET_V2DI and RS6000_BIF_VEC_SET_V1TI.
>>> ---
>>> gcc/config/rs6000/rs6000-builtins.def | 13 ---------
>>> gcc/config/rs6000/rs6000-c.cc | 40 ---------------------------
>>> 2 files changed, 53 deletions(-)
>>>
>>> diff --git a/gcc/config/rs6000/rs6000-builtins.def
>>> b/gcc/config/rs6000/rs6000-builtins.def
>>> index 896d9686ac6..0ebc940f395 100644
>>> --- a/gcc/config/rs6000/rs6000-builtins.def
>>> +++ b/gcc/config/rs6000/rs6000-builtins.def
>>> @@ -1263,19 +1263,6 @@
>>> const signed long long __builtin_vec_ext_v2di (vsll, signed int);
>>> VEC_EXT_V2DI nothing {extract}
>>>
>>> -;; VEC_SET_V1TI, VEC_SET_V2DF and VEC_SET_V2DI are used in
>>> -;; resolve_vec_insert(), rs6000-c.cc
>>> -;; TODO: Remove VEC_SET_V1TI, VEC_SET_V2DF and VEC_SET_V2DI once the uses
>>> -;; in resolve_vec_insert are replaced by the equivalent gimple statements.
>>> - const vsq __builtin_vec_set_v1ti (vsq, signed __int128, const int<0,0>);
>>> - VEC_SET_V1TI nothing {set}
>>> -
>>> - const vd __builtin_vec_set_v2df (vd, double, const int<1>);
>>> - VEC_SET_V2DF nothing {set}
>>> -
>>> - const vsll __builtin_vec_set_v2di (vsll, signed long long, const int<1>);
>>> - VEC_SET_V2DI nothing {set}
>>> ->> Unexpected empty line removed.
> ?? I don't remove the blank line before the removed comment, so there is
> still a single blank line before the next entry. Specifically, the code with
> the above removed now looks like:
>
> ...
> const signed long long __builtin_vec_ext_v2di (vsll, signed int);
> VEC_EXT_V2DI nothing {extract}
>
> const vsc __builtin_vsx_cmpge_16qi (vsc, vsc);
> CMPGE_16QI vector_nltv16qi {}
>
> const vsll __builtin_vsx_cmpge_2di (vsll, vsll);
> CMPGE_2DI vector_nltv2di {}
> ....
>
> Which looks OK to me?
ah, I missed that, so it's fine, thanks for clarifying!
>>
>> Similar to vec_init removal, we should also get rid of set bif attribute,
>> bif_is_set and altivec_expand_vec_set_builtin etc.
> That will also require removing:
>
> const vsq __builtin_vsx_set_1ti (vsq, signed __int128, const int<0,0>);
> SET_1TI vsx_set_v1ti {set}
>
> const vd __builtin_vsx_set_2df (vd, double, const int<0,1>);
> SET_2DF vsx_set_v2df {set}
>
> const vsll __builtin_vsx_set_2di (vsll, signed long long, const int<0,1>);
> SET_2DI vsx_set_v2di {set}
I have no idea what are these three builtins for, they have the same prototypes
as the above removed builtins (just with different bif names), and also are
bif_is_set so I'd expect they are handled the same as the removed ones.
I guessed they were designed to be used as overloaded instances for vec_set but
eventually we handled vec_set differently (via bif attribute).
>
> I would assume the C-code generation for the above will be as good or better
> than the code generation for the built-ins but will need to verify that. I
> haven't looked at them specifically.
>
Yeah, it's good to verify, thanks.
BR,
Kewen
> Carl
