Re: [PATCH, rs6000] Clean up the option_mask defines (part 1)

[will schmidt <will_schmidt@vnet.ibm.com>]

On Thu, 2022-05-26 at 05:47 -0500, Segher Boessenkool wrote:
> Hi!
> 

Hi, 
Thanks Kewen and Segher for the reviews.  Additional comments below.


> On Thu, May 26, 2022 at 03:01:37PM +0800, Kewen.Lin wrote:
> > on 2022/5/26 14:12, Kewen.Lin via Gcc-patches wrote:
> > > on 2022/5/26 04:25, will schmidt via Gcc-patches wrote:
> > > > We have an assortment of MASK and OPTION_MASK #defines
> > > > throughout
> > > > the rs6000 code, MASK_ALTIVEC and OPTION_MASK_ALTIVEC as an
> > > > example.
> > > > 
> > > > We currently #define the MASK_<xxxx> entries to their
> > > > OPTION_MASK_<xxxx>
> > > > equivalents so the two names could be used interchangeably.
> > > > 
> > > > The mapping is in place from when we switched from using
> > > > target_flags to rs6000_isa_flags via
> > > > commit 4d9675496a28ef6184f2a9c3ac5e6e3ea63606c1 in 2012.
> > > > 
> > > > This patch converts the references for most of the lingering
> > > > MASK_*
> > > > values to OPTION_MASK_*  and removes the now redundant defines.
> > > 
> > > Nice, thanks for the cleanup!
> 
> +1
> 
> > > I found there are still some masks left:
> > > 
> > > MASK_POWERPC64, MASK_64BIT and MASK_LITTLE_ENDIAN.
> > > 
> > > Is there one part 4 for them?  Or is there some particular reason
> > > not to clean up them?
> > 
> > aha, I see.  Those three are conditional definitions, I agree it's
> > better
> > to leave them alone. :)
> 
> It is much better to untangle this mess, and fix it :-)  But that is
> (potentially) a bigger job, of course, so let's not balloon this
> patch.

Right.  I have looked briefly at those, and was not convinced those
three would be trivial to rework.  In the interest if incremental
progress I didn't address those in this set.   :-)   If anything I'll
address those in a later patch, whether could be part4 but more likely
a different patchset.

> 
> > > > -    { "970", "ppc970", MASK_PPC_GPOPT | MASK_MFCRF |
> > > > MASK_POWERPC64 },
> > > > +    { "970", "ppc970", OPTION_MASK_PPC_GPOPT |
> > > > OPTION_MASK_MFCRF | MASK_POWERPC64 },
> > 
> > Nit: This line is too long.
> 
Yup, I missed that one. :-)

> Yeah, the longer names are a bit annoying in any case.  We'll get
> used
> to it (if those long lines are fixed ;-) )

Agree.  I would not be opposed to somewhat shorter names for these, but
naming is hard, and the long names are existing and sufficient for the
moment.

> 
> > Nit: Some of these BTM lines below exceed 80 characters, a few
> > already existed
> > previously.
> 
> Yes, and it is easily avoidable in this case.  Most of these comments
> have no content at all, and the rest could just be on separate lines.
> 
> But, are those builtin masks still used at all?  Can't we just use
> the
> option masks where they still are?  The builtins do not use them
> anymore :-)

They are still referenced in rs6000_builtin_mask_calculate() function,
which is used to assign a value to rs6000_builtin_mask, which is still
in use.  I had not yet dug deeper there, but agree it appears that is
only used to print the current options, so could probably be safely
eliminated.  I'll dig a bit more, but would handle that in a separate
patch.

Thanks
-Will


> 
> 
> Segher