This is the mail archive of the
mailing list for the GCC project.
Re: [PATCH], Patch #10, move PowerPC data structures & helper functions from rs6000.c to rs6000-internal.h
On Mon, Jul 22, 2019 at 02:59:39PM -0400, Michael Meissner wrote:
> On Mon, Jul 22, 2019 at 12:42:13AM -0500, Segher Boessenkool wrote:
> > On Sat, Jul 20, 2019 at 12:13:08PM -0400, Michael Meissner wrote:
> > > 2019-07-20 Michael Meissner <email@example.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.
> This was a mechanical patch, just moving the swath of code from rs6000.c to
And yet, your changelog should be correct ;-)
> So in change, I can go back to just:
> #define RELOAD_REG_VALID 0x01 /* Mode valid in register.. */
> #define RELOAD_REG_MULTIPLE 0x02 /* Mode takes multiple registers. */
> #define RELOAD_REG_INDEXED 0x04 /* Reg+reg addressing. */
> #define RELOAD_REG_OFFSET 0x08 /* Reg+offset addressing. */
> #define RELOAD_REG_PRE_INCDEC 0x10 /* PRE_INC/PRE_DEC valid. */
> #define RELOAD_REG_PRE_MODIFY 0x20 /* PRE_MODIFY valid. */
> #define RELOAD_REG_AND_M16 0x40 /* AND -16 addressing. */
> #define RELOAD_REG_QUAD_OFFSET 0x80 /* quad offset is limited. */
Yes. Just keep it as is. This is supposed to be a move, not a change.
> Some time later if I continue with the current code, that would be changed to:
Maybe. We'll see then.
> I was just trying to save a step since I was moving the definitions any way to
> add the alignment.
A move should not introduce any unnecessary changes. This only is much
more work for everyone (you: you need to write extra changelog for it.
Me: I see there are extra changes, and now I have to check everything
> > 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?
> The intention is to move bits to other files. In particular, I want to create
> a new rs6000-prefixed.c file that contains the bits specific to prefixed
That isn't a good split. All addressing-related stuff in one file might
make sense, just like the rs6000-call.c split did (which was worthwhile
because all the huge builtin stuff naturally fit there as well). But
only one kind of addressing? Nope, that does not sound good.
> However, longer term, it would be nice to move other things from rs6000.c to
> other files, such as the functions that deal with p8 fusion, and the legitimate
> address functions (which I still tend to mentally classify as GO_IF_LEGITIMATE
> type functions, even as you mention, we no longer have GO_IF_LEGITIMATE*).
But only where that makes sense. Dividing things is only good if a)
the division has an obvious, fundamental meaning; b) the interface
between that part and the rest is thin; c) there is something to actually
*win* from dividing things.
> > 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.
> This was intended to be just a mechanical patch to move things to the .h file.
That still needs an explanation: why is this a good thing, why do you
want that change? Sometimes that is obvious of course, but here it is
not. It would be a lot more obvious if there was more context.