Re: [PATCH v2 04/21] target/riscv: Introduce DisasExtend and new helpers

[Bin Meng]

On Wed, Aug 18, 2021 at 5:22 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Introduce get_gpr, dest_gpr, temp_new -- new helpers that do not force
> tcg globals into temps, returning a constant 0 for $zero as source and
> a new temp for $zero as destination.
>
> Introduce ctx->w for simplifying word operations, such as addw.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/riscv/translate.c | 102 +++++++++++++++++++++++++++++++--------
>  1 file changed, 82 insertions(+), 20 deletions(-)
>
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index d540c85a1a..d5cf5e5826 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -39,15 +39,25 @@ static TCGv load_val;
>
>  #include "exec/gen-icount.h"
>
> +/*
> + * If an operation is being performed on less than TARGET_LONG_BITS,
> + * it may require the inputs to be sign- or zero-extended; which will
> + * depend on the exact operation being performed.
> + */
> +typedef enum {
> +    EXT_NONE,
> +    EXT_SIGN,
> +    EXT_ZERO,
> +} DisasExtend;
> +
>  typedef struct DisasContext {
>      DisasContextBase base;
>      /* pc_succ_insn points to the instruction following base.pc_next */
>      target_ulong pc_succ_insn;
>      target_ulong priv_ver;
> -    bool virt_enabled;
> +    target_ulong misa;
>      uint32_t opcode;
>      uint32_t mstatus_fs;
> -    target_ulong misa;
>      uint32_t mem_idx;
>      /* Remember the rounding mode encoded in the previous fp instruction,
>         which we have already installed into env->fp_status.  Or -1 for
> @@ -55,6 +65,8 @@ typedef struct DisasContext {
>         to any system register, which includes CSR_FRM, so we do not have
>         to reset this known value.  */
>      int frm;
> +    bool w;
> +    bool virt_enabled;
>      bool ext_ifencei;
>      bool hlsx;
>      /* vector extension */
> @@ -64,7 +76,10 @@ typedef struct DisasContext {
>      uint16_t vlen;
>      uint16_t mlen;
>      bool vl_eq_vlmax;
> +    uint8_t ntemp;
>      CPUState *cs;
> +    TCGv zero;
> +    TCGv temp[4];

Why is 4? Is it enough? Perhaps a comment here is needed here?

>  } DisasContext;
>
>  static inline bool has_ext(DisasContext *ctx, uint32_t ext)
> @@ -172,27 +187,64 @@ static void gen_goto_tb(DisasContext *ctx, int n, 
> target_ulong dest)
>      }
>  }
>
> -/* Wrapper for getting reg values - need to check of reg is zero since
> - * cpu_gpr[0] is not actually allocated
> +/*
> + * Wrappers for getting reg values.
> + *
> + * The $zero register does not have cpu_gpr[0] allocated -- we supply the
> + * constant zero as a source, and an uninitialized sink as destination.
> + *
> + * Further, we may provide an extension for word operations.
>   */
> -static void gen_get_gpr(DisasContext *ctx, TCGv t, int reg_num)
> +static TCGv temp_new(DisasContext *ctx)
>  {
> -    if (reg_num == 0) {
> -        tcg_gen_movi_tl(t, 0);
> -    } else {
> -        tcg_gen_mov_tl(t, cpu_gpr[reg_num]);
> -    }
> +    assert(ctx->ntemp < ARRAY_SIZE(ctx->temp));
> +    return ctx->temp[ctx->ntemp++] = tcg_temp_new();
>  }
>
> -/* Wrapper for setting reg values - need to check of reg is zero since
> - * cpu_gpr[0] is not actually allocated. this is more for safety purposes,
> - * since we usually avoid calling the OP_TYPE_gen function if we see a write 
> to
> - * $zero
> - */
> -static void gen_set_gpr(DisasContext *ctx, int reg_num_dst, TCGv t)
> +static TCGv get_gpr(DisasContext *ctx, int reg_num, DisasExtend ext)
>  {
> -    if (reg_num_dst != 0) {
> -        tcg_gen_mov_tl(cpu_gpr[reg_num_dst], t);
> +    TCGv t;
> +
> +    if (reg_num == 0) {
> +        return ctx->zero;
> +    }
> +
> +    switch (ctx->w ? ext : EXT_NONE) {
> +    case EXT_NONE:
> +        return cpu_gpr[reg_num];
> +    case EXT_SIGN:
> +        t = temp_new(ctx);
> +        tcg_gen_ext32s_tl(t, cpu_gpr[reg_num]);
> +        return t;
> +    case EXT_ZERO:
> +        t = temp_new(ctx);
> +        tcg_gen_ext32u_tl(t, cpu_gpr[reg_num]);
> +        return t;
> +    }
> +    g_assert_not_reached();
> +}
> +
> +static void gen_get_gpr(DisasContext *ctx, TCGv t, int reg_num)
> +{
> +    tcg_gen_mov_tl(t, get_gpr(ctx, reg_num, EXT_NONE));
> +}
> +
> +static TCGv __attribute__((unused)) dest_gpr(DisasContext *ctx, int reg_num)
> +{
> +    if (reg_num == 0 || ctx->w) {
> +        return temp_new(ctx);
> +    }
> +    return cpu_gpr[reg_num];
> +}
> +
> +static void gen_set_gpr(DisasContext *ctx, int reg_num, TCGv t)
> +{
> +    if (reg_num != 0) {
> +        if (ctx->w) {
> +            tcg_gen_ext32s_tl(cpu_gpr[reg_num], t);

What about zero extension?

> +        } else {
> +            tcg_gen_mov_tl(cpu_gpr[reg_num], t);
> +        }
>      }
>  }
>
> @@ -927,8 +979,11 @@ static void riscv_tr_init_disas_context(DisasContextBase 
> *dcbase, CPUState *cs)
>      ctx->cs = cs;
>  }
>
> -static void riscv_tr_tb_start(DisasContextBase *db, CPUState *cpu)
> +static void riscv_tr_tb_start(DisasContextBase *dcbase, CPUState *cpu)
>  {
> +    DisasContext *ctx = container_of(dcbase, DisasContext, base);
> +
> +    ctx->zero = tcg_constant_tl(0);

This is better to be done in riscv_tr_init_disas_context() where ctx
members are initialized.

>  }
>
>  static void riscv_tr_insn_start(DisasContextBase *dcbase, CPUState *cpu)
> @@ -946,6 +1001,13 @@ static void riscv_tr_translate_insn(DisasContextBase 
> *dcbase, CPUState *cpu)
>
>      decode_opc(env, ctx, opcode16);
>      ctx->base.pc_next = ctx->pc_succ_insn;
> +    ctx->w = false;
> +
> +    for (int i = ctx->ntemp - 1; i >= 0; --i) {
> +        tcg_temp_free(ctx->temp[i]);
> +        ctx->temp[i] = NULL;
> +    }
> +    ctx->ntemp = 0;
>
>      if (ctx->base.is_jmp == DISAS_NEXT) {
>          target_ulong page_start;
> @@ -997,7 +1059,7 @@ static const TranslatorOps riscv_tr_ops = {
>
>  void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
>  {
> -    DisasContext ctx;
> +    DisasContext ctx = { };

Why is this change? I believe we should explicitly initialize the ctx
in riscv_tr_init_disas_context()

>
>      translator_loop(&riscv_tr_ops, &ctx.base, cs, tb, max_insns);
>  }
> --

Regards,
Bin