Re: Remove global state from gcc/tracer.c

[Richard Henderson <rth at redhat dot com>]

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~