Re: [Qemu-devel] [PATCH 03/15] Openrisc: add instructions translation

[Max Filippov]

Hi.

I've got a couple of questions/suggestions regarding the code.

On Thu, May 17, 2012 at 12:35 PM, Jia Liu <address@hidden> wrote:
> add the openrisc instructions translation.
>
> Signed-off-by: Jia Liu <address@hidden>

[...]

> +    case 0x0009:
> +        switch (op1) {
> +        case 0x03:   /*l.div*/
> +            LOG_DIS("l.div r%d, r%d, r%d\n", rd, ra, rb);
> +            if (rb != 0) {
> +                tcg_gen_div_tl(cpu_R[rd], cpu_R[ra], cpu_R[rb]);

You also need to take care of integer overflow/division by zero here,
otherwise it may crash QEMU.

> +            } else {
> +                /* exception here */
> +            }
> +            break;
> +        default:
> +            break;
> +        }
> +        break;
> +
> +    case 0x000a:
> +        switch (op1) {
> +        case 0x03:   /*l.divu*/
> +            LOG_DIS("l.divu r%d, r%d, r%d\n", rd, ra, rb);
> +            if (rb != 0) {
> +                tcg_gen_divu_tl(cpu_R[rd], cpu_R[ra], cpu_R[rb]);
> +            } else {
> +                /* exception here */
> +            }
> +            break;
> +        default:
> +            break;
> +        }
> +        break;
> +
> +    case 0x000b:
> +        switch (op1) {
> +        case 0x03:   /*l.mulu*/
> +            LOG_DIS("l.mulu r%d, r%d, r%d\n", rd, ra, rb);
> +            if (rb != 0 && ra != 0) {
> +                tcg_gen_ext32u_tl(cpu_R[ra], cpu_R[ra]);
> +                tcg_gen_ext32u_tl(cpu_R[rb], cpu_R[rb]);

This code would clobber high 32 bits of ra and rb if it was compiled
for 64 bit registers.
Are you going to support both 32 and 64 bit version of the ISA?
Could you please clarify *_tl usage here?

> +                tcg_gen_mul_tl(cpu_R[rd], cpu_R[ra], cpu_R[rb]);
> +            } else {
> +                tcg_gen_movi_tl(cpu_R[rd], 0);
> +            }
> +            break;
> +        default:
> +            break;
> +        }
> +        break;
> +
> +    case 0x000e:
> +        switch (op1) {
> +        case 0x00:   /*l.cmov*/
> +            LOG_DIS("l.cmov r%d, r%d, r%d\n", rd, ra, rb);
> +            {
> +                int lab = gen_new_label();
> +                TCGv sr_f = tcg_temp_new();
> +                tcg_gen_andi_tl(sr_f, cpu_sr, SR_F);
> +                tcg_gen_mov_tl(cpu_R[rd], cpu_R[rb]);
> +                tcg_gen_brcondi_tl(TCG_COND_NE, sr_f, SR_F, lab);

There may be an issue when rd == ra and the branch is not taken

> +                tcg_gen_mov_tl(cpu_R[rd], cpu_R[ra]);
> +                gen_set_label(lab);
> +                tcg_temp_free(sr_f);
> +            }
> +            break;
> +        default:
> +            break;
> +        }
> +        break;

[...]

> +    case 0x13:    /*l.maci*/
> +        {
> +            LOG_DIS("l.maci %d, r%d, %d\n", I5, ra, I11);
> +            TCGv_i64 tra = tcg_temp_new_i64();    /*  store cpu_R[ra]*/
> +            TCGv_i64 tmac = tcg_temp_new_i64();   /*  store machi maclo*/
> +            tcg_gen_movi_tl(t0, tmp);
> +            tcg_gen_ext32s_i64(tra, cpu_R[ra]);
> +            tcg_gen_ext32s_i64(tmac, t0);
> +            tcg_gen_mul_tl(tmac, tra, tmac);
> +            tcg_gen_trunc_i64_i32(cpu_R[ra], tmac);
> +            tcg_gen_add_i64(machi, machi, cpu_R[ra]);
> +            tcg_gen_add_i64(maclo, maclo, cpu_R[ra]);
> +            tcg_temp_free(tra);
> +            tcg_temp_free(tmac);
> +        }
> +        break;

[...]

> +static void dec_mac(DisasContext *dc, CPUOPENRISCState *env, uint32_t insn)
> +{
> +    uint32_t op0;
> +    uint32_t ra, rb;
> +    op0 = field(insn, 0, 4);
> +    ra = field(insn, 16, 5);
> +    rb = field(insn, 11, 5);
> +    TCGv_i64 t0 = tcg_temp_new();
> +    TCGv_i64 t1 = tcg_temp_new();
> +    switch (op0) {
> +    case 0x0001:   /*l.mac*/
> +        {
> +            LOG_DIS("l.mac r%d, r%d\n", ra, rb);
> +            tcg_gen_ext_i32_i64(t0, cpu_R[ra]);
> +            tcg_gen_ext_i32_i64(t1, cpu_R[rb]);
> +            tcg_gen_mul_i64(t0, t0, t1);
> +            tcg_gen_trunc_i64_i32(cpu_R[ra], t0);
> +            tcg_gen_add_tl(maclo, maclo, cpu_R[ra]);
> +            tcg_gen_add_tl(machi, machi, cpu_R[ra]);

>From the ISA I've got an impression that mac/maci should be
implemented like this (signedness left alone):

            tcg_gen_mul_tl(t0, cpu_R[ra], cpu_R[rb]);
            tcg_gen_ext_i32_i64(t1, t0);
            tcg_gen_concat_i32_i64(t2, maclo, machi);
            tcg_gen_add_i64(t2, t2, t1);
            tcg_gen_trunc_i64(maclo, t2);
            tcg_gen_shri_i64(t2, t2, 32);
            tcg_gen_trunc_i64(machi, t2);

[...]

> +static void dec_float(DisasContext *dc, CPUOPENRISCState *env, uint32_t insn)
> +{
> +    uint32_t op0;
> +    uint32_t ra, rb, rd;
> +    op0 = field(insn, 0, 8);
> +    ra = field(insn, 16, 5);
> +    rb = field(insn, 11, 5);
> +    rd = field(insn, 21, 5);
> +
> +    switch (op0) {
> +    case 0x10:    /*lf.add.d*/
> +        LOG_DIS("lf.add.d r%d, r%d, r%d\n", rd, ra, rb);
> +        tcg_gen_add_i64(cpu_R[rd], cpu_R[ra], cpu_R[rb]);

Through this function you generate integer operations on the
registers, although ISA
suggests that there should be either single- or double-precision
floating point operations.

-- 
Thanks.
-- Max