Hi!
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.
> >> - { "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.
Yeah, the longer names are a bit annoying in any case. We'll get used
to it (if those long lines are fixed ;-) )
> 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 :-)
Segher
