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

[Jia Liu]

Hi Max

Thanks for comments.

On Thu, May 17, 2012 at 8:11 PM, Max Filippov <address@hidden> wrote:
> 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 */

overflow/division by zero is handled here by patch 05/15.

>> +            }
>> +            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?
>

I used uint32_t at first, change it into *_tl for 64bit support in the future.
May I keep it *_tl? If I can not, I'll back it to uint32.

>> +                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

Thanks, I'll check this.

>
>> +                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);
>
> [...]
>

Thanks very much, I'll replace it in V2.

>> +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.
>

Sorry, I  didn't find a TCG-IR that make single- or double-precision
floating point operations, may you give me some hits?

> --
> Thanks.
> -- Max


Regards,
Jia.