| Message ID | 1478121319-31986-3-git-send-email-laurent@vivier.eu |
|---|---|
| State | New |
| Headers | show |
On 11/02/2016 03:15 PM, Laurent Vivier wrote: > 680x0 movem can load/store words and long words > and can use more addressing modes. > Coldfire can only use long words with (Ax) and (d16,Ax) > addressing modes. > > Signed-off-by: Laurent Vivier <laurent@vivier.eu> > --- > target-m68k/translate.c | 96 ++++++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 79 insertions(+), 17 deletions(-) Reviewed-by: Richard Henderson <rth@twiddle.net> r~
On 11/02/2016 03:15 PM, Laurent Vivier wrote: > + if ((insn & 7) + 8 == i && > + m68k_feature(s->env, M68K_FEATURE_EXT_FULL)) { > + /* M68020+: if the addressing register is the > + * register moved to memory, the value written > + * is the initial value decremented by the size of > + * the operation > + * M68000/M68010: the value is the initial value > + */ > + TCGv tmp = tcg_temp_new(); > + tcg_gen_sub_i32(tmp, mreg(i), incr); > + gen_store(s, opsize, addr, tmp); > + tcg_temp_free(tmp); This doesn't look right. Is the value stored the intermediate value of the decremented register, or the final value? What you're storing is reg-4, which is neither of these things. I could see, maybe, that reg-4 might well turn out to be the right value for movem {a0-a7}, (sp)- since sp == a7, and therefore stored first. But I question that's the correct result for movem {a0-a7}, (a1)- If it's the incremental value, then you can just store "addr" and you don't need a temp. If it's the final value, then you can compute tcg_gen_subi_i32(tmp, AREG(insn, 0), ctpop32(mask) * 4); r~
Le 03/11/2016 à 20:47, Richard Henderson a écrit : > On 11/02/2016 03:15 PM, Laurent Vivier wrote: >> + if ((insn & 7) + 8 == i && >> + m68k_feature(s->env, M68K_FEATURE_EXT_FULL)) { >> + /* M68020+: if the addressing register is the >> + * register moved to memory, the value written >> + * is the initial value decremented by the >> size of >> + * the operation >> + * M68000/M68010: the value is the initial value >> + */ >> + TCGv tmp = tcg_temp_new(); >> + tcg_gen_sub_i32(tmp, mreg(i), incr); >> + gen_store(s, opsize, addr, tmp); >> + tcg_temp_free(tmp); > > This doesn't look right. Is the value stored the intermediate value of > the decremented register, or the final value? What you're storing is > reg-4, which is neither of these things. > > I could see, maybe, that reg-4 might well turn out to be the right value > for > > movem {a0-a7}, (sp)- > > since sp == a7, and therefore stored first. But I question that's the > correct result for > > movem {a0-a7}, (a1)- > > If it's the incremental value, then you can just store "addr" and you > don't need a temp. If it's the final value, then you can compute > > tcg_gen_subi_i32(tmp, AREG(insn, 0), ctpop32(mask) * 4); > As it was not clear for me, I have written a test to see what was the good value. my test program is: top: .space 64,0 stack: .text .globl _start _start: lea stack,%a4 lea 1,%a0 lea 2,%a1 lea 3,%a2 lea 4,%a3 lea 5,%a5 lea 6,%a6 moveq.l #8, %d0 moveq.l #9, %d1 moveq.l #10, %d2 moveq.l #11, %d3 moveq.l #12, %d4 moveq.l #13, %d5 moveq.l #14, %d6 moveq.l #15, %d7 movem.l %a0-%a7/%d0-%d7,-(%a4) on a real 68040: initial value of A4 is 0x800020ec final value of A4 is 0x800020ac (gdb) x/15x 0x800020ac 0x800020ac: 0x00000008 0x00000009 0x0000000a 0x0000000b 0x800020bc: 0x0000000c 0x0000000d 0x0000000e 0x0000000f 0x800020cc: 0x00000001 0x00000002 0x00000003 0x00000004 0x800020dc: 0x800020e8 0x00000005 0x00000006 Stored value is thus 0x800020e8 so this is initial value - 4. [I have tried the same test with a1, for the same result] Laurent
On 11/03/2016 02:11 PM, Laurent Vivier wrote: > Le 03/11/2016 à 20:47, Richard Henderson a écrit : >> On 11/02/2016 03:15 PM, Laurent Vivier wrote: >>> + if ((insn & 7) + 8 == i && >>> + m68k_feature(s->env, M68K_FEATURE_EXT_FULL)) { >>> + /* M68020+: if the addressing register is the >>> + * register moved to memory, the value written >>> + * is the initial value decremented by the >>> size of >>> + * the operation >>> + * M68000/M68010: the value is the initial value >>> + */ >>> + TCGv tmp = tcg_temp_new(); >>> + tcg_gen_sub_i32(tmp, mreg(i), incr); >>> + gen_store(s, opsize, addr, tmp); >>> + tcg_temp_free(tmp); >> >> This doesn't look right. Is the value stored the intermediate value of >> the decremented register, or the final value? What you're storing is >> reg-4, which is neither of these things. >> >> I could see, maybe, that reg-4 might well turn out to be the right value >> for >> >> movem {a0-a7}, (sp)- >> >> since sp == a7, and therefore stored first. But I question that's the >> correct result for >> >> movem {a0-a7}, (a1)- >> >> If it's the incremental value, then you can just store "addr" and you >> don't need a temp. If it's the final value, then you can compute >> >> tcg_gen_subi_i32(tmp, AREG(insn, 0), ctpop32(mask) * 4); >> > > As it was not clear for me, I have written a test to see what was the > good value. > > my test program is: > > top: > .space 64,0 > stack: > .text > .globl _start > _start: > lea stack,%a4 > lea 1,%a0 > lea 2,%a1 > lea 3,%a2 > lea 4,%a3 > lea 5,%a5 > lea 6,%a6 > moveq.l #8, %d0 > moveq.l #9, %d1 > moveq.l #10, %d2 > moveq.l #11, %d3 > moveq.l #12, %d4 > moveq.l #13, %d5 > moveq.l #14, %d6 > moveq.l #15, %d7 > movem.l %a0-%a7/%d0-%d7,-(%a4) > > on a real 68040: > > initial value of A4 is 0x800020ec > final value of A4 is 0x800020ac > > (gdb) x/15x 0x800020ac > 0x800020ac: 0x00000008 0x00000009 0x0000000a 0x0000000b > 0x800020bc: 0x0000000c 0x0000000d 0x0000000e 0x0000000f > 0x800020cc: 0x00000001 0x00000002 0x00000003 0x00000004 > 0x800020dc: 0x800020e8 0x00000005 0x00000006 > > Stored value is thus 0x800020e8 so this is initial value - 4. > [I have tried the same test with a1, for the same result] Weird. But, ok. r~
On 11/02/2016 03:15 PM, Laurent Vivier wrote: > + for (i = 15; i >= 0; i--, mask >>= 1) { > + if (mask & 1) { > + if ((insn & 7) + 8 == i && > + m68k_feature(s->env, M68K_FEATURE_EXT_FULL)) { > + /* M68020+: if the addressing register is the > + * register moved to memory, the value written > + * is the initial value decremented by the size of > + * the operation > + * M68000/M68010: the value is the initial value > + */ > + TCGv tmp = tcg_temp_new(); > + tcg_gen_sub_i32(tmp, mreg(i), incr); > + gen_store(s, opsize, addr, tmp); > + tcg_temp_free(tmp); > + } else { > + gen_store(s, opsize, addr, mreg(i)); > + } > + if (mask != 1) { > + tcg_gen_sub_i32(addr, addr, incr); > + } > + } One more thing: This is pre-decrement. Why are you decrementing after the store? Seems to me this should be if (mask & 1) { tcg_gen_sub_i32(addr, addr, incr); if (REG(insn, 0) + 8 == i ...) ... } r~
Le 03/11/2016 à 21:47, Richard Henderson a écrit : > On 11/02/2016 03:15 PM, Laurent Vivier wrote: >> + for (i = 15; i >= 0; i--, mask >>= 1) { >> + if (mask & 1) { >> + if ((insn & 7) + 8 == i && >> + m68k_feature(s->env, M68K_FEATURE_EXT_FULL)) { >> + /* M68020+: if the addressing register is the >> + * register moved to memory, the value written >> + * is the initial value decremented by the >> size of >> + * the operation >> + * M68000/M68010: the value is the initial value >> + */ >> + TCGv tmp = tcg_temp_new(); >> + tcg_gen_sub_i32(tmp, mreg(i), incr); >> + gen_store(s, opsize, addr, tmp); >> + tcg_temp_free(tmp); >> + } else { >> + gen_store(s, opsize, addr, mreg(i)); >> + } >> + if (mask != 1) { >> + tcg_gen_sub_i32(addr, addr, incr); >> + } >> + } > > One more thing: This is pre-decrement. Why are you decrementing after > the store? Seems to me this should be > > if (mask & 1) { > tcg_gen_sub_i32(addr, addr, incr); > if (REG(insn, 0) + 8 == i ...) > ... > } > Because it has already been decremented by gen_lea()... so this a problem if we have page fault, except if we use your "areg writeback" series, and we will. Thanks, Laurent
On 11/04/2016 01:59 AM, Laurent Vivier wrote: > Le 03/11/2016 à 21:47, Richard Henderson a écrit : >> On 11/02/2016 03:15 PM, Laurent Vivier wrote: >>> + for (i = 15; i >= 0; i--, mask >>= 1) { >>> + if (mask & 1) { >>> + if ((insn & 7) + 8 == i && >>> + m68k_feature(s->env, M68K_FEATURE_EXT_FULL)) { >>> + /* M68020+: if the addressing register is the >>> + * register moved to memory, the value written >>> + * is the initial value decremented by the >>> size of >>> + * the operation >>> + * M68000/M68010: the value is the initial value >>> + */ >>> + TCGv tmp = tcg_temp_new(); >>> + tcg_gen_sub_i32(tmp, mreg(i), incr); >>> + gen_store(s, opsize, addr, tmp); >>> + tcg_temp_free(tmp); >>> + } else { >>> + gen_store(s, opsize, addr, mreg(i)); >>> + } >>> + if (mask != 1) { >>> + tcg_gen_sub_i32(addr, addr, incr); >>> + } >>> + } >> >> One more thing: This is pre-decrement. Why are you decrementing after >> the store? Seems to me this should be >> >> if (mask & 1) { >> tcg_gen_sub_i32(addr, addr, incr); >> if (REG(insn, 0) + 8 == i ...) >> ... >> } >> > > Because it has already been decremented by gen_lea()... so this a > problem if we have page fault, except if we use your "areg writeback" > series, and we will. Ah, I see. No, gen_lea doesn't writeback; only gen_ea does. But I wonder if this couldn't be cleaned up a tad. I'll get back to you. r~
diff --git a/target-m68k/translate.c b/target-m68k/translate.c index 1cf88a4..93f1270 100644 --- a/target-m68k/translate.c +++ b/target-m68k/translate.c @@ -1667,14 +1667,25 @@ static void gen_push(DisasContext *s, TCGv val) tcg_gen_mov_i32(QREG_SP, tmp); } +static TCGv mreg(int reg) +{ + if (reg < 8) { + /* Dx */ + return cpu_dregs[reg]; + } + /* Ax */ + return cpu_aregs[reg & 7]; +} + DISAS_INSN(movem) { TCGv addr; int i; uint16_t mask; - TCGv reg; TCGv tmp; - int is_load; + int is_load = (insn & 0x0400) != 0; + int opsize = (insn & 0x40) != 0 ? OS_LONG : OS_WORD; + TCGv incr; mask = read_im16(env, s); tmp = gen_lea(env, s, insn, OS_LONG); @@ -1682,25 +1693,74 @@ DISAS_INSN(movem) gen_addr_fault(s); return; } + addr = tcg_temp_new(); tcg_gen_mov_i32(addr, tmp); - is_load = ((insn & 0x0400) != 0); - for (i = 0; i < 16; i++, mask >>= 1) { - if (mask & 1) { - if (i < 8) - reg = DREG(i, 0); - else - reg = AREG(i, 0); - if (is_load) { - tmp = gen_load(s, OS_LONG, addr, 0); - tcg_gen_mov_i32(reg, tmp); - } else { - gen_store(s, OS_LONG, addr, reg); + incr = tcg_const_i32(opsize_bytes(opsize)); + + if (is_load) { + /* memory to register */ + TCGv r[16]; + for (i = 0; i < 16; i++) { + if ((mask >> i) & 1) { + r[i] = gen_load(s, opsize, addr, 1); + tcg_gen_add_i32(addr, addr, incr); + } + } + for (i = 0; i < 16; i++, mask >>= 1) { + if (mask & 1) { + tcg_gen_mov_i32(mreg(i), r[i]); + tcg_temp_free(r[i]); + } + } + if ((insn & 070) == 030) { + /* movem (An)+,X */ + tcg_gen_mov_i32(AREG(insn, 0), addr); + } + + } else { + /* register to memory */ + + if ((insn & 070) == 040) { + /* movem X,-(An) */ + + for (i = 15; i >= 0; i--, mask >>= 1) { + if (mask & 1) { + if ((insn & 7) + 8 == i && + m68k_feature(s->env, M68K_FEATURE_EXT_FULL)) { + /* M68020+: if the addressing register is the + * register moved to memory, the value written + * is the initial value decremented by the size of + * the operation + * M68000/M68010: the value is the initial value + */ + TCGv tmp = tcg_temp_new(); + tcg_gen_sub_i32(tmp, mreg(i), incr); + gen_store(s, opsize, addr, tmp); + tcg_temp_free(tmp); + } else { + gen_store(s, opsize, addr, mreg(i)); + } + if (mask != 1) { + tcg_gen_sub_i32(addr, addr, incr); + } + } + } + tcg_gen_mov_i32(AREG(insn, 0), addr); + } else { + /* movem X,(An)+ is not allowed */ + + for (i = 0; i < 16; i++, mask >>= 1) { + if (mask & 1) { + gen_store(s, opsize, addr, mreg(i)); + tcg_gen_add_i32(addr, addr, incr); + } } - if (mask != 1) - tcg_gen_addi_i32(addr, addr, 4); } } + + tcg_temp_free(incr); + tcg_temp_free(addr); } DISAS_INSN(bitop_im) @@ -3858,7 +3918,9 @@ void register_m68k_insns (CPUM68KState *env) BASE(pea, 4840, ffc0); BASE(swap, 4840, fff8); INSN(bkpt, 4848, fff8, BKPT); - BASE(movem, 48c0, fbc0); + INSN(movem, 48d0, fbf8, CF_ISA_A); + INSN(movem, 48e8, fbf8, CF_ISA_A); + INSN(movem, 4880, fb80, M68000); BASE(ext, 4880, fff8); BASE(ext, 48c0, fff8); BASE(ext, 49c0, fff8);
680x0 movem can load/store words and long words and can use more addressing modes. Coldfire can only use long words with (Ax) and (d16,Ax) addressing modes. Signed-off-by: Laurent Vivier <laurent@vivier.eu> --- target-m68k/translate.c | 96 ++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 79 insertions(+), 17 deletions(-)