| Message ID | 87pqzdegwc.fsf@firetop.home |
|---|---|
| State | New |
| Headers | show |
Richard Sandiford wrote: > I've got a series of patches that speed up the switching process by > avoiding target_reinit. Rather than spam the list with the entire > series now, I thought I'd better ask for feedback on the general approach. I think this is entirely sensible. It's a step towards a more modular compiler, and we all know that we need that. I'll be happy to help review the patches as necessary once you're ready. Thanks,
On Sat, 26 Jun 2010, Richard Sandiford wrote: > The approach I went for is very similar to what we already we for > cfun and crtl. That is, we have structures of the form: > > type1 x_field1; > ... > typeN x_fieldN; > > and the current versions of the variables are then accessed using macros > such as: > > #define field1 (...->x_field1) > ... > #define fieldN (...->x_fieldN) > > I wanted to make it as easy as possible to add more global variables > (caching data is good!) so I ended up describing them in special .def files. > A new generator program, gentarget-globals, then produces the actual > C code. > > Also, I didn't want to pessimise the majority of targets by adding > extra pointer chasing where it wasn't needed. If a target doesn't > want to switch subtargets -- the default assumption -- then the > macros above actually have the form: > > #define field1 (global_state.x_field1) > ... > #define fieldN (global_state.x_fieldN) I intend to do something similar for variables set by command-line options. That is, I intend to put them in a "struct gcc_options" or similar and #define the present variable names to reference fields of a global_options structure. I don't plan to make the "target" and "optimize" attributes use switched copies of such structures (or of the relevant substructures), though that would be a plausible next step. (Right now those attributes have generated functions that save optimization flags in a cl_optimization structure with each flag going in one byte. Switching pointers to different structures and only creating such structures when needed should certainly be more efficient.) In more detail, I see option processing as going in the following stages, where the gcc_options structure is restricted to integer/enum/string/... state that can readily be used in both the driver and the compilers proper: 1a. Initialize gcc_options structure. 1b. Handle each option in turn, updating the structure as appropriate. 1c. Finalize the structure (determine settings once all options have been handled). 2. Configure other compiler state (only in the compilers proper, not in the driver); this might include the state that you are putting in your subtarget structures. "target" and "optimize" attributes would logically take a copy of the structure saved after step 1b, process their options (the logical effect should be as if the options had gone on the end of the command line), and reapply steps 1c and 2 to the copy of the options structure used and the relevant subtarget structures, causing those copies to be used for the function to which the attributes are applied.
On Sat, Jun 26, 2010 at 10:02 PM, Mark Mitchell <mark@codesourcery.com> wrote: > Richard Sandiford wrote: > >> I've got a series of patches that speed up the switching process by >> avoiding target_reinit. Rather than spam the list with the entire >> series now, I thought I'd better ask for feedback on the general approach. > > I think this is entirely sensible. It's a step towards a more modular > compiler, and we all know that we need that. I'll be happy to help > review the patches as necessary once you're ready. Well. I don't like the idea of using .def files and a generator program. In fact we are happily using no .def files for target hooks. So why not have a target-independent piece of the structure definition and a target macro implementing a target specific piece in it. Thus, sth like struct globals { ... TARGET_GLOBALS; }; and targets can define that if they need target specific globals. Using .def files is over-engineering here IMHO and I do not see any advantage in using it. Richard. > Thanks, > > -- > Mark Mitchell > CodeSourcery > mark@codesourcery.com > (650) 331-3385 x713 >
On Sun, Jun 27, 2010 at 12:15 AM, Richard Guenther <richard.guenther@gmail.com> wrote: > On Sat, Jun 26, 2010 at 10:02 PM, Mark Mitchell <mark@codesourcery.com> wrote: >> Richard Sandiford wrote: >> >>> I've got a series of patches that speed up the switching process by >>> avoiding target_reinit. Rather than spam the list with the entire >>> series now, I thought I'd better ask for feedback on the general approach. >> >> I think this is entirely sensible. It's a step towards a more modular >> compiler, and we all know that we need that. I'll be happy to help >> review the patches as necessary once you're ready. > > Well. I don't like the idea of using .def files and a generator program. > In fact we are happily using no .def files for target hooks. > > So why not have a target-independent piece of the structure definition > and a target macro implementing a target specific piece in it. Thus, > sth like > > struct globals { > ... > TARGET_GLOBALS; > }; > > and targets can define that if they need target specific globals. > > Using .def files is over-engineering here IMHO and I do not see any > advantage in using it. Btw, I'm happy with the general idea and using cfun/crtl like accessors. Just I hate the idea of yet another bunch of .def files and another generator program where I see no advantage. Richard.
On Sun, 27 Jun 2010, Richard Guenther wrote: > On Sat, Jun 26, 2010 at 10:02 PM, Mark Mitchell <mark@codesourcery.com> wrote: > > Richard Sandiford wrote: > > > >> I've got a series of patches that speed up the switching process by > >> avoiding target_reinit. Rather than spam the list with the entire > >> series now, I thought I'd better ask for feedback on the general approach. > > > > I think this is entirely sensible. It's a step towards a more modular > > compiler, and we all know that we need that. I'll be happy to help > > review the patches as necessary once you're ready. > > Well. I don't like the idea of using .def files and a generator program. > In fact we are happily using no .def files for target hooks. I believe we're waiting for build system and middle-end review of Joern's patch <http://gcc.gnu.org/ml/gcc-patches/2010-06/msg01863.html> to introduce just such a .def file. (If that can get in, Joern should then produce the patch showing the fixed body of existing hook documentation we want to license under both GPL and GFDL, for RMS to review; he can deal with relicensing requests for fixed concrete pieces of text, where the licenses in question already exist, reasonably efficiently.)
On Sun, Jun 27, 2010 at 12:25 AM, Joseph S. Myers <joseph@codesourcery.com> wrote: > On Sun, 27 Jun 2010, Richard Guenther wrote: > >> On Sat, Jun 26, 2010 at 10:02 PM, Mark Mitchell <mark@codesourcery.com> wrote: >> > Richard Sandiford wrote: >> > >> >> I've got a series of patches that speed up the switching process by >> >> avoiding target_reinit. Rather than spam the list with the entire >> >> series now, I thought I'd better ask for feedback on the general approach. >> > >> > I think this is entirely sensible. It's a step towards a more modular >> > compiler, and we all know that we need that. I'll be happy to help >> > review the patches as necessary once you're ready. >> >> Well. I don't like the idea of using .def files and a generator program. >> In fact we are happily using no .def files for target hooks. > > I believe we're waiting for build system and middle-end review of Joern's > patch <http://gcc.gnu.org/ml/gcc-patches/2010-06/msg01863.html> to > introduce just such a .def file. (If that can get in, Joern should then > produce the patch showing the fixed body of existing hook documentation we > want to license under both GPL and GFDL, for RMS to review; he can deal > with relicensing requests for fixed concrete pieces of text, where the > licenses in question already exist, reasonably efficiently.) Ok. That doesn't make me like .def files for globals any more though. In fact it's exactly included once and the point of .def files is to avoid duplication of data or to put related data together. Richard.
Richard Guenther <richard.guenther@gmail.com> writes: > On Sat, Jun 26, 2010 at 10:02 PM, Mark Mitchell <mark@codesourcery.com> wrote: >> Richard Sandiford wrote: >> >>> I've got a series of patches that speed up the switching process by >>> avoiding target_reinit. Rather than spam the list with the entire >>> series now, I thought I'd better ask for feedback on the general approach. >> >> I think this is entirely sensible. It's a step towards a more modular >> compiler, and we all know that we need that. I'll be happy to help >> review the patches as necessary once you're ready. > > Well. I don't like the idea of using .def files and a generator program. > In fact we are happily using no .def files for target hooks. > > So why not have a target-independent piece of the structure definition > and a target macro implementing a target specific piece in it. Thus, > sth like > > struct globals { > ... > TARGET_GLOBALS; > }; > > and targets can define that if they need target specific globals. Were you thinking that the global structure ought to contain all the state, or just pointers to substructures? I didn't make it clear, but the patches do the latter. I wanted to avoid having a big "all our globals" structure that gets included pretty much everywhere, and that depends on pretty much all the rtl-leaning headers. E.g. it would need to know about libfuncs, optabs, HARD_REG_SET, etc. Having substructures seemed like a way of keeping a bit of modularity, and avoiding having to recompile everything if you just touch an optabs-related variable (say). > Using .def files is over-engineering here IMHO and I do not see any > advantage in using it. OK, I should probably have explained why I did that. One reason was that, at least with the implementation I went for, you had to write a fair bit of boilerplate to add a new group of options (sometimes due to the old "you can't #define a #define" problem). Because the main globals structure is just a collection of pointers, you had to: 1) add the substructure itself (obviously) 2) declare the default version of the substructure (default_foo) 3) declare a pointer to the current version of the substructure (this_foo) 4) arrange for default_foo to be used directly by the accessor macros on targets that don't need this feature 5) define the default version of the substructure (default_foo) 6) define the pointer to the current version of the substructure (this_foo) 7) add a pointer field to the main globals structure 8) add an assignment from this pointer field to this_foo in the function that restores a state 9) add a memory allocation call to the function that creates a new globals structure (1)-(4) are done in a particular target header, (5)-(6) are done in a C file, (7)-(8) are done in a common target header, and (9) is done in a common C file. Having a .def file meant you could do all these things from one place, and from one directive. Admittedly this could be simplified if we don't care about pessimising "ordinary" targets. The advantage of the .def file though is that we don't really need to worry. The scheme isn't that complicated in itself, and the .def file automates most of it. I was wanting to cut down the amount of work involved in adding groups, and also to make it easier to take a different (perhaps C++-inflected) approach later. Another reason is that I didn't want to change which data is garbage-collected and which isn't. As it stands, there's a lot of non-garbage-collected data, so you need a lot of ugly GTY((skip))s. Or to put it another way, you have to invert the way you look at gc, and say what _isn't_ gc-ed rather than what is. If we're effectively treating the fields as global variables (and we are), then I like the idea of using variable-style GTY markers, and leaving the generator to work things out. (Which is very easy to do.) I also wanted to avoid having to add a boilerplate #define for each field by hand, although I admit that's not a good enough reason on its own. I suppose at the end of the day though, it just doesn't look that ugly to me. ;-) Richard
"Joseph S. Myers" <joseph@codesourcery.com> writes: > I intend to do something similar for variables set by command-line > options. That is, I intend to put them in a "struct gcc_options" or > similar and #define the present variable names to reference fields of a > global_options structure. I don't plan to make the "target" and > "optimize" attributes use switched copies of such structures (or of the > relevant substructures), though that would be a plausible next step. > (Right now those attributes have generated functions that save > optimization flags in a cl_optimization structure with each flag going in > one byte. Switching pointers to different structures and only creating > such structures when needed should certainly be more efficient.) > > In more detail, I see option processing as going in the following stages, > where the gcc_options structure is restricted to integer/enum/string/... > state that can readily be used in both the driver and the compilers > proper: > > 1a. Initialize gcc_options structure. > > 1b. Handle each option in turn, updating the structure as appropriate. > > 1c. Finalize the structure (determine settings once all options have been > handled). > > 2. Configure other compiler state (only in the compilers proper, not in > the driver); this might include the state that you are putting in your > subtarget structures. > > "target" and "optimize" attributes would logically take a copy of the > structure saved after step 1b, process their options (the logical effect > should be as if the options had gone on the end of the command line), and > reapply steps 1c and 2 to the copy of the options structure used and the > relevant subtarget structures, causing those copies to be used for the > function to which the attributes are applied. Sounds good. Options handling is certainly one of the big missing pieces that my patches do nothing to solve. Both before and after the patches, the MIPS port needs to stash away various target flags in global variables, reinstate them for each switch, then override them based on the new target. The stashing is done in the middle of stage (1c) above and is very ad hoc. It would be great to have a way of switching the option state along with the target state. (The patch series does put derived values like align_loops_log in the target structure.) Richard
Index: gcc/config/mips/mips.h =================================================================== --- gcc/config/mips/mips.h 2010-06-26 19:06:40.000000000 +0100 +++ gcc/config/mips/mips.h 2010-06-26 19:51:54.000000000 +0100 @@ -3027,6 +3027,7 @@ struct mips_asm_switch { extern const struct mips_rtx_cost_data *mips_cost; extern bool mips_base_mips16; extern enum mips_code_readable_setting mips_code_readable; +extern GTY(()) struct target_globals *mips16_globals; #endif /* Enable querying of DFA units. */ @@ -3061,3 +3062,6 @@ #define FINAL_PRESCAN_INSN(INSN, OPVEC, support this feature. */ #define ASM_PREFERRED_EH_DATA_FORMAT(CODE,GLOBAL) \ (((GLOBAL) ? DW_EH_PE_indirect : 0) | DW_EH_PE_absptr) + +/* For switching between MIPS16 and non-MIPS16 modes. */ +#define SWITCHABLE_TARGET 1 Index: gcc/config/mips/mips.c =================================================================== --- gcc/config/mips/mips.c 2010-06-26 19:06:40.000000000 +0100 +++ gcc/config/mips/mips.c 2010-06-26 19:51:54.000000000 +0100 @@ -58,6 +58,7 @@ the Free Software Foundation; either ver #include "gimple.h" #include "bitmap.h" #include "diagnostic.h" +#include "target-globals.h" /* True if X is an UNSPEC wrapper around a SYMBOL_REF or LABEL_REF. */ #define UNSPEC_ADDRESS_P(X) \ @@ -568,6 +569,9 @@ static GTY (()) int mips_output_filename /* Likewise for HIGHs. */ static const char *mips_hi_relocs[NUM_SYMBOL_TYPES]; +/* Target state for MIPS16. */ +struct target_globals *mips16_globals; + /* Index R is the smallest register class that contains register R. */ const enum reg_class mips_regno_to_class[FIRST_PSEUDO_REGISTER] = { LEA_REGS, LEA_REGS, M16_REGS, V1_REG, @@ -15200,9 +15204,15 @@ mips_set_mips16_mode (int mips16_p) /* (Re)initialize MIPS target internals for new ISA. */ mips_init_relocs (); - if (was_mips16_p >= 0 || was_mips16_pch_p >= 0) - /* Reinitialize target-dependent state. */ - target_reinit (); + if (mips16_p) + { + if (!mips16_globals) + mips16_globals = save_target_globals (); + else + restore_target_globals (mips16_globals); + } + else + restore_target_globals (&default_target_globals); was_mips16_p = mips16_p; was_mips16_pch_p = mips16_p;
GCC has a fair number of global variables that cache target-dependent data. This makes it difficult to switch between subtargets on the fly, such as when switching between a MIPS16 and a non-MIPS16 function. Our current approach is to call target_reinit each time we make such a switch. This function goes off and redoes a fair chunk of the target initialisation process, and although it works (or least worked) pretty well, it is very slow. I've got a series of patches that speed up the switching process by avoiding target_reinit. Rather than spam the list with the entire series now, I thought I'd better ask for feedback on the general approach. Take the following very contrived example: int x = 1; #define B(X) \ void __attribute__((mips16, noinline)) X##_mips16 (void) { x++; } \ void __attribute__((nomips16, noinline)) X (void) { X##_mips16 (); } #define C(X) B(X##0) B(X##1) B(X##2) B(X##3) B(X##4) \ B(X##5) B(X##6) B(X##7) B(X##8) B(X##9) #define D(X) C(X##0) C(X##1) C(X##2) C(X##3) C(X##4) \ C(X##5) C(X##6) C(X##7) C(X##8) C(X##9) D(foo) (that's 100 non-MIPS16 functions and 100 MIPS16 functions). At the moment, a typical -O2 -g compilation takes: real 0m17.705s user 0m17.533s sys 0m0.172s After the patches it takes: real 0m0.336s user 0m0.324s sys 0m0.016s The generated code is the same. The approach I went for is very similar to what we already we for cfun and crtl. That is, we have structures of the form: type1 x_field1; ... typeN x_fieldN; and the current versions of the variables are then accessed using macros such as: #define field1 (...->x_field1) ... #define fieldN (...->x_fieldN) I wanted to make it as easy as possible to add more global variables (caching data is good!) so I ended up describing them in special .def files. A new generator program, gentarget-globals, then produces the actual C code. Also, I didn't want to pessimise the majority of targets by adding extra pointer chasing where it wasn't needed. If a target doesn't want to switch subtargets -- the default assumption -- then the macros above actually have the form: #define field1 (global_state.x_field1) ... #define fieldN (global_state.x_fieldN) The advantages I see of this approach are: - There's very little code churn. Most code that uses these variables isn't affected. - It puts all the target-dependent global state in separate files. At the moment, this state tends to be intermingled with target-independent or short-lived data, and it isn't always easy to see which is which. - It should be easier to make systematic changes to the global state in future. The main disadvantage I see is that it makes debugging a bit harder. My defence against that is: - It's no worse than what we already do for cfun and crtl. (A very weak argument at best.) - AIUI, you can avoid the problem by using macro debugging, although I've never tried it myself. I'm afraid it's also true that the new .def files are closer to syntactic vinegar than syntactic sugar. As a taster, I've attached four patches: - the main infrastructure patch - a mechanical and uninteresting patch to add HARD_REG_SET_H to Makefile.in - a patch to put some hard-reg-set.h variables in a .def file - the patch to make MIPS use this infrastructure Thoughts? Richard gcc/ * doc/tm.texi (SWITCHABLE_TARGET): Document. * Makefile.in (target_globals_def): New variable. (target_globals_h): Likewise. (TARGET_GLOBALS_H): Likewise. (OBJS-common): Add target-globals.o. (gtype-desc.o): Depend on $(TARGET_GLOBALS_H). (target-globals.o): New rule. ($(target_globals_h)): Likewise. (s-target-globals): Likewise. (GTFILES): Add $(target_globals_h). (build/gentarget-globals.o): New rule. * defaults.h (SWITCHABLE_TARGET): Define. * gengtype.c (open_base_files): Add target-globals.h to the include list. * target-globals.def: New file. * gentarget-globals.c: Likewise. * target-globals.c: Likewise. gcc/ * Makefile.in (HARD_REG_SET_H): New variable. Use it instead of hard-reg-set.h in all dependency lists. gcc/ * Makefile.in (HARD_REG_SET_H): Add target-hard-regs.h and $(TM_H). (target-globals.o): Depend on $(HARD_REG_SET_H). * target-hard-regs.def: New file. * target-globals.def: Include it. * target-globals.c: Include hard-reg-set.h. * hard-reg-set.h: Include target-hard-regs.h. (fixed_regs): Delete. (fixed_reg_set, call_used_regs, call_really_used_regs): Likewise. (call_used_reg_set, call_fixed_reg_set): Likewise. (regs_invalidated_by_call, no_caller_save_reg_set): Likewise. (reg_alloc_order, inv_reg_alloc_order, reg_class_contents): Likewise. (reg_class_size, reg_class_subclasses, reg_class_subunion): Likewise. (reg_class_superunion, reg_names, reg_class_names): Likewise. * reginfo.c (fixed_regs, fixed_reg_set, call_used_regs): Delete. (call_used_reg_set, call_really_used_regs): Likewise. (call_fixed_reg_set, regs_invalidated_by_call): Likewise. (reg_alloc_order, inv_reg_alloc_order, reg_class_contents): Likewise. (reg_class_size, reg_class_subclasses, reg_class_subunion): Likewise. (reg_class_superunion, reg_names, reg_class_names): Likewise. (initial_call_really_used_regs): New variable. (initial_reg_alloc_order): Likewise. (initial_reg_names, initial_reg_class_names): Likewise. (init_reg_sets): Assert that initial_call_really_used_regs, initial_reg_alloc_order, initial_reg_names and initial_reg_class_names are all the same size as their variable counterparts. Use them to initialize those counterparts. * caller-save.c (no_caller_save_reg_set): Delete. * ira.c (setup_reg_class_relations): Remove local reg_class_names. * config/mep/mep.c (reg_class_names): Delete commented-out definition. gcc/ * config/mips/mips.h (mips16_globals): Declare. (SWITCHABLE_TARGET): Define. * config/mips/mips.c: Include target-globals.h. (mips16_globals): New variable. (mips_set_mips16_mode): Use save_target_globals and restore_target_globals instead of target_reinit.