Re: [RFC PATCH 3/8] accel/tcg: skip the can_do_io stores in user-only builds

On 17/8/26 21:00, Matt Turner wrote:
> Every translation block stores to cpu->neg.can_do_io twice: false before
> the first instruction, true before the last one. Nothing reads it in a
> user-only build. There is no memory-mapped I/O in linux-user, and every
> reader is in system_ss: cputlb.c, watchpoint.c, icount-common.c and
> tcg-accel-ops-icount.c.
> 
> Two stores per TB is not much on its own, but TBs are short. An emulated
> alpha gcc 16.2.0 compiling the SQLite 3.45.1 amalgamation (255k lines,
> -O2) executes 34.2 billion TBs at 6.04 guest instructions each, so this is
> 68 billion stores for nothing.
> 
> Measured on an x86-64 host, LTO build, on top of the preceding two
> patches:
> 
>      before: 1,469,729,281,442 instructions
>      after:  1,402,816,253,499 instructions   -4.55%
> 
>      before: 120.97s wall clock
>      after:  115.75s wall clock              -4.32%

Nice.

> The emulated compiler produces byte-identical output.
> 
> Signed-off-by: Matt Turner <[email protected]>
> ---
>   accel/tcg/translator.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git ./accel/tcg/translator.c ./accel/tcg/translator.c
> index cd7d079fe0..29e609b2ec 100644
> --- ./accel/tcg/translator.c
> +++ ./accel/tcg/translator.c
> @@ -21,12 +21,14 @@
>   #include "disas/disas.h"
>   #include "tb-internal.h"
>   
> +#ifndef CONFIG_USER_ONLY
>   static void set_can_do_io(DisasContextBase *db, bool val)
>   {
>       QEMU_BUILD_BUG_ON(sizeof_field(CPUState, neg.can_do_io) != 1);
>       tcg_gen_st8_i32(tcg_constant_i32(val), tcg_env,
>                       offsetof(CPUState, neg.can_do_io) - sizeof(CPUState));
>   }
> +#endif

I'm a bit reluctant to clutter translator.c with #ifdef'ry (in
particular when we are not protecting system-specific API, so
this can be evaluated at runtime).
We could have a tcg_user_emulation() or user_mode_emulation()
helper hidding the #ifdef; or could move set_can_do_io() to
accel/tcg/translate-all.c, or maybe others have better idea.
The overall change LGTM otherwise.

>   
>   bool translator_io_start(DisasContextBase *db)
>   {
> @@ -210,17 +212,25 @@ void translator_loop(CPUState *cpu, TranslationBlock *tb, int *max_insns,
>       /*
>        * Manage can_do_io for the translation block: set to false before
>        * the first insn and set to true before the last insn.
> +     *
> +     * Nothing reads can_do_io in user-only builds.  There is no MMIO
> +     * there, and every reader (cputlb.c, watchpoint.c, icount) is in
> +     * system_ss, so skip the two stores per TB entirely.
>        */
>       if (db->num_insns == 1) {
>           tcg_debug_assert(first_insn_start == db->insn_start);
>       } else {
>           tcg_debug_assert(first_insn_start != db->insn_start);
> +#ifndef CONFIG_USER_ONLY
>           tcg_ctx->emit_before_op = first_insn_start;
>           set_can_do_io(db, false);
> +#endif
>       }
> +#ifndef CONFIG_USER_ONLY
>       tcg_ctx->emit_before_op = db->insn_start;
>       set_can_do_io(db, true);
>       tcg_ctx->emit_before_op = NULL;
> +#endif
>   
>       /* May be used by disas_log or plugin callbacks. */
>       tb->size = db->pc_next - db->pc_first;