Re: [PATCH,RS6000 2/5] Rework the RS6000_BTM defines.
On Tue, Jun 07, 2022 at 11:45:13AM -0500, will schmidt wrote: > Additional comments below. > I've made note of the comments, and request (ask) that this be > approved, with a pinky promise that I intend to follow up on the > suggestions in my next patch series. Suggestions aren't requirements :-) > > If we drop bu_mask in function rs6000_target_modify_macros, function > > rs6000_builtin_mask_calculate will have only one use place in > > function > > rs6000_option_override_internal. IMHO this function > > rs6000_builtin_mask_calculate also becomes stale after built-in > > function > > rewriting and needs some updates with new bif framework later. > > The DEBUG output using the builtin_mask still appeared to have some > potential value, but I can make a point to investigate that further. "Potential value" is a value of zero, if not a negative value. If some debug output has real and current value (which are two sides of the same coin), it will be apparent to every reader. Debug output that isn't useful currently is throw-away, and should be thrown away. It is easy to recreate (it is a totally boring number of print statements after all), and you can pull it from git history anyway. Segher
Re: [PATCH,RS6000 2/5] Rework the RS6000_BTM defines.
On Tue, 2022-06-07 at 10:50 +0800, Kewen.Lin wrote: > Hi Will, Hi! > > The whole series looks good to me, thanks! :-) > IMHO one place can be > further refactored, not sure if it's worth to updating together in > this series, it's ... Additional comments below. I've made note of the comments, and request (ask) that this be approved, with a pinky promise that I intend to follow up on the suggestions in my next patch series. > > on 2022/6/7 06:05, will schmidt wrote: > > [PATCH,RS6000 2/5) Rework the RS6000_BTM defines. > > > > The RS6000_BTM_ definitions are mostly unused after the > > rs6000 > > builtin code was reworked. The remaining references can be > > replaced > > with the OPTION_MASK_ and MASK_ equivalents. > > > > This patch remvoes the defines: > > RS6000_BTM_FRES, RS6000_BTM_FRSQRTE, RS6000_BTM_FRSQRTES, > > RS6000_BTM_POPCNTD, RS6000_BTM_CELL, RS6000_BTM_DFP, > > RS6000_BTM_HARD_FLOAT, RS6000_BTM_LDBL128, RS6000_BTM_64BIT, > > RS6000_BTM_POWERPC64, RS6000_BTM_FLOAT128, RS6000_BTM_FLOAT128_HW > > RS6000_BTM_MMA, RS6000_BTM_P10. > > > > I note that the BTM -> OPTION_MASK mappings are not always 1-to-1. > > in particular the BTM_FRES and BTM_FRSQRTE values were both mapped > > to > > OPTION_MASK_PPC_GFXOPT, while the BTM_FRE and BTM_FRSQRTES both > > mapped > > to OPTION_MASK_POPCNTB. In total I spent quite a bit of time > > double-checking these since it looked like copy/paste errors. I > > split > > some of these changes out into a subsequent patch to limit the > > amount > > of potential confusion in any particular patch. > > > > gcc/ > > * config/rs6000/rs6000-c.cc: Update comments. > > * config/rs6000/rs6000.cc (RS6000_BTM_FRES, RS6000_BTM_FRSQRTE, > > RS6000_BTM_FRSQRTES, RS6000_BTM_POPCNTD, RS6000_BTM_CELL, > > RS6000_BTM_64BIT, RS6000_BTM_POWERPC64, RS6000_BTM_DFP, > > RS6000_BTM_HARD_FLOAT,RS6000_BTM_LDBL128, RS6000_BTM_FLOAT128, > > RS6000_BTM_FLOAT128_HW, RS6000_BTM_MMA, RS6000_BTM_P10): > > Replace > > with OPTION_MASK_PPC_GFXOPT, OPTION_MASK_PPC_GFXOPT, > > OPTION_MASK_POPCNTB, OPTION_MASK_POPCNTD, > > OPTION_MASK_FPRND, MASK_64BIT, MASK_POWERPC64, > > OPTION_MASK_DFP, OPTION_MASK_SOFT_FLOAT, OPTION_MASK_MULTIPLE, > > OPTION_MASK_FLOAT128_KEYWORD, OPTION_MASK_FLOAT128_HW, > > OPTION_MASK_MMA, OPTION_MASK_POWER10. > > * config/rs6000/rs6000.h (RS6000_BTM_FRES, RS6000_BTM_FRSQRTE, > > RS6000_BTM_FRSQRTES, RS6000_BTM_POPCNTD, RS6000_BTM_CELL, > > RS6000_BTM_DFP, RS6000_BTM_HARD_FLOAT, RS6000_BTM_LDBL128, > > RS6000_BTM_64BIT, RS6000_BTM_POWERPC64, RS6000_BTM_FLOAT128, > > RS6000_BTM_FLOAT128_HW, RS6000_BTM_MMA, RS6000_BTM_P10): > > Delete. > > > > diff --git a/gcc/config/rs6000/rs6000-c.cc > > b/gcc/config/rs6000/rs6000-c.cc > > index 9c8cbd7a66e4..4c99afc761ae 100644 > > --- a/gcc/config/rs6000/rs6000-c.cc > > +++ b/gcc/config/rs6000/rs6000-c.cc > > @@ -594,13 +594,13 @@ rs6000_target_modify_macros (bool define_p, > > HOST_WIDE_INT flags, > > via the target attribute/pragma. */ > >if ((flags & OPTION_MASK_FLOAT128_HW) != 0) > > rs6000_define_or_undefine_macro (define_p, > > "__FLOAT128_HARDWARE__"); > > > >/* options from the builtin masks. */ > > - /* Note that RS6000_BTM_CELL is enabled only if (rs6000_cpu == > > - PROCESSOR_CELL) (e.g. -mcpu=cell). */ > > - if ((bu_mask & RS6000_BTM_CELL) != 0) > > + /* Note that OPTION_MASK_FPRND is enabled only if > > + (rs6000_cpu == PROCESSOR_CELL) (e.g. -mcpu=cell). */ > > + if ((bu_mask & OPTION_MASK_FPRND) != 0) > > rs6000_define_or_undefine_macro (define_p, "__PPU__"); > > > > ... here. In function rs6000_target_modify_macros, bu_mask is used > by > two places, the beginning debug outputting and the above > OPTION_MASK_FPRND > check. I wonder if we can get rid of bu_mask and just use sth. like: > > (rs6000_cpu == PROCESSOR_CELL) && (flags & OPTION_MASK_FPRND) > Agreed. > // the others are using "flags &", it's passed by rs6000_isa_flags, > // should be the same as just using OPTION_MASK_FPRND. > > If we drop bu_mask in function rs6000_target_modify_macros, function > rs6000_builtin_mask_calculate will have only one use place in > function > rs6000_option_override_internal. IMHO this function > rs6000_builtin_mask_calculate also becomes stale after built-in > function > rewriting and needs some updates with new bif framework later. The DEBUG output using the builtin_mask still appeared to have some potential value, but I can make a point to investigate that further. I do have in my queue to try to resolve PR 101865, that is the bug with ARCH_PWR8. I got into this OPTION_MASK side-quest as part of the investigation into that bug. I can make a point to investigate and clean up the bu_mask usage as part of that series. Thanks -Will > > BR, > Kewen
Re: [PATCH,RS6000 2/5] Rework the RS6000_BTM defines.
Hi Will, The whole series looks good to me, thanks! IMHO one place can be further refactored, not sure if it's worth to updating together in this series, it's ... on 2022/6/7 06:05, will schmidt wrote: > [PATCH,RS6000 2/5) Rework the RS6000_BTM defines. > > The RS6000_BTM_ definitions are mostly unused after the rs6000 > builtin code was reworked. The remaining references can be replaced > with the OPTION_MASK_ and MASK_ equivalents. > > This patch remvoes the defines: > RS6000_BTM_FRES, RS6000_BTM_FRSQRTE, RS6000_BTM_FRSQRTES, > RS6000_BTM_POPCNTD, RS6000_BTM_CELL, RS6000_BTM_DFP, > RS6000_BTM_HARD_FLOAT, RS6000_BTM_LDBL128, RS6000_BTM_64BIT, > RS6000_BTM_POWERPC64, RS6000_BTM_FLOAT128, RS6000_BTM_FLOAT128_HW > RS6000_BTM_MMA, RS6000_BTM_P10. > > I note that the BTM -> OPTION_MASK mappings are not always 1-to-1. > in particular the BTM_FRES and BTM_FRSQRTE values were both mapped to > OPTION_MASK_PPC_GFXOPT, while the BTM_FRE and BTM_FRSQRTES both mapped > to OPTION_MASK_POPCNTB. In total I spent quite a bit of time > double-checking these since it looked like copy/paste errors. I split > some of these changes out into a subsequent patch to limit the amount > of potential confusion in any particular patch. > > gcc/ > * config/rs6000/rs6000-c.cc: Update comments. > * config/rs6000/rs6000.cc (RS6000_BTM_FRES, RS6000_BTM_FRSQRTE, > RS6000_BTM_FRSQRTES, RS6000_BTM_POPCNTD, RS6000_BTM_CELL, > RS6000_BTM_64BIT, RS6000_BTM_POWERPC64, RS6000_BTM_DFP, > RS6000_BTM_HARD_FLOAT,RS6000_BTM_LDBL128, RS6000_BTM_FLOAT128, > RS6000_BTM_FLOAT128_HW, RS6000_BTM_MMA, RS6000_BTM_P10): Replace > with OPTION_MASK_PPC_GFXOPT, OPTION_MASK_PPC_GFXOPT, > OPTION_MASK_POPCNTB, OPTION_MASK_POPCNTD, > OPTION_MASK_FPRND, MASK_64BIT, MASK_POWERPC64, > OPTION_MASK_DFP, OPTION_MASK_SOFT_FLOAT, OPTION_MASK_MULTIPLE, > OPTION_MASK_FLOAT128_KEYWORD, OPTION_MASK_FLOAT128_HW, > OPTION_MASK_MMA, OPTION_MASK_POWER10. > * config/rs6000/rs6000.h (RS6000_BTM_FRES, RS6000_BTM_FRSQRTE, > RS6000_BTM_FRSQRTES, RS6000_BTM_POPCNTD, RS6000_BTM_CELL, > RS6000_BTM_DFP, RS6000_BTM_HARD_FLOAT, RS6000_BTM_LDBL128, > RS6000_BTM_64BIT, RS6000_BTM_POWERPC64, RS6000_BTM_FLOAT128, > RS6000_BTM_FLOAT128_HW, RS6000_BTM_MMA, RS6000_BTM_P10): Delete. > > diff --git a/gcc/config/rs6000/rs6000-c.cc b/gcc/config/rs6000/rs6000-c.cc > index 9c8cbd7a66e4..4c99afc761ae 100644 > --- a/gcc/config/rs6000/rs6000-c.cc > +++ b/gcc/config/rs6000/rs6000-c.cc > @@ -594,13 +594,13 @@ rs6000_target_modify_macros (bool define_p, > HOST_WIDE_INT flags, > via the target attribute/pragma. */ >if ((flags & OPTION_MASK_FLOAT128_HW) != 0) > rs6000_define_or_undefine_macro (define_p, "__FLOAT128_HARDWARE__"); > >/* options from the builtin masks. */ > - /* Note that RS6000_BTM_CELL is enabled only if (rs6000_cpu == > - PROCESSOR_CELL) (e.g. -mcpu=cell). */ > - if ((bu_mask & RS6000_BTM_CELL) != 0) > + /* Note that OPTION_MASK_FPRND is enabled only if > + (rs6000_cpu == PROCESSOR_CELL) (e.g. -mcpu=cell). */ > + if ((bu_mask & OPTION_MASK_FPRND) != 0) > rs6000_define_or_undefine_macro (define_p, "__PPU__"); > ... here. In function rs6000_target_modify_macros, bu_mask is used by two places, the beginning debug outputting and the above OPTION_MASK_FPRND check. I wonder if we can get rid of bu_mask and just use sth. like: (rs6000_cpu == PROCESSOR_CELL) && (flags & OPTION_MASK_FPRND) // the others are using "flags &", it's passed by rs6000_isa_flags, // should be the same as just using OPTION_MASK_FPRND. If we drop bu_mask in function rs6000_target_modify_macros, function rs6000_builtin_mask_calculate will have only one use place in function rs6000_option_override_internal. IMHO this function rs6000_builtin_mask_calculate also becomes stale after built-in function rewriting and needs some updates with new bif framework later. BR, Kewen
[PATCH,RS6000 2/5] Rework the RS6000_BTM defines.
[PATCH,RS6000 2/5) Rework the RS6000_BTM defines.
The RS6000_BTM_ definitions are mostly unused after the rs6000
builtin code was reworked. The remaining references can be replaced
with the OPTION_MASK_ and MASK_ equivalents.
This patch remvoes the defines:
RS6000_BTM_FRES, RS6000_BTM_FRSQRTE, RS6000_BTM_FRSQRTES,
RS6000_BTM_POPCNTD, RS6000_BTM_CELL, RS6000_BTM_DFP,
RS6000_BTM_HARD_FLOAT, RS6000_BTM_LDBL128, RS6000_BTM_64BIT,
RS6000_BTM_POWERPC64, RS6000_BTM_FLOAT128, RS6000_BTM_FLOAT128_HW
RS6000_BTM_MMA, RS6000_BTM_P10.
I note that the BTM -> OPTION_MASK mappings are not always 1-to-1.
in particular the BTM_FRES and BTM_FRSQRTE values were both mapped to
OPTION_MASK_PPC_GFXOPT, while the BTM_FRE and BTM_FRSQRTES both mapped
to OPTION_MASK_POPCNTB. In total I spent quite a bit of time
double-checking these since it looked like copy/paste errors. I split
some of these changes out into a subsequent patch to limit the amount
of potential confusion in any particular patch.
gcc/
* config/rs6000/rs6000-c.cc: Update comments.
* config/rs6000/rs6000.cc (RS6000_BTM_FRES, RS6000_BTM_FRSQRTE,
RS6000_BTM_FRSQRTES, RS6000_BTM_POPCNTD, RS6000_BTM_CELL,
RS6000_BTM_64BIT, RS6000_BTM_POWERPC64, RS6000_BTM_DFP,
RS6000_BTM_HARD_FLOAT,RS6000_BTM_LDBL128, RS6000_BTM_FLOAT128,
RS6000_BTM_FLOAT128_HW, RS6000_BTM_MMA, RS6000_BTM_P10): Replace
with OPTION_MASK_PPC_GFXOPT, OPTION_MASK_PPC_GFXOPT,
OPTION_MASK_POPCNTB, OPTION_MASK_POPCNTD,
OPTION_MASK_FPRND, MASK_64BIT, MASK_POWERPC64,
OPTION_MASK_DFP, OPTION_MASK_SOFT_FLOAT, OPTION_MASK_MULTIPLE,
OPTION_MASK_FLOAT128_KEYWORD, OPTION_MASK_FLOAT128_HW,
OPTION_MASK_MMA, OPTION_MASK_POWER10.
* config/rs6000/rs6000.h (RS6000_BTM_FRES, RS6000_BTM_FRSQRTE,
RS6000_BTM_FRSQRTES, RS6000_BTM_POPCNTD, RS6000_BTM_CELL,
RS6000_BTM_DFP, RS6000_BTM_HARD_FLOAT, RS6000_BTM_LDBL128,
RS6000_BTM_64BIT, RS6000_BTM_POWERPC64, RS6000_BTM_FLOAT128,
RS6000_BTM_FLOAT128_HW, RS6000_BTM_MMA, RS6000_BTM_P10): Delete.
diff --git a/gcc/config/rs6000/rs6000-c.cc b/gcc/config/rs6000/rs6000-c.cc
index 9c8cbd7a66e4..4c99afc761ae 100644
--- a/gcc/config/rs6000/rs6000-c.cc
+++ b/gcc/config/rs6000/rs6000-c.cc
@@ -594,13 +594,13 @@ rs6000_target_modify_macros (bool define_p, HOST_WIDE_INT
flags,
via the target attribute/pragma. */
if ((flags & OPTION_MASK_FLOAT128_HW) != 0)
rs6000_define_or_undefine_macro (define_p, "__FLOAT128_HARDWARE__");
/* options from the builtin masks. */
- /* Note that RS6000_BTM_CELL is enabled only if (rs6000_cpu ==
- PROCESSOR_CELL) (e.g. -mcpu=cell). */
- if ((bu_mask & RS6000_BTM_CELL) != 0)
+ /* Note that OPTION_MASK_FPRND is enabled only if
+ (rs6000_cpu == PROCESSOR_CELL) (e.g. -mcpu=cell). */
+ if ((bu_mask & OPTION_MASK_FPRND) != 0)
rs6000_define_or_undefine_macro (define_p, "__PPU__");
/* Tell the user if we support the MMA instructions. */
if ((flags & OPTION_MASK_MMA) != 0)
rs6000_define_or_undefine_macro (define_p, "__MMA__");
diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
index d4defc855d02..253110910bfa 100644
--- a/gcc/config/rs6000/rs6000.cc
+++ b/gcc/config/rs6000/rs6000.cc
@@ -3381,32 +3381,32 @@ rs6000_builtin_mask_calculate (void)
{
return (((TARGET_ALTIVEC)? RS6000_BTM_ALTIVEC : 0)
| ((TARGET_CMPB) ? RS6000_BTM_CMPB : 0)
| ((TARGET_VSX) ? RS6000_BTM_VSX : 0)
| ((TARGET_FRE) ? RS6000_BTM_FRE : 0)
- | ((TARGET_FRES) ? RS6000_BTM_FRES : 0)
- | ((TARGET_FRSQRTE) ? RS6000_BTM_FRSQRTE : 0)
- | ((TARGET_FRSQRTES) ? RS6000_BTM_FRSQRTES : 0)
- | ((TARGET_POPCNTD) ? RS6000_BTM_POPCNTD : 0)
- | ((rs6000_cpu == PROCESSOR_CELL) ? RS6000_BTM_CELL : 0)
+ | ((TARGET_FRES) ? OPTION_MASK_PPC_GFXOPT : 0)
+ | ((TARGET_FRSQRTE) ? OPTION_MASK_PPC_GFXOPT : 0)
+ | ((TARGET_FRSQRTES) ? OPTION_MASK_POPCNTB: 0)
+ | ((TARGET_POPCNTD) ? OPTION_MASK_POPCNTD: 0)
+ | ((rs6000_cpu == PROCESSOR_CELL) ? OPTION_MASK_FPRND : 0)
| ((TARGET_P8_VECTOR) ? RS6000_BTM_P8_VECTOR : 0)
| ((TARGET_P9_VECTOR) ? RS6000_BTM_P9_VECTOR : 0)
| ((TARGET_P9_MISC) ? RS6000_BTM_P9_MISC : 0)
| ((TARGET_MODULO)? RS6000_BTM_MODULO: 0)
- | ((TARGET_64BIT) ? RS6000_BTM_64BIT : 0)
- | ((TARGET_POWERPC64) ? RS6000_BTM_POWERPC64 : 0)
+ | ((TARGET_64BIT) ? MASK_64BIT : 0)
+ | ((TARGET_POWERPC64) ? MASK_P
