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