Re: [PATCH 12/13] rs6000, remove __builtin_vsx_xvcmpeqsp built-in

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

Hi,

on 2024/5/24 02:21, Carl Love wrote:
> 
> 
> On 5/13/24 22:37, Kewen.Lin wrote:
>> Hi,
>>
>> on 2024/4/20 05:18, Carl Love wrote:
>>> rs6000, remove __builtin_vsx_xvcmpeqsp built-in
>>>
>>> The built-in __builtin_vsx_xvcmpeqsp is a duplicate of the overloaded
>>> vec_cmpeq built-in.  The built-in is undocumented.  The built-in and
>>> the test cases are removed.
>>>
>>> gcc/ChangeLog:
>>> 	* config/rs6000/rs6000-builtins.def (__builtin_vsx_xvcmpeqsp):
>>> 	Remove built-in definition.
>>>
>>
>> Ah, you separated this __builtin_vsx_xvcmpeqsp from the one for
>> __builtin_vsx_xvcmpeqsp_p, it's fine, please ignore the comments for
>> considering this __builtin_vsx_xvcmpeqsp in my previous reply to 11/13.
>>
>>
>>> gcc/testsuite/ChangeLog:
>>> 	* vsx-builtin-3.c (do_cmp): Remove test case for
>>> 	__builtin_vsx_xvcmpeqsp.
>>> ---
>>>  gcc/config/rs6000/rs6000-builtins.def            | 3 ---
>>>  gcc/testsuite/gcc.target/powerpc/vsx-builtin-3.c | 2 --
>>>  2 files changed, 5 deletions(-)
>>>
>>> diff --git a/gcc/config/rs6000/rs6000-builtins.def b/gcc/config/rs6000/rs6000-builtins.def
>>> index 2f6149edd5f..19d05b8043a 100644
>>> --- a/gcc/config/rs6000/rs6000-builtins.def
>>> +++ b/gcc/config/rs6000/rs6000-builtins.def
>>> @@ -1613,9 +1613,6 @@
>>>    const signed int __builtin_vsx_xvcmpeqdp_p (signed int, vd, vd);
>>>      XVCMPEQDP_P vector_eq_v2df_p {pred}
>>>  
>>> -  const vf __builtin_vsx_xvcmpeqsp (vf, vf);
>>> -    XVCMPEQSP vector_eqv4sf {}
>>> -
>>>    const vd __builtin_vsx_xvcmpgedp (vd, vd);
>>>      XVCMPGEDP vector_gev2df {}
>>>  
>>> diff --git a/gcc/testsuite/gcc.target/powerpc/vsx-builtin-3.c b/gcc/testsuite/gcc.target/powerpc/vsx-builtin-3.c
>>> index 35ea31b2616..245893dc0e3 100644
>>> --- a/gcc/testsuite/gcc.target/powerpc/vsx-builtin-3.c
>>> +++ b/gcc/testsuite/gcc.target/powerpc/vsx-builtin-3.c
>>> @@ -27,7 +27,6 @@
>>>  /* { dg-final { scan-assembler "xvcmpeqdp" } } */
>>>  /* { dg-final { scan-assembler "xvcmpgtdp" } } */
>>>  /* { dg-final { scan-assembler "xvcmpgedp" } } */
>>> -/* { dg-final { scan-assembler "xvcmpeqsp" } } */
>>>  /* { dg-final { scan-assembler "xvcmpgtsp" } } */
>>>  /* { dg-final { scan-assembler "xvcmpgesp" } } */
>>>  /* { dg-final { scan-assembler "xxsldwi" } } */
>>> @@ -112,7 +111,6 @@ int do_cmp (void)
>>>    d[i][0] = __builtin_vsx_xvcmpgtdp (d[i][1], d[i][2]); i++;
>>>    d[i][0] = __builtin_vsx_xvcmpgedp (d[i][1], d[i][2]); i++;
>>>  
>>> -  f[i][0] = __builtin_vsx_xvcmpeqsp (f[i][1], f[i][2]); i++;
>>>    f[i][0] = __builtin_vsx_xvcmpgtsp (f[i][1], f[i][2]); i++;
>>>    f[i][0] = __builtin_vsx_xvcmpgesp (f[i][1], f[i][2]); i++;
>>>    return i;
>>
>> As the other in this patch series, I prefer to change it with
>> vec_cmpeq here, OK for trunk with this tweaked (also keep the
>> scan there), thanks!
> 
> When I went to change the test case I noticed that __builtin_vsx_xvcmpeqsp and vec_cmpeq both return a vector where the element is all ones if the comparison is True and zeros if False.  However, the return type for __builtin_vsx_xvcmpeqsp is vector floats but vec_cmpeq returns vector bool.
> 

Ah, so they are not equivalent from prototype perspective.

> The PVIPR says the vec_cmpeq built-in returns a value where each bit in the vector element is a 1 if the comparison is equal and 0 otherwise.  However, the documented result is a vector bool int for the floating point comparison.  The return value for __builtin_vsx_xvcmpeqsp was vector float.

IMHO PVIPR prototype (returning vector bool) makes more sense,
it does match better with what the result holds.

> 
> So, the "bit values" returned are the same but not of the same type. So technically vec_cmpeq is not a drop in replacement for __builtin_vsx_xvcmpeqsp.  Given that, perhaps we should not be removing __builtin_vsx_xvcmpeqsp?
> 
> The testcase has to be changed from:
>      f[i][0] = __builtin_vsx_xvcmpeqsp (f[i][1], f[i][2]); i++;
>      bi[i][0] = vec_cmpeq (f[i][1], f[i][2]); i++;

For the test case change, I'd expect that it can work with:

-  f[i][0] = __builtin_vsx_xvcmpeqsp (f[i][1], f[i][2]); i++;
+  f[i][0] = (vector float) vec_cmpeq (f[i][1], f[i][2]); i++;

> 
> I am thinking we should drop this patch from the series, i.e. don't remove __builtin_vsx_xvcmpeqsp.  Thoughts?
> 

Since __builtin_vsx_xvcmpeqsp is an undocumented built-in, I don't
expect users to use it, even there is someone, IMHO vector bool is
a better fit.  In case someone actually wants the vector non-bool
type, he/she can just add an explicit conversion.  So I'm inclined
to remove the vsx_xvcmpeqsp, users should try to use PVIPR built-ins
as possible as they can.  But I'm also fine for holding on this, as
there are some other related built-ins cmp* (cmpge,cmpgt...), we
can re-visit and handle them together later.

BR,
Kewen