Kewen:
On 5/24/24 03:43, Kewen.Lin wrote:
> 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.
Yes, I tend to agree. I think the user would use be likely using the test so
they could create a mask to selectively replace vector elements. A bool type
make more sense in that case.
>
>>
>> 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++;
Yes, that does work.
>
>>
>> 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.
My preference would be to skip this for now and then come back later with a new
patch to address all of the various comparisons for both float and double.
Carl
