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

[LIU Zhiwei]

On 2023/1/25 5:21, Richard Henderson wrote:
> 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.
Although all temps allocated by temp_new() will be freed after the 
instruction translation automatically, we can improve current 
implementation.
> Second, get_address may make modifications to 'addr' which you don't 
> want to write back.
Good catch.
> Third, you are not checking for rd != rs1.
Yes.
> 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);

Yes, we should write back the 'addr' without modification.

Best Regards,
Zhiwei

> r~