Hi! On Sat, Jul 20, 2019 at 12:13:08PM -0400, Michael Meissner wrote: > 2019-07-20 Michael Meissner <meiss...@linux.ibm.com> > > * config/rs6000/rs6000-internal.h (rs6000_hard_regno_mode_ok_p): > Move various declarations relating to addressing and register > allocation to rs6000-internal.h from rs6000.c so that in the > future we can move things out of rs6000.c.
Just say (rs6000_hard_regno_mode_ok_p): New declaration. for the things that only had a definition before. > Make the static arrays global, That's not this entry. Say that in the entries where it applies. > and define them in rs6000.c. Say that in the corresponding entry for rs6000.c . > (enum rs6000_reg_type): Likewise. This one always was a declaration. (... ten gazillion "Likewise." ...) Most of those are *not* the same thing. Don't say "likewise" if not the same comment applies. If it is hard to write a proper changelog, your patch series probably could use some restructuring. Or sometimes the changelog you need just is more work than you would prefer. You don't necessarily have to keep the same order in the changelog as in the patch, if that helps. But roughly the same order helps review, so please consider that too ;-) > +/* Simplfy register classes into simpler classifications. We assume (Typo, not new, but still a typo :-) ) > +/* Register classes we care about in secondary reload or go if legitimate > + address. We only need to worry about GPR, FPR, and Altivec registers > here, > + along an ANY field that is the OR of the 3 register classes. */ We haven't had GO_IF_LEGITIMATE_ADDRESS for ten years now, please don't introduce new references to it ;-) > +#define RELOAD_REG_VALID 0x0001 /* Mode valid in register.. */ > +#define RELOAD_REG_MULTIPLE 0x0002 /* Mode takes multiple registers. */ > +#define RELOAD_REG_INDEXED 0x0004 /* Reg+reg addressing. */ > +#define RELOAD_REG_OFFSET 0x0008 /* Reg+offset addressing. */ > +#define RELOAD_REG_PRE_INCDEC 0x0010 /* PRE_INC/PRE_DEC valid. */ > +#define RELOAD_REG_PRE_MODIFY 0x0020 /* PRE_MODIFY valid. */ > +#define RELOAD_REG_AND_M16 0x0040 /* AND -16 addressing. */ > +#define RELOAD_REG_QUAD_OFFSET 0x0080 /* quad offset is limited. */ Why all the extra zeroes? If you introduce some 0x100 later, just leave the 0x80 as 0x80 please, that is much more readable. It's hard to tell whether the problem is factored sanely, or if this creates a big mountain of spaghetti instead. Can you show how this is used later? Normally, you send a whole series, and then perhaps many of the first are preparatory only, but a reviewer can see where things are headed, and *then* simple refactorings like this can make sense. The way this patch looks now you are just making a lot of data global. Segher
