On 05/23/2013 03:56 AM, David Malcolm wrote: > 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.
Plausible. Another thing I should mention while you're doing all of these static function to class member conversions is that as written we're losing target-specific optimizations that can be done on purely local functions. This is trivially fixed by placing these new classes in an anonymous namespace. > +private: > + > + /* Minimal outgoing edge probability considered for superblock > + formation. */ > + STATEFUL int probability_cutoff; > + STATEFUL int branch_ratio_cutoff; > + > + /* A bit BB->index is set if BB has already been seen, i.e. it is > + connected to some trace already. */ > + STATEFUL sbitmap bb_seen; ... > +#if GLOBAL_STATE > +/* Global definitions of static data. */ > +int tracer_state::probability_cutoff; > +int tracer_state::branch_ratio_cutoff; > +sbitmap tracer_state::bb_seen; > +#endif ... > +tracer_state::tracer_state() > +#if !GLOBAL_STATE > + : probability_cutoff(0), > + branch_ratio_cutoff(0), > + bb_seen() > +#endif > +{ > +} > + What I don't like about this arrangement is that it requires three repetitions of the state variables and their initializations. I wonder if we can do better with class tracer_state { private: struct data { int probability_cutoff; int branch_ratio_cutoff; sbitmap bb_seen; data() : probability_cutoff(0), branch_ratio_cutoff(0) bb_seen() { } }; STATEFUL data d; public: tracer_state() { } ... }; #if GLOBAL_STATE tracer_state::data tracer_state::d; #endif which does require accesses as "d.foo" instead of just foo, but at least we've cut down on the boilerplate. Though with this I wonder if we need a CONSTEXPR define to markup tracer_state::data::data so that the global variable doesn't wind up running a constructor at runtime? (I.e. performs correctly if inefficiently for stage1, but stage[23] use g++ and thus can have the c++11 extension?) > #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 */ I've an idea that this will perform very badly. With ctxt being a global (if tls) variable, it's call clobbered. Thus we'll wind up reloading it all the time, which for pic involves another call to the __get_tlsaddr runtime function. For a very heavily used pointers, we're almost certainly better off having the data be referenced via "this", where at least the starting point is known to be function invariant. r~