Hi Max
Thanks for comments.
On Thu, May 17, 2012 at 8:11 PM, Max Filippov <jcmvb...@gmail.com> wrote:
> Hi.
>
> I've got a couple of questions/suggestions regarding the code.
>
> On Thu, May 17, 2012 at 12:35 PM, Jia Liu <pro...@gmail.com> wrote:
>> add the openrisc instructions translation.
>>
>> 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);
>> + 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.
