Remove global state from gcc/tracer.c

On Thu, 2013-05-23 at 11:59 -0400, David Malcolm wrote:
> On Thu, 2013-05-23 at 06:56 -0400, David Malcolm wrote:
> > On Thu, 2013-05-23 at 07:14 +0200, Jakub Jelinek wrote:
> > > On Wed, May 22, 2013 at 08:45:45PM -0400, David Malcolm wrote:
> > > > I'm attempting to eliminate global state from the insides of gcc.
> > > > 
> > > > gcc/tracer.c has various global variables, which are only used during
> > > > the lifetime of the execute callback of that pass, and cleaned up at the
> > > > end of each invocation of the pass.
> > > > 
> > > > The attached patch introduces a class to hold the state of the pass
> > > > ("tracer_state"), eliminating these globals.  An instance of the state
> > > > is created on the stack, and all of the various "static" functions in
> > > > tracer.c that used the globals become member functions of the state.
> > > > Hence the state is passed around by the implicit "this" of the
> > > > tracer_state, avoiding the need to patch each individual use of a field
> > > > within the state, minimizing the diff.
> > > 
> > > But do we want to handle the global state this way?  This adds overhead
> > > to (almost?) every single function (now method) in the file (because it gets
> > > an extra argument).  While that might be fine for rarely executed functions,
> > > if it involves also hot functions called many times, where especially on
> > > register starved hosts it could increase register pressure, plus the
> > > overhead of passing the this argument everywhere, this could start to be
> > > noticeable.  Sure, if you plan to do that just in one pass (but, why then?),
> > > it might be tiny slowdown, but after you convert the hundreds of passes in
> > > gcc that contain global state it might become significant.
> > > 
> > > There are alternative approaches that should be considered.
> > 
> > I thought of a possible way of doing this, attached is a
> > proof-of-concept attempt.
> > 
> > The idea is to use (and then not use) C++'s "static" syntax for class
> > methods and fields.  By making that optional with a big configure-time
> > switch, it gives us a way of making state be either global vs on-stack,
> > with minimal syntax changes.  In one configuration (for building gcc as
> > a library) there would be implicit this-> throughout, but in the other
> > (for speedy binaries) it would all compile away to global state, as per
> > the status quo.
> > 
> > This assumes that doing:
> > 
> >    tracer_state state;
> >    changed = state.tail_duplicate ();
> > 
> > is legitimate; when using global state, "state" is empty, and the call
> > to
> >   state.tail_duplicate ()
> > becomes effectively:
> >   state::tail_duplicate ()
> > since it's static in that configuration.
> > 
> > > E.g. global state of a pass can be moved into a per-pass structure,
> > > and have some way how to aggregate those per pass structures together from
> > > all the passes in the whole compiler (that can be either manual process,
> > > say each pass providing its own *-passstate.h and one big header including
> > > all that together), or automatic ones (say gengstate or a new tool could
> > > create those for us from special markings in the source, say new option on
> > > GTY or something) and have some magic macro how to access the global state
> > > within the pass (thispass->fieldname ?).  Then e.g. depending on how the
> > > compiler would be configured and built, thispass could be just address of a
> > > pass struct var (i.e. essentially keep the global state as is, for
> > > performance reasons), or when trying to build compiler as a library (with
> > > -fpic overhead we probably don't want for cc1/cc1plus - we can build all the
> > > *.o files twice, like libtool does) thispass could expand to __thread
> > > pointer var dereference plus a field inside of the global compiler state
> > > structure it points to for the current pass.  Thus, the library version
> > > of the compiler would be somewhat slower (both -fpic overhead and TLS
> > > overhead), and would need either a few of the entrypoints tweaked to adjust
> > > the TLS pointer to the global state, or we could require users to just call
> > > a special function to make the global state current in the current thread
> > > before calling compiler internals.
> > 
> > Thanks.   Though I thought we were trying to move away from relying on
> > GTY parsing?   (Sorry not to be able to answer more fully yet, need to
> > get family ready for school...)
> 
> I've warmed to your idea of having tooling to support state, and
> creating a generic framework for this.  For example, consider the
> (long-term) use-case of embedding GCC's code as a library inside a
> multithreaded app, where each thread could be JIT-compiling say OpenGL
> shader code to machine code (perhaps with some extra non-standard
> passes).  To get there, I'd need to isolate *all* of GGC's state, and
> when I look at, say, the garbage-collector, I shudder.
> 
> So I'm interested in writing some compile-time tooling for generic
> state-management in GCC, to sidestep the conflict between speed in the
> status quo single state case vs support for multiple states.
> 
> Here's a possible way of doing it:
> 
> Given e.g. gcc/foo.c containing some state:
>   static some_type bar;
> 
> then replace this with:
> 
>   DEFINE_STATE(some_type, bar);
> 
> which is trivial to parse, and have a preprocessing tool autogenerate a
> header in the builddir:
>    gcc/foo.c.state.h
> that conditionally has something like this:
> 
> #if SUPPORT_MULTIPLE_STATES
> struct foo_c_state
> {
>   some_type bar;
> };
> # define bar   ctxt->x_foo_c_bar;
> /* there's a    "context *ctxt;" variable somewhere, possibly
>    using TLS */
> 
> /* alternatively, we could do this as:
>     ctxt.x_foo_c_bar
>    and make changing context be an operation involving a memcpy, so the
>    single global ctxt gets its state overwritten by a copy.  */
> 
> #else /* the single state case */
> 
> /* Just a global, as before: */
> some_type bar;
> 
> #endif
> 
> That way, any code that uses "bar" has equal meaning (after
> preprocessing) in the !SUPPORT_MULTIPLE_STATES case to the status quo,
> and thus ought to retain the performance for that use case.
> 
> Probably need a DECLARE_STATE() macro also.
> 
> (I didn't want to reuse GTY() since there's plenty of global state that
> isn't GC-managed, and IIRC the plan is to move away from GTY)
> 
> I can try prototyping this, if this approach sounds reasonable.  Is this
> the sort of thing that would be best done as a branch? (e.g. a feature
> branch in git).
> 
> BTW, I'm not convinced that TLS is the way to go for isolating these
> details: what if someone wanted to parallelize a slow pass by
> introducing a thread pool, optimizing each function on a different
> thread?  I think it's reasonable to require a specific API call on the
> part of client code when changing context (OpenGL works this way iirc,
> though it *does* use TLS).  But I suppose that if the
> DECLARE/DEFINE_STATE macros are in place, it becomes easier to
> experiment with various implementations of state handling.
> 
> In a similar vein, would "universe" be a better name than "context"?
> "context" suggests threads to some people, whereas what I have in mind
> is the ability to have multiple clients of a future gcc-as-a-library,
> each client having the ability to have "parallel universes" of gcc
> state; some clients might spread themselves across several threads, but
> want to see the same universe.  If GTY/PCH etc are layered on top of the
> state management code, then clearly you can't share ptrs between states,
> and the "parallel universe" metaphor is perhaps clearer.
> 
> Hope this is constructive (and that I'm vaguely on the right track here)
> Dave

I wrote a crude prototype of this, and am attaching in patch form (which
includes a mix of handwritten and generated files for ease of seeing
what's going on).  It uses a Python script (gcc/genstate.py) to parse
the state from .c files (run by hand).

I converted tracer.c and ggc-page.c

It compiles, but doesn't yet run (I haven't written the initialization
code).

The universe type's fields are *pointers* to the underlying state
structs to avoid having it depend on every header file, and to avoid
having to expose previously-private types; the generated headers are
only used by their corresponding file.  All that's exposed of each state
struct is its sizeof, so that universe instances can malloc these at
startup.

It doesn't handle state that has initializers (e.g. the pass instances).

Dave

Remove global state from gcc/tracer.c

Commit Message

Comments

Patch