Re: [Qemu-devel] [PATCH v3 08/16] target-or32: Add translation routines

[Jia Liu]

Hi Max,

On Fri, Jun 8, 2012 at 8:56 PM, Max Filippov <address@hidden> wrote:
> Hi Jia.
>
> On Fri, Jun 8, 2012 at 4:00 AM, Jia Liu <address@hidden> wrote:
>
> [...]
>
>>>> +    case 0x0009:
>>>> +        switch (op1) {
>>>> +        case 0x03:   /*l.div*/
>>>> +            LOG_DIS("l.div r%d, r%d, r%d\n", rd, ra, rb);
>>>> +            {
>>>> +                TCGv_i32 sr_ove;
>>>> +                int lab = gen_new_label();
>>>> +                sr_ove = tcg_temp_new();
>>>> +                tcg_gen_ori_tl(cpu_sr, cpu_sr, SR_OV);
>>>> +                tcg_gen_ori_tl(cpu_sr, cpu_sr, SR_CY);
>>>> +                tcg_gen_andi_tl(sr_ove, cpu_sr, SR_OVE);
>>>> +                if (rb == 0) {
>>>> +                    tcg_gen_brcondi_tl(TCG_COND_NE, sr_ove, SR_OVE, lab);
>>>> +                    gen_exception(dc, EXCP_RANGE);
>>>> +                    gen_set_label(lab);
>>>> +                } else {
>>>> +                    if (ra == 0xffffffff && rb == 0x80000000) {
>>>
>>> Cannot do that: ra and rb are register numbers, not the values
>>> contained in these registers.
>>> Hence you need to generate code that will check these combinations of
>>> register values.
>>>
>>
>>        case 0x03:   /*l.div*/
>>            LOG_DIS("l.div r%d, r%d, r%d\n", rd, ra, rb);
>>            {
>>                int lab0 = gen_new_label();
>>                int lab1 = gen_new_label();
>>                int lab2 = gen_new_label();
>>                TCGv_i32 sr_ove = tcg_temp_new_i32();
>>                tcg_gen_andi_tl(sr_ove, cpu_sr, SR_OVE);
>>                if (rb == 0) {
>>                    tcg_gen_ori_tl(cpu_sr, cpu_sr, SR_OV);
>>                    tcg_gen_ori_tl(cpu_sr, cpu_sr, SR_CY);
>>                    tcg_gen_brcondi_tl(TCG_COND_NE, sr_ove, SR_OVE, lab0);
>>                    gen_exception(dc, EXCP_RANGE);
>>                    gen_set_label(lab0);
>>                } else {
>>                    tcg_gen_brcondi_tl(TCG_COND_EQ, cpu_R[rb],
>>                                       0x00000000, lab1);
>>                    tcg_gen_brcondi_tl(TCG_COND_NE, cpu_R[ra],
>>                                       0xffffffff, lab2);
>>                    tcg_gen_brcondi_tl(TCG_COND_NE, cpu_R[rb],
>>                                       0x80000000, lab2);
>>                    gen_set_label(lab1);
>>                    tcg_gen_ori_tl(cpu_sr, cpu_sr, SR_OV);
>>                    tcg_gen_ori_tl(cpu_sr, cpu_sr, SR_CY);
>>                    tcg_gen_brcondi_tl(TCG_COND_NE, sr_ove, SR_OVE, lab2);
>>                    gen_exception(dc, EXCP_RANGE);
>>                    gen_set_label(lab2);
>>                    tcg_gen_div_tl(cpu_R[rd], cpu_R[ra], cpu_R[rb]);
>
> You still divide by zero/overflow here in case SR_OVE is not set.
> What value goes to the rd register in case of division by 0? Maybe
> you should skip the division altogether?
>
>>                }
>>                tcg_temp_free_i32(sr_ove);
>>            }
>>            break;
>>
>>
>> is this right?
>
> Much better now (:
>
>>>> +                        tcg_gen_brcondi_tl(TCG_COND_NE, sr_ove, SR_OVE, 
>>>> lab);
>>>> +                        gen_exception(dc, EXCP_RANGE);
>>>> +                        gen_set_label(lab);
>>>> +                    } else {
>>>> +                        tcg_gen_div_tl(cpu_R[rd], cpu_R[ra], cpu_R[rb]);
>>>> +                    }
>>>> +                }
>>>> +                tcg_temp_free(sr_ove);
>>>> +            }
>>>> +            break;
>>>> +
>>>> +        default:
>>>> +            gen_illegal_exception(dc);
>>>> +            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) {
>>>> +                TCGv_i32 sr_ove;
>>>> +                int lab = gen_new_label();
>>>> +                sr_ove = tcg_temp_new();
>>>> +                tcg_gen_ori_tl(cpu_sr, cpu_sr, SR_OV);
>>>> +                tcg_gen_ori_tl(cpu_sr, cpu_sr, SR_CY);
>>>> +                tcg_gen_andi_tl(sr_ove, cpu_sr, SR_OVE);
>>>> +                tcg_gen_brcondi_tl(TCG_COND_NE, sr_ove, SR_OVE, lab);
>>>> +                gen_exception(dc, EXCP_RANGE);
>>>> +                gen_set_label(lab);
>>>> +                tcg_temp_free(sr_ove);
>>>> +            } else if (rb != 0) {
>>>
>>> 'if (rb != 0)' and the following 'else' block are redundant here.
>>>
>>> I feel that I repeatedly fail to explain what's wrong with these div/divu
>>> implementations; could you please add testcases for l.div and l.divu
>>> that divide by the register other than r0 that contains 0 value?
>>>
>>
>> and
>>
>>        case 0x03:   /*l.divu*/
>>            LOG_DIS("l.divu r%d, r%d, r%d\n", rd, ra, rb);
>>            {
>>                int lab0 = gen_new_label();
>>                int lab1 = gen_new_label();
>>                TCGv_i32 sr_ove = tcg_temp_new();
>>                tcg_gen_andi_tl(sr_ove, cpu_sr, SR_OVE);
>>                if (rb == 0) {
>>                    tcg_gen_ori_tl(cpu_sr, cpu_sr, SR_OV);
>>                    tcg_gen_ori_tl(cpu_sr, cpu_sr, SR_CY);
>>                    tcg_gen_brcondi_tl(TCG_COND_NE, sr_ove, SR_OVE, lab0);
>>                    gen_exception(dc, EXCP_RANGE);
>>                    gen_set_label(lab0);
>>                } else {
>>                    tcg_gen_brcondi_tl(TCG_COND_NE, cpu_R[rb],
>>                                       0x00000000, lab1);
>>                    tcg_gen_ori_tl(cpu_sr, cpu_sr, SR_OV);
>>                    tcg_gen_ori_tl(cpu_sr, cpu_sr, SR_CY);
>>                    tcg_gen_brcondi_tl(TCG_COND_NE, sr_ove, SR_OVE, lab1);
>>                    gen_exception(dc, EXCP_RANGE);
>>                    gen_set_label(lab1);
>>                    tcg_gen_divu_tl(cpu_R[rd], cpu_R[ra], cpu_R[rb]);
>
> Ditto.
>
>>                }
>>                tcg_temp_free_i32(sr_ove);
>>            }
>>            break;
>>
>> is this OK?
>>
>>>> +                tcg_gen_divu_tl(cpu_R[rd], cpu_R[ra], cpu_R[rb]);
>>>> +            } else {
>>>> +                break;
>>>> +            }
>>>> +            break;
>>>> +
>>>> +        default:
>>>> +            gen_illegal_exception(dc);
>>>> +            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) {
>>>> +                TCGv result = tcg_temp_new();
>>>> +                TCGv high = tcg_temp_new();
>>>> +                TCGv tra = tcg_temp_new();
>>>> +                TCGv trb = tcg_temp_new();
>>>> +                TCGv_i32 sr_ove = tcg_temp_new();
>>>> +                int lab = gen_new_label();
>>>> +                int lab2 = gen_new_label();
>>>> +                tcg_gen_extu_i32_i64(tra, cpu_R[ra]);
>>>> +                tcg_gen_extu_i32_i64(trb, cpu_R[rb]);
>>>> +                tcg_gen_mul_tl(result, cpu_R[ra], cpu_R[rb]);
>>>
>>> You've calculated tra and trb but haven't uses them here.
>>>
>>>> +                tcg_gen_shri_tl(high, result, (sizeof(target_ulong) * 8));
>>>
>>> You probably meant result and high to be _i64.
>>>
>>>> +                tcg_gen_brcondi_tl(TCG_COND_EQ, high, 0x0, lab);
>>>> +                tcg_gen_ori_tl(cpu_sr, cpu_sr, SR_CY);
>>>> +                tcg_gen_ori_tl(cpu_sr, cpu_sr, SR_OV);
>>>> +                tcg_gen_andi_tl(sr_ove, cpu_sr, SR_OVE);
>>>> +                tcg_gen_brcondi_tl(TCG_COND_NE, sr_ove, SR_OVE, lab2);
>>>> +                gen_exception(dc, EXCP_RANGE);
>>>> +                gen_set_label(lab);
>>>> +                gen_set_label(lab2);
>>>
>>> No need to set two labels at one point.
>>>
>>
>>        case 0x03:   /*l.mulu*/
>>            LOG_DIS("l.mulu r%d, r%d, r%d\n", rd, ra, rb);
>>            if (rb != 0 && ra != 0) {
>>                TCGv_i64 result = tcg_temp_new_i64();
>>                TCGv_i64 tra = tcg_temp_new_i64();
>>                TCGv_i64 trb = tcg_temp_new_i64();
>>                TCGv high = tcg_temp_new();
>>                TCGv_i32 sr_ove = tcg_temp_new();
>>                int lab = gen_new_label();
>>                tcg_gen_extu_i32_i64(tra, cpu_R[ra]);
>>                tcg_gen_extu_i32_i64(trb, cpu_R[rb]);
>>                tcg_gen_mul_i64(result, tra, trb);
>>                tcg_temp_free(tra);
>>                tcg_temp_free(trb);
>>                tcg_gen_shri_i64(high, result, (sizeof(target_ulong) * 8));
>
> TARGET_LONG_BITS?
>

yes, thank you, I'll fix them, all.

>>                tcg_gen_brcondi_tl(TCG_COND_EQ, high, 0x00000000, lab);
>>                tcg_gen_ori_tl(cpu_sr, cpu_sr, SR_CY);
>>                tcg_gen_ori_tl(cpu_sr, cpu_sr, SR_OV);
>
> Not an issue, but you could just tcg_gen_ori_tl(cpu_sr, cpu_sr, SR_CY | 
> SR_OV);
>

all right, thank you, I'll fix them all.

>>                tcg_gen_andi_tl(sr_ove, cpu_sr, SR_OVE);
>>                tcg_gen_brcondi_tl(TCG_COND_NE, sr_ove, SR_OVE, lab);
>>                gen_exception(dc, EXCP_RANGE);
>>                gen_set_label(lab);
>>                tcg_temp_free(high);
>>                tcg_gen_trunc_i64_tl(cpu_R[rd], result);
>>                tcg_temp_free(result);
>>                tcg_temp_free(sr_ove);
>>            } else {
>>                tcg_gen_movi_tl(cpu_R[rd], 0);
>>            }
>>            break;
>>
>> is it right this time?
>
> Looks good to me.
>
>>>> +                tcg_gen_trunc_i64_tl(cpu_R[rd], result);
>>>> +                tcg_temp_free(result);
>>>> +                tcg_temp_free(high);
>>>> +                tcg_temp_free(sr_ove);
>>>> +                tcg_temp_free(tra);
>>>> +                tcg_temp_free(trb);
>>>> +            } else {
>>>> +                tcg_gen_movi_tl(cpu_R[rd], 0);
>>>> +            }
>>>> +            break;
>>>> +
>>>> +        default:
>>>> +            gen_illegal_exception(dc);
>>>> +            break;
>>>> +        }
>>>> +        break;
>>>> +
>>>
>>> [...]
>>>
>>>> +    case 0x13:    /*l.maci*/
>>>> +        LOG_DIS("l.maci %d, r%d, %d\n", I5, ra, I11);
>>>> +        {
>>>> +            TCGv t1 = tcg_temp_new();
>>>> +            TCGv t2 = tcg_temp_new();
>>>> +            TCGv ttmp = tcg_temp_new();   /*  store machi maclo*/
>>>> +            ttmp = tcg_const_tl(tmp);
>>>
>>> Leaked previous ttmp temporary.
>>>
>>>> +            tcg_gen_mul_tl(t0, cpu_R[ra], ttmp);
>>>
>>> What t0?
>>>
>>>> +            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_i32(maclo, t2);
>>>> +            tcg_gen_shri_i64(t2, t2, 32);
>>>> +            tcg_gen_trunc_i64_i32(machi, t2);
>>>> +            tcg_temp_free(t0);
>>>> +            tcg_temp_free(t1);
>>>> +            tcg_temp_free(t2);
>>>
>>> Leaked ttmp temporary.
>>
>>
>>    case 0x13:    /*l.maci*/
>>        LOG_DIS("l.maci %d, r%d, %d\n", I5, ra, I11);
>>        {
>>            TCGv t1 = tcg_temp_new();
>>            TCGv t2 = tcg_temp_new();
>>            TCGv ttmp = tcg_const_tl(tmp);  /*  store machi maclo*/
>>            tcg_gen_mul_tl(ttmp, cpu_R[ra], ttmp);
>>            tcg_gen_ext_i32_i64(t1, ttmp);
>>            tcg_gen_concat_i32_i64(t2, maclo, machi);
>>            tcg_gen_add_i64(t2, t2, t1);
>>            tcg_gen_trunc_i64_i32(maclo, t2);
>>            tcg_gen_shri_i64(t2, t2, 32);
>>            tcg_gen_trunc_i64_i32(machi, t2);
>>            tcg_temp_free(ttmp);
>>            tcg_temp_free(t1);
>>            tcg_temp_free(t2);
>>        }
>>        break;
>>
>>
>>>
>>>> +        }
>>>> +        break;
>>>
>>> [...]
>>>
>>>> +    case 0x20:   /*l.ld*/
>>>> +        LOG_DIS("l.ld r%d, r%d, %d\n", rd, ra, I16);
>>>> +        tcg_gen_addi_tl(t0, cpu_R[ra], sign_extend(I16, 16));
>>>> +        tcg_gen_qemu_ld64(cpu_R[rd], t0, dc->mem_idx);
>>>
>>> Cannot ld64 into _tl register.
>>>
>>
>>    case 0x20:   /*l.ld*/
>>        LOG_DIS("l.ld r%d, r%d, %d\n", rd, ra, I16);
>>        {
>>            check_ob64s(dc);
>
> This throws an exception in the 32-bit registers case, right?
>

this is legal check, like MIPS's check_insn();
You will see all of them in v4.

>>            TCGv_i64 t0 = tcg_temp_new_i64();
>>            tcg_gen_addi_i64(t0, cpu_R[ra], sign_extend(I16, 16));
>>            tcg_gen_qemu_ld64(cpu_R[rd], t0, dc->mem_idx);
>>            tcg_temp_free_i64(t0);
>>        }
>>        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();
>>>> +    TCGv_i64 t2 = tcg_temp_new();
>>>> +    switch (op0) {
>>>> +    case 0x0001:   /*l.mac*/
>>>> +        LOG_DIS("l.mac r%d, r%d\n", ra, rb);
>>>> +        {
>>>> +            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_i32(maclo, t2);
>>>> +            tcg_gen_shri_i64(t2, t2, 32);
>>>> +            tcg_gen_trunc_i64_i32(machi, t2);
>>>> +            tcg_temp_free(t0);
>>>> +            tcg_temp_free(t1);
>>>> +            tcg_temp_free(t2);
>>>
>>> Instead of freeing temporaries repeatedly in some cases (and
>>> leaking them in others) you could free them once after the switch.
>>>
>>
>>    case 0x0001:   /*l.mac*/
>>        LOG_DIS("l.mac r%d, r%d\n", ra, rb);
>>        {
>>            TCGv_i64 t0 = tcg_temp_new();
>>            TCGv_i64 t1 = tcg_temp_new();
>>            TCGv_i64 t2 = tcg_temp_new();
>>            tcg_gen_mul_tl(t0, cpu_R[ra], cpu_R[rb]);
>
> t0 should be _tl according to that.

Aha, thank you, I'll fix it.

>
>>            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_i32(maclo, t2);
>>            tcg_gen_shri_i64(t2, t2, 32);
>>            tcg_gen_trunc_i64_i32(machi, t2);
>>            tcg_temp_free(t0);
>>            tcg_temp_free(t1);
>>            tcg_temp_free(t2);
>>        }
>>        break;
>>
>> I think define use and free them separately make code more clear :-)
>
> Ok, at your preference.
>
> Looks like I completely missed temporaries vs local temporaries issue
> spotted by Richard, so expect more review rounds.
>

Richard is great!

> Jia, is there kernel/rootfs image available for openrisc?
>

kernel/rootfs image and hello-world in my google drive, I'll send the
URL with v4, and v4 is soon.

> --
> Thanks.
> -- Max

Regards,
Jia