Re: [PATCH v9 03/13] accel: collecting JIT statistics

[Richard Henderson]

On 10/7/19 11:28 AM, Alex Bennée wrote:
> @@ -1795,6 +1799,10 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
>          if (flag & TB_EXEC_STATS) {
>              tb->tb_stats->stats_enabled |= TB_EXEC_STATS;
>          }
> +
> +        if (flag & TB_JIT_STATS) {
> +            tb->tb_stats->stats_enabled |= TB_JIT_STATS;
> +        }

So, assuming that you really meant this, and not replacing as I was guessing vs
patch 2, then this is

    tb->tb_stats->stats_enabled |=
        flag & (TB_EXEC_STATS | TB_JIT_STATS);

But I still think it's weird to be wanting to modify the shared structure.
What does that mean for concurrent code generation?

> +    /*
> +     * Collect JIT stats when enabled. We batch them all up here to
> +     * avoid spamming the cache with atomic accesses
> +     */
> +    if (tb_stats_enabled(tb, TB_JIT_STATS)) {
> +        TBStatistics *ts = tb->tb_stats;
> +        qemu_mutex_lock(&ts->jit_stats_lock);
> +
> +        ts->code.num_guest_inst += prof->translation.nb_guest_insns;
> +        ts->code.num_tcg_ops += prof->translation.nb_ops_pre_opt;
> +        ts->code.num_tcg_ops_opt += tcg_ctx->nb_ops;
> +        ts->code.spills += prof->translation.nb_spills;
> +        ts->code.out_len += tb->tc.size;
> +
> +        ts->translations.total++;
> +        if (phys_page2 != -1) {
> +            ts->translations.spanning++;
> +        }
> +
> +        g_ptr_array_add(ts->tbs, tb);
> +
> +        qemu_mutex_unlock(&ts->jit_stats_lock);
> +    }

Hmm.  So we're to interpret all of code.field as sums, the average of which can
be obtained by dividing by translations.total.  Ok, but that should probably be
mentioned in a comment in/near the structure definition.

What are we planning to do with the set of all tb's collected here?

> @@ -3125,6 +3126,7 @@ static void temp_sync
>          case TEMP_VAL_REG:
>              tcg_out_st(s, ts->type, ts->reg,
>                         ts->mem_base->reg, ts->mem_offset);
> +            s->prof.translation.nb_spills++;
>              break;
>  
>          case TEMP_VAL_MEM:

This is not a spill in the common compiler definition.

This is "write the temp to its backing storage".  While this does happen in the
course of spilling, the vast majority of these are because we've finished
modifying a global temp and must now update memory.  Which is not nearly the
same thing as "spill".

A spill in the compiler definition happens in tcg_reg_alloc, right after the
comment "We must spill something".  ;-)


r~