Re: [PATCH v3 09/14] RISC-V: Adding T-Head MemIdx extension

[Richard Henderson <richard.henderson@linaro.org>]

On 1/24/23 09:59, Christoph Muellner wrote:
> +/* XTheadMemIdx */
> +
> +/*
> + * Load with memop from indexed address and add (imm5 << imm2) to rs1.
> + * If !preinc, then the load address is rs1.
> + * If  preinc, then the load address is rs1 + (imm5) << imm2).
> + */
> +static bool gen_load_inc(DisasContext *ctx, arg_th_meminc *a, MemOp memop,
> +                         bool preinc)
> +{
> +    TCGv rd = dest_gpr(ctx, a->rd);
> +    TCGv addr = get_address(ctx, a->rs1, preinc ? a->imm5 << a->imm2 : 0);
> +
> +    tcg_gen_qemu_ld_tl(rd, addr, ctx->mem_idx, memop);
> +    addr = get_address(ctx, a->rs1, !preinc ? a->imm5 << a->imm2 : 0);

First, you're leaking the previous 'addr' temporary.
Second, get_address may make modifications to 'addr' which you don't want to write back.
Third, you are not checking for rd != rs1.

I think what you want is

     int imm = a->imm5 << a->imm2;
     TCGv addr = get_address(ctx, a->rs1, preinc ? imm : 0);
     TCGv rd = dest_gpr(ctx, a->rd);
     TCGv rs1 = get_gpr(ctx, a->rs1, EXT_NONE);

     tcg_gen_qemu_ld_tl(rd, addr, ctx->mem_idx, memop);
     tcg_gen_addi_tl(rs1, rs1, imm);
     gen_set_gpr(ctx, a->rd, rd);
     gen_set_gpr(ctx, a->rs1, rs1);


r~