Re: [PATCH], Patch #10, move PowerPC data structures & helper functions from rs6000.c to rs6000-internal.h

[Segher Boessenkool <segher@kernel.crashing.org>]

Hi!

On Sat, Jul 20, 2019 at 12:13:08PM -0400, Michael Meissner wrote:
> 2019-07-20  Michael Meissner  <meissner@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