[PATCH] Rs6000 infrastructure cleanup (switches), revised patch #4

[Michael Meissner]

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?

-- 
Michael Meissner, IBM
5 Technology Place Drive, M/S 2757, Westford, MA 01886-3141, USA
meissner@linux.vnet.ibm.com	fax +1 (978) 399-6899