| Message ID | 20121013002119.GA8991@ibm-tiger.the-meissners.org |
|---|---|
| State | New |
| Headers | show |
On Fri, 12 Oct 2012, Michael Meissner wrote: > I decided to see if it was possible to simplify the change over by adding > another flag word in the .opt handling to give the old names (TARGET_<xxx> and > MASK_<xxx>). For Joseph Myers and Neil Booth, the issue is when changing all > of the switches that use Mask(xxx) and InverseMask(xxx) to also use Var(xxx), > the option machinery changes the names of the macros to OPTION_<xxx> and > OPTION_MASK_<xxx>, which in turn causes lots and lots of changes for patch > review. Some can't be omitted, where we referred to the 'target_flags' and > 'target_flags_explicit' fields, but at least it reduces the number of other > changes. I think changing the names is safer - it's immediately obvious as a build failure if you missed anything. If you have MASK_* names for bits in more than one flags variable, there's a risk of accidentally testing a bit in the wrong variable, or ORing together bits that belong in different variables in a way that can't possibly work, without this causing immediately visible problems. Maybe you're actually only using the names for a single variable, but it still seems error-prone for future changes. I guess TARGET_* names should be safe in a way that MASK_* ones aren't for multiple variables - but then I wouldn't have options to do things two different ways, but instead use TARGET_* instead of OPTION_* and fix existing uses of OPTION_* for such bits. I don't know if with C++ it's possible to keep the names the same *and* ensure that compile time errors occur if bits from different variables are used together or a bit is used with the wrong variable *and* avoid any other issues occurring as a consequence of such changes.
On Mon, Oct 15, 2012 at 03:52:01PM +0000, Joseph S. Myers wrote: > On Fri, 12 Oct 2012, Michael Meissner wrote: > > > I decided to see if it was possible to simplify the change over by adding > > another flag word in the .opt handling to give the old names (TARGET_<xxx> and > > MASK_<xxx>). For Joseph Myers and Neil Booth, the issue is when changing all > > of the switches that use Mask(xxx) and InverseMask(xxx) to also use Var(xxx), > > the option machinery changes the names of the macros to OPTION_<xxx> and > > OPTION_MASK_<xxx>, which in turn causes lots and lots of changes for patch > > review. Some can't be omitted, where we referred to the 'target_flags' and > > 'target_flags_explicit' fields, but at least it reduces the number of other > > changes. > > I think changing the names is safer - it's immediately obvious as a build > failure if you missed anything. If you have MASK_* names for bits in more > than one flags variable, there's a risk of accidentally testing a bit in > the wrong variable, or ORing together bits that belong in different > variables in a way that can't possibly work, without this causing > immediately visible problems. Maybe you're actually only using the names > for a single variable, but it still seems error-prone for future changes. Well to be safest, we should have a prefix for each word if you define more than one flag word. Preferably a name that the user can specify in the .opt file. > I guess TARGET_* names should be safe in a way that MASK_* ones aren't for > multiple variables - but then I wouldn't have options to do things two > different ways, but instead use TARGET_* instead of OPTION_* and fix > existing uses of OPTION_* for such bits. I can see the MASK/OPTION_MASK thing, but not having TARGET_* means there are lots and lots of code changes. Unfortunately in order to bring the number of changes down to a point where the patches can be reviewed, my previous patches did: #define TARGET_FOO OPTION_FOO #define MASK_FOO OPTION_MASK_FOO > I don't know if with C++ it's possible to keep the names the same *and* > ensure that compile time errors occur if bits from different variables are > used together or a bit is used with the wrong variable *and* avoid any > other issues occurring as a consequence of such changes. I would like a way to delete the target_flags field if we don't define any flags using it (it would affect the pch stuff that preserves and checks the target_flags). David and I have talked about moving to accessor macros. I'm thinking of something like: mfoo Target Report Mask(FOO) SetFunction ExplicitFunction TargetName If TargetName were defined, it would use TARGET_<xxx> instead of OPTION_<xxx>, but the OPTION_MASK_<xxx> would not be changed. If SetFunction was defined, the opt*.awk files would generate: #define SET_FOO(VALUE) \ do { \ if (VALUE) \ target_flags &= ~MASK_FOO; \ else \ target_flags |= MASK_FOO; \ } while (0) If ExplicitFunction was defined, the opt*.awk files would generate: #define EXPLICIT_FOO(VALUE) \ ((global_options_set.x_target_flags & MASK_FOO) != 0) And then I would change options a few at a time. When I've converted all of the options, I would then go back to adding the Var(yyy) options, but the SET_<xxx> and EXPLICIT_<xxx> would not change (or it could key off of TargetName). How would you feel about SetFunction, ExplicitFunction, and the reduced TargetName?
On Mon, 15 Oct 2012, Michael Meissner wrote: > > I think changing the names is safer - it's immediately obvious as a build > > failure if you missed anything. If you have MASK_* names for bits in more > > than one flags variable, there's a risk of accidentally testing a bit in > > the wrong variable, or ORing together bits that belong in different > > variables in a way that can't possibly work, without this causing > > immediately visible problems. Maybe you're actually only using the names > > for a single variable, but it still seems error-prone for future changes. > > Well to be safest, we should have a prefix for each word if you define more > than one flag word. Preferably a name that the user can specify in the .opt > file. Yes, for MASK_*. > > I guess TARGET_* names should be safe in a way that MASK_* ones aren't for > > multiple variables - but then I wouldn't have options to do things two > > different ways, but instead use TARGET_* instead of OPTION_* and fix > > existing uses of OPTION_* for such bits. > > I can see the MASK/OPTION_MASK thing, but not having TARGET_* means there are > lots and lots of code changes. > > Unfortunately in order to bring the number of changes down to a point where the > patches can be reviewed, my previous patches did: > > #define TARGET_FOO OPTION_FOO > #define MASK_FOO OPTION_MASK_FOO The first of those #defines might be an intermediate step towards actually replacing OPTION_FOO by TARGET_FOO everywhere (since there seems to be no actual need for the different naming convention there, only for the masks). But I don't really think we should delay the mechanical replacement much (changing all OPTION_* that aren't OPTION_MASK_* to be TARGET_* should be a straightforward change to make automatically, although a pain to review the results of that substitution so maybe best kept in a separate patch from one doing anything more substantive). That is: 1. Patch adding TARGET_FOO aliases for OPTION_FOO (small change to the awk scripts and associated documentation, I expect). 2. Large, mechanical, automatically generated patch to change existing OPTION_FOO users (or maybe one such patch per target). 3. Patch removing the OPTION_FOO name (small change to awk scripts and documentation). Then you've eliminated one unnecessary cause of changes when moving bits out of target_flags. > If TargetName were defined, it would use TARGET_<xxx> instead of OPTION_<xxx>, > but the OPTION_MASK_<xxx> would not be changed. Not needed, given the above sequence of changes. > If SetFunction was defined, the opt*.awk files would generate: > > #define SET_FOO(VALUE) \ > do { \ > if (VALUE) \ > target_flags &= ~MASK_FOO; \ > else \ > target_flags |= MASK_FOO; \ > } while (0) > > If ExplicitFunction was defined, the opt*.awk files would generate: > > #define EXPLICIT_FOO(VALUE) \ > ((global_options_set.x_target_flags & MASK_FOO) != 0) I'd like any such new macros to take an argument that's the pointer to the relevant options structure (global_options, global_options_set). If the place where the macro is called has a pointer available, then it can be passed in, otherwise pass in &global_options or &global_options_set unless and until such a pointer becomes available in the relevant place. > How would you feel about SetFunction, ExplicitFunction, and the reduced > TargetName? The principle of having macros for setting flags or testing if they are explicitly set is fine, though it's not clear to me that they need any such special settings as SetFunction and ExplicitFunction (rather than being generated unconditionally). I'd quite like the macros such as target_flags that refer to global options to end up not being lvalues at all. That helps ensure that option settings are only modified in limited places that have options pointers. It would be nice eventually for such things as "optimize" and "target" attributes to be able to swap options structures, and to work closer to how options on the command line are processed - for that, you want careful control on what places actually modify options at all.
On Tue, Oct 16, 2012 at 03:02:47PM +0000, Joseph S. Myers wrote: > That is: > > 1. Patch adding TARGET_FOO aliases for OPTION_FOO (small change to the awk > scripts and associated documentation, I expect). > > 2. Large, mechanical, automatically generated patch to change existing > OPTION_FOO users (or maybe one such patch per target). I just grep'ed for OPTION_, filtering out OPTION_MASK_, TARGET_OPTION_OVERRIDE, OPTION_DEFAULT_SPECS_*, OPTION_GLIBC, OPTION_UCLIBC, OPTION_BIONIC, TARGET_OPTION_SAVE, TARGET_OPTION_RESTORE, TARGET_OPTION_PRINT, OPTION_TARGET_CPU_DEFAULT, TARGET_OPTION_VALID_ATTRIBUTE_P, _SPEC[\" ], MIPS_ARCH_*, TARGET_OPTION_*, RS6000_CPU_OPTION_NATIVE, and there is only one place where OPTION_* is used as a test (config/linux-android.h). The only other port to do OPTION_* is x86, and there they have a bunch of #defines that map OPTION_<xxx> into TARGET_<xxx>. So it looks fairly straight forward to do the conversion in one jump. > 3. Patch removing the OPTION_FOO name (small change to awk scripts and > documentation). > > Then you've eliminated one unnecessary cause of changes when moving bits > out of target_flags. > > > If TargetName were defined, it would use TARGET_<xxx> instead of OPTION_<xxx>, > > but the OPTION_MASK_<xxx> would not be changed. > > Not needed, given the above sequence of changes. Yep, I would prefer not to have to add TargetName, though it is simple enough. > > If SetFunction was defined, the opt*.awk files would generate: > > > > #define SET_FOO(VALUE) \ > > do { \ > > if (VALUE) \ > > target_flags &= ~MASK_FOO; \ > > else \ > > target_flags |= MASK_FOO; \ > > } while (0) > > > > If ExplicitFunction was defined, the opt*.awk files would generate: > > > > #define EXPLICIT_FOO(VALUE) \ > > ((global_options_set.x_target_flags & MASK_FOO) != 0) > > I'd like any such new macros to take an argument that's the pointer to the > relevant options structure (global_options, global_options_set). If the > place where the macro is called has a pointer available, then it can be > passed in, otherwise pass in &global_options or &global_options_set unless > and until such a pointer becomes available in the relevant place. It occurs to me that now that we've committed to GCC being done in C++, we could just make global_options{,_set} be a class instead of a structure. So you could say: global_options.set_FOO (value) Or: global_options.set_FOO (); global_options.clear_FOO (); I could generate the macros (or inline functions) if you would prefer to stick the C style of doing things. However, as an old C dinosaur, I'm not sure of all of the ramifications of doing this. It just seems it would be cleaner to use the class structure, instead of passing pointers. > > How would you feel about SetFunction, ExplicitFunction, and the reduced > > TargetName? > > The principle of having macros for setting flags or testing if they are > explicitly set is fine, though it's not clear to me that they need any > such special settings as SetFunction and ExplicitFunction (rather than > being generated unconditionally). Yes, it is simpler not to have to add another flag. I was just trying to be conservative in generating things other ports might not reference. > I'd quite like the macros such as target_flags that refer to global > options to end up not being lvalues at all. That helps ensure that option > settings are only modified in limited places that have options pointers. > It would be nice eventually for such things as "optimize" and "target" > attributes to be able to swap options structures, and to work closer to > how options on the command line are processed - for that, you want careful > control on what places actually modify options at all. Yep, though unfortunately that are various ports that do want to change optimization options if not set.
On Tue, 16 Oct 2012, Michael Meissner wrote: > It occurs to me that now that we've committed to GCC being done in C++, we > could just make global_options{,_set} be a class instead of a structure. So > you could say: > > global_options.set_FOO (value) > > Or: > > global_options.set_FOO (); > global_options.clear_FOO (); > > I could generate the macros (or inline functions) if you would prefer to stick > the C style of doing things. However, as an old C dinosaur, I'm not sure of > all of the ramifications of doing this. It just seems it would be cleaner to > use the class structure, instead of passing pointers. In general, as much as possible should use an instance of struct gcc_options that is passed explicitly to the relevant code (or associated with the function being compiled, etc.), rather than using global_options directly (explicitly or implicitly). The existing way of doing that is using a pointer to a gcc_options structure. With a class I'd think you'd still need to pass it around as either a pointer or a reference (even if you then use member functions for some operations on these structures), and I'm not aware of any particular advantage of using a reference. I do not think most functions that happen to take a gcc_options pointer (often along with lots of other pointers to other pieces of state) are particularly suited to being member functions of gcc_options. Given that existing practice is passing pointers around, I'd think that's appropriate for any new such functions / macros, unless and until we have some clear notion of when functionality should or should not be a member function of gcc_options.
On Wed, Oct 17, 2012 at 12:26:42AM +0000, Joseph S. Myers wrote: > On Tue, 16 Oct 2012, Michael Meissner wrote: > > > It occurs to me that now that we've committed to GCC being done in C++, we > > could just make global_options{,_set} be a class instead of a structure. So > > you could say: > > > > global_options.set_FOO (value) > > > > Or: > > > > global_options.set_FOO (); > > global_options.clear_FOO (); > > > > I could generate the macros (or inline functions) if you would prefer to stick > > the C style of doing things. However, as an old C dinosaur, I'm not sure of > > all of the ramifications of doing this. It just seems it would be cleaner to > > use the class structure, instead of passing pointers. > > In general, as much as possible should use an instance of struct > gcc_options that is passed explicitly to the relevant code (or associated > with the function being compiled, etc.), rather than using global_options > directly (explicitly or implicitly). > > The existing way of doing that is using a pointer to a gcc_options > structure. With a class I'd think you'd still need to pass it around as > either a pointer or a reference (even if you then use member functions for > some operations on these structures), and I'm not aware of any particular > advantage of using a reference. I do not think most functions that happen > to take a gcc_options pointer (often along with lots of other pointers to > other pieces of state) are particularly suited to being member functions > of gcc_options. > > Given that existing practice is passing pointers around, I'd think that's > appropriate for any new such functions / macros, unless and until we have > some clear notion of when functionality should or should not be a member > function of gcc_options. In thinking about it this morning, I don't think we need the options machinery to generate new functions. We can just use the set_option function in opts-common.c to set those options. I likely will add a convenience function in rs6000.c to default most of the arguments for this.
Index: gcc/opth-gen.awk =================================================================== --- gcc/opth-gen.awk (revision 192400) +++ gcc/opth-gen.awk (working copy) @@ -332,10 +332,9 @@ for (i = 0; i < n_opts; i++) { if (name != "" && mask_bits[name] == 0) { mask_bits[name] = 1 vname = var_name(flags[i]) - mask = "MASK_" + mask = opt_prefix_mask(flags[i]) mask_1 = "1" if (vname != "") { - mask = "OPTION_MASK_" if (host_wide_int[vname] == "yes") mask_1 = "HOST_WIDE_INT_1" } else @@ -375,14 +374,13 @@ for (i = 0; i < n_opts; i++) { if (name != "" && mask_macros[name] == 0) { mask_macros[name] = 1 vname = var_name(flags[i]) - macro = "OPTION_" - mask = "OPTION_MASK_" + macro = opt_prefix_target(name) + mask = opt_prefix_mask(name) if (vname == "") { vname = "target_flags" - macro = "TARGET_" - mask = "MASK_" extra_mask_macros[name] = 1 } + print "#define " macro name \ " ((" vname " & " mask name ") != 0)" } @@ -398,13 +396,12 @@ for (i = 0; i < n_opts; i++) { opt = opt_args("InverseMask", flags[i]) if (opt ~ ",") { vname = var_name(flags[i]) - macro = "OPTION_" - mask = "OPTION_MASK_" + macro = opt_prefix_target(flags[i]) + mask = opt_prefix_mask(flags[i]) if (vname == "") { vname = "target_flags" - macro = "TARGET_" - mask = "MASK_" } + print "#define " macro nth_arg(1, opt) \ " ((" vname " & " mask nth_arg(0, opt) ") == 0)" } Index: gcc/opt-functions.awk =================================================================== --- gcc/opt-functions.awk (revision 192400) +++ gcc/opt-functions.awk (working copy) @@ -242,18 +242,14 @@ function var_set(flags) s = opt_args("Mask", flags); if (s != "") { vn = var_name(flags); - if (vn) - return "0, CLVC_BIT_SET, OPTION_MASK_" s - else - return "0, CLVC_BIT_SET, MASK_" s + mask = opt_prefix_mask(flags) + return "0, CLVC_BIT_SET, " mask s } s = nth_arg(0, opt_args("InverseMask", flags)); if (s != "") { vn = var_name(flags); - if (vn) - return "0, CLVC_BIT_CLEAR, OPTION_MASK_" s - else - return "0, CLVC_BIT_CLEAR, MASK_" s + mask = opt_prefix_mask(flags) + return "0, CLVC_BIT_CLEAR, " mask s } if (flag_set_p("Enum.*", flags)) { en = opt_args("Enum", flags); @@ -297,3 +293,25 @@ function lang_sanitized_name(name) gsub( "[^" alnum "_]", "X", name ) return name } + +# Given the option called NAME which is a mask argument, return the prefix for +# the option (i.e. TARGET_ or OPTION_). +function opt_prefix_target(name) +{ + variable = var_name(name) + if (variable == "" || flag_set_p("TargetName", name)) + return "TARGET_"; + else + return "OPTION_"; +} + +# Given the option called NAME which is a mask argument, return the prefix for +# the mask name (i.e. MASK_ or OPTION_MASK_). +function opt_prefix_mask(name) +{ + variable = var_name(name) + if (variable == "" || flag_set_p("TargetName", name)) + return "MASK_"; + else + return "OPTION_MASK_"; +} Index: gcc/doc/options.texi =================================================================== --- gcc/doc/options.texi (revision 192400) +++ gcc/doc/options.texi (working copy) @@ -339,12 +339,14 @@ You may also specify @code{Var} to selec @code{target_flags}. The options-processing script will automatically allocate a unique bit -for the option. If the option is attached to @samp{target_flags}, -the script will set the macro @code{MASK_@var{name}} to the appropriate -bitmask. It will also declare a @code{TARGET_@var{name}} macro that has -the value 1 when the option is active and 0 otherwise. If you use @code{Var} -to attach the option to a different variable, the associated macros are -called @code{OPTION_MASK_@var{name}} and @code{OPTION_@var{name}} respectively. +for the option. If the option is attached to @samp{target_flags}, the +script will set the macro @code{MASK_@var{name}} to the appropriate +bitmask. It will also declare a @code{TARGET_@var{name}} macro that +has the value 1 when the option is active and 0 otherwise. If you use +@code{Var} to attach the option to a different variable and do not +used the @code{TargetName} opiton, the associated macros are called +@code{OPTION_MASK_@var{name}} and @code{OPTION_@var{name}} +respectively. @item InverseMask(@var{othername}) @itemx InverseMask(@var{othername}, @var{thisname}) @@ -472,4 +474,10 @@ format}. @item NoDWARFRecord The option is omitted from the producer string written by @option{-grecord-gcc-switches}. + +@item TargetName +If the option was declared with @code{Mask} or @code{InverseMask} and +also @code{Var} to specify an alternative flag word, the +@code{TARGET_@var{name}} and @code{MASK_@var{name}} macros will be +created of @code{OPTION_@var{name}} and @code{OPTION_MASK_@var{name}}. @end table