Re: [PATCH v3 08/14] RISC-V: Adding T-Head MemPair extension

[LIU Zhiwei]

On 2023/1/25 4:44, Richard Henderson wrote:
> On 1/24/23 09:59, Christoph Muellner wrote:
> > +static bool gen_loadpair_tl(DisasContext *ctx, arg_th_pair *a, MemOp 
> > memop,
> > +                            int shamt)
> > +{
> > +    TCGv rd1 = dest_gpr(ctx, a->rd1);
> > +    TCGv rd2 = dest_gpr(ctx, a->rd2);
> > +    TCGv addr1 = tcg_temp_new();
> > +    TCGv addr2 = tcg_temp_new();
> > +
> > +    addr1 = get_address(ctx, a->rs, a->sh2 << shamt);
> > +    if ((memop & MO_SIZE) == MO_64) {
> > +        addr2 = get_address(ctx, a->rs, 8 + (a->sh2 << shamt));
> > +    } else {
> > +        addr2 = get_address(ctx, a->rs, 4 + (a->sh2 << shamt));
> > +    }
> > +
> > +    tcg_gen_qemu_ld_tl(rd1, addr1, ctx->mem_idx, memop);
> > +    tcg_gen_qemu_ld_tl(rd2, addr2, ctx->mem_idx, memop);
> > +    gen_set_gpr(ctx, a->rd1, rd1);
> > +    gen_set_gpr(ctx, a->rd2, rd2);
>
> Since dest_gpr may return cpu_gpr[n], this may update the rd1 before 
> recognizing the exception that the second load may generate.  Is that 
> correct?

Thanks. It's a bug. We should load all memory addresses to  local TCG  
temps first.

Do you think we should probe all the memory addresses for the store pair 
instructions? If so, can we avoid the use of a helper function?

> The manual says that rd1, rd2, and rs1 must not be the same, but you 
> do not check this.

The main reason is that assembler can do this check. Is it necessary to 
check this in QEMU?

Best Regards,
Zhiwei

> r~