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~
