| Message ID | 82211644fb1f61894e5b99a7c5fdb8e73539ddc0.camel@vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Clean up MASK_ and RS6000_BTM_ defines | expand |
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_<xxxx> definitions are mostly unused after the rs6000 > builtin code was reworked. The remaining references can be replaced > with the OPTION_MASK_<xxxx> and MASK_<xxxx> 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
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_<xxxx> definitions are mostly unused after the > > rs6000 > > builtin code was reworked. The remaining references can be > > replaced > > with the OPTION_MASK_<xxxx> and MASK_<xxxx> 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
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
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_POWERPC64 : 0) | ((TARGET_CRYPTO) ? RS6000_BTM_CRYPTO : 0) | ((TARGET_HTM) ? RS6000_BTM_HTM : 0) - | ((TARGET_DFP) ? RS6000_BTM_DFP : 0) - | ((TARGET_HARD_FLOAT) ? RS6000_BTM_HARD_FLOAT : 0) + | ((TARGET_DFP) ? OPTION_MASK_DFP : 0) + | ((TARGET_HARD_FLOAT) ? OPTION_MASK_SOFT_FLOAT : 0) | ((TARGET_LONG_DOUBLE_128 && TARGET_HARD_FLOAT - && !TARGET_IEEEQUAD) ? RS6000_BTM_LDBL128 : 0) - | ((TARGET_FLOAT128_TYPE) ? RS6000_BTM_FLOAT128 : 0) - | ((TARGET_FLOAT128_HW) ? RS6000_BTM_FLOAT128_HW : 0) - | ((TARGET_MMA) ? RS6000_BTM_MMA : 0) - | ((TARGET_POWER10) ? RS6000_BTM_P10 : 0)); + && !TARGET_IEEEQUAD) ? OPTION_MASK_MULTIPLE : 0) + | ((TARGET_FLOAT128_TYPE) ? OPTION_MASK_FLOAT128_KEYWORD : 0) + | ((TARGET_FLOAT128_HW) ? OPTION_MASK_FLOAT128_HW : 0) + | ((TARGET_MMA) ? OPTION_MASK_MMA : 0) + | ((TARGET_POWER10) ? OPTION_MASK_POWER10 : 0)); } /* Implement TARGET_MD_ASM_ADJUST. All asm statements are considered to clobber the XER[CA] bit because clobbering that bit without telling the compiler worked just fine with versions of GCC before GCC 5, and @@ -24047,28 +24047,28 @@ static struct rs6000_opt_mask const rs6000_opt_masks[] = static struct rs6000_opt_mask const rs6000_builtin_mask_names[] = { { "altivec", RS6000_BTM_ALTIVEC, false, false }, { "vsx", RS6000_BTM_VSX, false, false }, { "fre", RS6000_BTM_FRE, false, false }, - { "fres", RS6000_BTM_FRES, false, false }, - { "frsqrte", RS6000_BTM_FRSQRTE, false, false }, - { "frsqrtes", RS6000_BTM_FRSQRTES, false, false }, - { "popcntd", RS6000_BTM_POPCNTD, false, false }, - { "cell", RS6000_BTM_CELL, false, false }, + { "fres", OPTION_MASK_PPC_GFXOPT, false, false }, + { "frsqrte", OPTION_MASK_PPC_GFXOPT, false, false }, + { "frsqrtes", OPTION_MASK_POPCNTB, false, false }, + { "popcntd", OPTION_MASK_POPCNTD, false, false }, + { "cell", OPTION_MASK_FPRND, false, false }, { "power8-vector", RS6000_BTM_P8_VECTOR, false, false }, { "power9-vector", RS6000_BTM_P9_VECTOR, false, false }, { "power9-misc", RS6000_BTM_P9_MISC, false, false }, { "crypto", RS6000_BTM_CRYPTO, false, false }, { "htm", RS6000_BTM_HTM, false, false }, - { "hard-dfp", RS6000_BTM_DFP, false, false }, - { "hard-float", RS6000_BTM_HARD_FLOAT, false, false }, - { "long-double-128", RS6000_BTM_LDBL128, false, false }, - { "powerpc64", RS6000_BTM_POWERPC64, false, false }, - { "float128", RS6000_BTM_FLOAT128, false, false }, - { "float128-hw", RS6000_BTM_FLOAT128_HW,false, false }, - { "mma", RS6000_BTM_MMA, false, false }, - { "power10", RS6000_BTM_P10, false, false }, + { "hard-dfp", OPTION_MASK_DFP, false, false }, + { "hard-float", OPTION_MASK_SOFT_FLOAT, false, false }, + { "long-double-128", OPTION_MASK_MULTIPLE, false, false }, + { "powerpc64", MASK_POWERPC64, false, false }, + { "float128", OPTION_MASK_FLOAT128_KEYWORD, false, false }, + { "float128-hw", OPTION_MASK_FLOAT128_HW,false, false }, + { "mma", OPTION_MASK_MMA, false, false }, + { "power10", OPTION_MASK_POWER10, false, false }, }; /* Option variables that we want to support inside attribute((target)) and #pragma GCC target operations. */ diff --git a/gcc/config/rs6000/rs6000.h b/gcc/config/rs6000/rs6000.h index 2ff17a16e43c..384c5f1599a5 100644 --- a/gcc/config/rs6000/rs6000.h +++ b/gcc/config/rs6000/rs6000.h @@ -2251,24 +2251,10 @@ extern int frame_pointer_needed; #define RS6000_BTM_P9_VECTOR MASK_P9_VECTOR /* ISA 3.0 vector. */ #define RS6000_BTM_P9_MISC MASK_P9_MISC /* ISA 3.0 misc. non-vector */ #define RS6000_BTM_CRYPTO MASK_CRYPTO /* crypto funcs. */ #define RS6000_BTM_HTM MASK_HTM /* hardware TM funcs. */ #define RS6000_BTM_FRE MASK_POPCNTB /* FRE instruction. */ -#define RS6000_BTM_FRES MASK_PPC_GFXOPT /* FRES instruction. */ -#define RS6000_BTM_FRSQRTE MASK_PPC_GFXOPT /* FRSQRTE instruction. */ -#define RS6000_BTM_FRSQRTES MASK_POPCNTB /* FRSQRTES instruction. */ -#define RS6000_BTM_POPCNTD MASK_POPCNTD /* Target supports ISA 2.06. */ -#define RS6000_BTM_CELL MASK_FPRND /* Target is cell powerpc. */ -#define RS6000_BTM_DFP MASK_DFP /* Decimal floating point. */ -#define RS6000_BTM_HARD_FLOAT MASK_SOFT_FLOAT /* Hardware floating point. */ -#define RS6000_BTM_LDBL128 MASK_MULTIPLE /* 128-bit long double. */ -#define RS6000_BTM_64BIT MASK_64BIT /* 64-bit addressing. */ -#define RS6000_BTM_POWERPC64 MASK_POWERPC64 /* 64-bit registers. */ -#define RS6000_BTM_FLOAT128 MASK_FLOAT128_KEYWORD /* IEEE 128-bit float. */ -#define RS6000_BTM_FLOAT128_HW MASK_FLOAT128_HW /* IEEE 128-bit float h/w. */ -#define RS6000_BTM_MMA MASK_MMA /* ISA 3.1 MMA. */ -#define RS6000_BTM_P10 MASK_POWER10 enum rs6000_builtin_type_index { RS6000_BTI_NOT_OPAQUE,