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

[LIU Zhiwei <zhiwei_liu@linux.alibaba.com>]

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~