Hi Jia,
more comments on remaining issues visible with unaided eye.
On Wed, Jun 6, 2012 at 4:27 PM, Jia Liu <pro...@gmail.com> wrote:
> Add OpenRISC translation routines.
>
> Signed-off-by: Jia Liu <pro...@gmail.com>
> ---
[...]
> + 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.
> + 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?
> + 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.
> + 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.
> + }
> + 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 0x34: /*l.sd*/
> + LOG_DIS("l.sd %d, r%d, r%d, %d\n", I5, ra, rb, I11);
> + tcg_gen_addi_tl(t0, cpu_R[ra], sign_extend(tmp, 16));
> + tcg_gen_qemu_st64(cpu_R[rb], t0, dc->mem_idx);
Ditto.
[...]
> +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.
> + }
> + break;
> +
> + case 0x0002: /*l.msb*/
> + LOG_DIS("l.msb 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_sub_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;
> +
> + default:
> + gen_illegal_exception(dc);
> + break;
> + }
> +}
--
Thanks.
-- Max
