Re: [PATCH v3 25/48] tcg: Add insn_start_words to TCGContext

[Anton Johansson]

On 5/31/23 06:03, Richard Henderson wrote:
> This will enable replacement of TARGET_INSN_START_WORDS in tcg.c.
> Split out "tcg/insn-start-words.h" and use it in target/.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   include/tcg/insn-start-words.h | 17 +++++++++++++++++
>   include/tcg/tcg-op.h           |  8 ++++----
>   include/tcg/tcg-opc.h          |  6 +++---
>   include/tcg/tcg.h              |  9 ++-------
>   accel/tcg/perf.c               |  8 ++++++--
>   accel/tcg/translate-all.c      | 13 ++++++++-----
>   target/i386/helper.c           |  2 +-
>   target/openrisc/sys_helper.c   |  2 +-
>   tcg/tcg.c                      | 16 +++++++++++-----
>   9 files changed, 53 insertions(+), 28 deletions(-)
>   create mode 100644 include/tcg/insn-start-words.h
>
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index 594f1db1a7..7cff2c5915 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -64,6 +64,7 @@
>   #include "tb-context.h"
>   #include "internal.h"
>   #include "perf.h"
> +#include "tcg/insn-start-words.h"
>   
>   /* Make sure all possible CPU event bits fit in tb->trace_vcpu_dstate */
>   QEMU_BUILD_BUG_ON(CPU_TRACE_DSTATE_MAX_EVENTS >
> @@ -132,19 +133,20 @@ static int64_t decode_sleb128(const uint8_t **pp)
>   static int encode_search(TranslationBlock *tb, uint8_t *block)
>   {
>       uint8_t *highwater = tcg_ctx->code_gen_highwater;
> +    uint64_t *insn_data = tcg_ctx->gen_insn_data;
>       uint8_t *p = block;
>       int i, j, n;
>   
>       for (i = 0, n = tb->icount; i < n; ++i) {
>           uint64_t prev;
>   
> -        for (j = 0; j < TARGET_INSN_START_WORDS; ++j) {
> +        for (j = 0; j < TARGET_INSN_START_WORDS; ++j, ++insn_data) {
>               if (i == 0) {
>                   prev = (!(tb_cflags(tb) & CF_PCREL) && j == 0 ? tb->pc : 0);
>               } else {
> -                prev = tcg_ctx->gen_insn_data[i - 1][j];
> +                prev = insn_data[-TARGET_INSN_START_WORDS];
>               }
> -            p = encode_sleb128(p, tcg_ctx->gen_insn_data[i][j] - prev);
> +            p = encode_sleb128(p, *insn_data - prev);
>           }
Maybe just personal preference, but I would have written

    prev = tcg_ctx->gen_insn_data[(i-1)*TARGET_INSN_START_WORDS + j]

and similarly for encode_sleb128(...) as i think it's easier to see 
what's going on.
Not used to seeing negative array indices so had to do a double take :)

But I assume we get better asm using insn_data?

Otherwise:
Reviewed-by: Anton Johansson <anjo@rev.ng>