Hi Philippe, thank you for the feedback.

> > diff --git a/tcg/tcg.c b/tcg/tcg.c
> > index dfd48b8264..154a4dafa7 100644
> > --- a/tcg/tcg.c
> > +++ b/tcg/tcg.c
> > @@ -136,6 +136,10 @@ static void tcg_out_goto_tb(TCGContext *s, int
which);
> >   static void tcg_out_op(TCGContext *s, TCGOpcode opc, TCGType type,
> >                          const TCGArg args[TCG_MAX_OP_ARGS],
> >                          const int const_args[TCG_MAX_OP_ARGS]);
> > +#if defined(EMSCRIPTEN)
>
> Maybe we can let this independently of EMSCRIPTEN, to reduce #ifdef'ry.

Sure, I'll make this independent of EMSCRIPTEN in the next version of the
series.

> > +static void tcg_out_label_cb(TCGContext *s, TCGLabel *l);
> > +static int tcg_out_tb_end(TCGContext *s);
> > +#endif
> >   #if TCG_TARGET_MAYBE_vec
> >   static bool tcg_out_dup_vec(TCGContext *s, TCGType type, unsigned
vece,
> >                               TCGReg dst, TCGReg src);
> > @@ -251,7 +255,7 @@ TCGv_env tcg_env;
> >   const void *tcg_code_gen_epilogue;
> >   uintptr_t tcg_splitwx_diff;
> >
> > -#ifndef CONFIG_TCG_INTERPRETER
> > +#if !defined(CONFIG_TCG_INTERPRETER) && !defined(EMSCRIPTEN)
>
> s/&&/||/ otherwise breaks TCI? (various cases)

Thank you for pointing this out. Let me break it down with a table to
clarify this logic.

                  | !defined(CONFIG_TCG_INTERPRETER) | !defined(EMSCRIPTEN)
| && | || |
------------------+----------------------------------+----------------------+----+----+
non-emcc + TCI    |              F                   |           T
 | F  |  T |
non-emcc + non-TCI|              T                   |           T
 | T  |  T |
emcc + TCI        |              F                   |           F
 | F  |  F |
emcc + wasm       |              T                   |           F
 | F  |  T |

If we use "||", this condition becomes always true for non-emscripten
builds, regardless of whether TCI is used. That would break TCI's intended
behaviour. On emscripten builds, both of TCI and wasm backends define
tcg_qemu_tb_exec on their own so the declaration here is unnecessary. This
aligns with the behaviour of "&&", not "||". So unless I'm missing
something, "&&" seems to match the intent?

That said, this condition is a bit complex. Parhaps we shoud introduce a
clearer flag (e.g., HAS_TCG_QEMU_TB_EXEC) in tcg-target.h?

> Out of curiosity, have you tried to run a big-endian guest?

I've just tried a s390x guest by booting Linux. It worked after applying a
small bugfix (unrelated to endianness, shown below). I'll include this fix
in the next verison of the patch series.

> @@ -1718,6 +1718,7 @@ static void tcg_wasm_out_sub2_i64(TCGContext *s,
TCGReg retl, TCGReg reth,
>          tcg_wasm_out_op_local_get(s, TMP64_LOCAL_0_IDX);
>          tcg_wasm_out_op_global_get_r(s, al);
>          tcg_wasm_out_op_i64_gt_u(s);
> +        tcg_wasm_out_op_i64_extend_i32_s(s);
>
>          tcg_wasm_out_op_local_get(s, TMP64_LOCAL_0_IDX);
>          tcg_wasm_out_op_global_set_r(s, retl);
> @@ -1727,6 +1728,7 @@ static void tcg_wasm_out_sub2_i64(TCGContext *s,
TCGReg retl, TCGReg reth,
>          tcg_wasm_out_op_global_get_r(s, retl);
>          tcg_wasm_out_op_global_get_r(s, al);
>          tcg_wasm_out_op_i64_gt_u(s);
> +        tcg_wasm_out_op_i64_extend_i32_s(s);
>      }

Reply via email to