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
