Re: [PATCH 2/13] rs6000, Remove __builtin_vsx_xvcvspsxws built-in

["Kewen.Lin" <linkw@linux.ibm.com>]

Hi Carl,

on 2024/5/25 04:18, Carl Love wrote:
> Kewen:
> 
> On 5/14/24 01:43, Kewen.Lin wrote:
>> Hi,
>>
>> on 2024/4/20 05:17, Carl Love wrote:
>>> rs6000, Remove __builtin_vsx_xvcvspsxws built-in
>>>
>>> The built-in __builtin_vsx_xvcvspsxws is a duplicate of the vec_signed
>>> built-in that is documented in the PVIPR.  The __builtin_vsx_xvcvspsxws
>>> built-in is not documented and there are no test cases for it.
>>>
>>> This patch removes the redundant built-in.
>>
>> By revisiting the comments on the previous version:
>> https://gcc.gnu.org/pipermail/gcc-patches/2024-February/646723.html
> 
> The comments from the previous version:
> -----------------------------------------------------------------
>    I think we should recommend users to adopt the recommended built-ins in
>    PVIPR, by checking the corresponding mnemonic in PVIPR, I got:
> 
>    __builtin_vsx_xvcvspsxws -> vec_signed
>    __builtin_vsx_xvcvspsxds -> N/A
>    __builtin_vsx_xvcvspuxds -> N/A
>    __builtin_vsx_xvcvdpsxws -> vec_signed{e,o}
>    __builtin_vsx_xvcvdpuxws -> vec_unsigned{e,o}
>    __builtin_vsx_xvcvdpuxds_uns -> vec_unsigned
>    __builtin_vsx_xvcvspdp   -> vec_double{e,o}
>    __builtin_vsx_xvcvdpsp   -> vec_float{e,o}
>    __builtin_vsx_xvcvspuxws -> vec_unsigned
>    __builtin_vsx_xvcvsxwdp  -> vec_double{e,o}
>    __builtin_vsx_xvcvuxddp_uns> vec_double
> 
>    For __builtin_vsx_xvcvspsxds and __builtin_vsx_xvcvspuxds which don't have
>    the according PVIPR built-ins, we can extend the current vec_{un,}signed{e,o}
>    to cover them and document them following the section mentioning PVIPR.
> 
> are handled by multiple patches in the new series.  The main comment on the previous patch series was to remove most of the built-ins as they were redundant.  So, basically most of the patches in the previous series were thrown out and a new series to remove the built-ins in the current series.
> ----------------------------------------------------------------------------
> 
> That all said, I distinctly remember addressing each of the above built-ins.  The work on the series got
> interrupted a couple of times and it looks like some of the patches to address the above got lost.  My bad.
> The following is a list of which patch takes care of removing the duplicate built-ins.

No problem, thanks for working on this!!

> 
> __builtin_vsx_xvcvspsxws                     patch 2 removes this built-in
> __builtin_vsx_xvcvspsxds -> N/A              patch 4 extends vec_{un,}signede to cover this built-in,
> 					     Built-in used in rs6000-overload.def.  Built-in now for                           
>                                              internal use only.
> __builtin_vsx_xvcvspuxds -> N/A              patch 4 extends vec_{un,}signedo to cover this built-in.
> 					     Built-in used in rs6000-overload.def.  Built-in now for
>                                              internal use only 
> 
> 
> __builtin_vsx_xvcvdpsxws -> vec_signed{e,o}   removed in patch 4
> __builtin_vsx_xvcvdpuxws -> vec_unsigned{e,o} removed in patch 4
> 
> __builtin_vsx_xvcvdpuxds_uns -> vec_unsigned  remove in patch 4
> __builtin_vsx_xvcvspuxws -> vec_unsigned      remove in patch 4

Just to avoid some misunderstanding, I guess you meant the new patch 4?
As the current patch 4 doesn't remove these.

> 
> The following will changes will be put into a new patch when the series is reposted.  It appears they
> got lost in the current series.  My bad.
> 
> __builtin_vsx_xvcvspdp   -> vec_double{e,o}   remove in new patch number 5
> __builtin_vsx_xvcvdpsp   -> vec_float{e,o}    remove in new patch number 5
> 
> __builtin_vsx_xvcvsxwdp  -> vec_double{e,o}   remove in new patch number 5
> __builtin_vsx_xvcvuxddp_uns> vec_double       remove in new patch number 5
> 
>>
>> I wonder if it's intentional to keep the others, at least bifs
>> __builtin_vsx_xvcvdpuxds_uns, __builtin_vsx_xvcvspuxws and
>> __builtin_vsx_xvcvuxddp_uns looks removable, users can just uses the
>> equivalent ones in PVIPR.  And for the others, users can still use
>> the PVIPR ones by considering endianness (controlling with endianness
>> macros).
>>
> 
> Hopefully that makes it clearer where the various changes are.   
> 
> The next series will add a new patch 5 in the series.  The remaining patches in this series, patches 5, 6, ... will get moved to patch 6, 7, ... in the next posting of the built-in cleanup patch series.
> 

OK, thanks!

BR,
Kewen