Re: [Qemu-devel] [PATCH RFC v4 01/12] target/rx: TCG translation

[Yoshinori Sato]

On Tue, 26 Mar 2019 00:50:53 +0900,
Richard Henderson wrote:
> 
> On 3/25/19 2:38 AM, Yoshinori Sato wrote:
> >>> +static bool trans_EMUL_mr(DisasContext *ctx, arg_EMUL_mr *a)
> >>> +{
> >>> +    TCGv val, mem;
> >>> +    mem = tcg_temp_new();
> >>> +    val = rx_load_source(ctx, mem, a->ld, a->mi, a->rs);
> >>> +    tcg_gen_muls2_i32(cpu_regs[a->rd], cpu_regs[a->rd + 1],
> >>> +                      cpu_regs[a->rd], val);
> >>
> >> Both of these need to check for rd == 15 and return false.
> > 
> > Specifying 15 for rd does not result in an invalid instruction.
> > I made it to output a log.
> 
> Does the hardware wrap around and modify R0, then?
> Whatever you do, you cannot allow the guest to read
> beyond the end of the cpu_regs array.

R0 is not changed either.
As there is no flag change, it seems to be treated as nop.

> >>> +    gen_set_label(l1);
> >>> +    tcg_gen_movi_i32(cpu_psw_c, 0);
> >>> +    tcg_gen_movi_i32(cpu_psw_o, 0);
> >>
> >> Likewise.
> > 
> > As this is a two-operand, rd does not change when the shift count is zero.
> 
> The insn may be two operand, but the arg structure is three operand.  You have
> written the entire rest of the function as if rd is relevant.  If you add the
> tcg_gen_mov_i32 you complete that illusion.  However, there will be no mov
> issued, because tcg_gen_mov_i32 checks for dest == src.

This instruction have rs and rd.
This instruction is defined as follows,
cpu_regs[rd] <<= cpu_regs[rs];

so there is no need to transfer src to dst.

If the shift count is an immediate value, it becomes a 3-operand,
so a copy is necessary if it is 0.

And since there was a bit mask missing here, this problem is fixed.
> 
> 
> r~
> 

-- 
Yosinori Sato