This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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  <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.
> 
> This was a mechanical patch, just moving the swath of code from rs6000.c to
> rs6000-internal.h.

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
else, too).

> > 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
> addressing.

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.


Segher


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]