| Message ID | 7bbff7c6-706f-42ae-af37-b08a4f927c50@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | rs6000, built-in cleanup patch series | expand |
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! BR, Kewen
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. 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. 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++; I am thinking we should drop this patch from the series, i.e. don't remove __builtin_vsx_xvcmpeqsp. Thoughts? Carl > > BR, > Kewen >
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
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
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;