Patchwork RFC: Putting target-dependent global state into switchable structures

login
register
mail settings
Submitter Richard Sandiford
Date June 26, 2010, 7:24 p.m.
Message ID <87pqzdegwc.fsf@firetop.home>
Download mbox | patch
Permalink /patch/57062/
State New
Headers show

Comments

Richard Sandiford - June 26, 2010, 7:24 p.m.
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.
Mark Mitchell - June 26, 2010, 8:02 p.m.
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,
Joseph S. Myers - June 26, 2010, 8:08 p.m.
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.
Richard Guenther - June 26, 2010, 10:15 p.m.
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
>
Richard Guenther - June 26, 2010, 10:19 p.m.
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.
Joseph S. Myers - June 26, 2010, 10:25 p.m.
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.)
Richard Guenther - June 26, 2010, 10:36 p.m.
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 Sandiford - June 27, 2010, 7:30 a.m.
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
Richard Sandiford - July 4, 2010, 9:46 p.m.
"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

Patch

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;