Message ID | 1477604609-2206-2-git-send-email-laurent@vivier.eu |
---|---|
State | New |
Headers | show |
On 10/27/2016 03:43 PM, Laurent Vivier wrote: > +DISAS_INSN(cmpm) > +{ > + int opsize = insn_opsize(insn); > + TCGv tmp = tcg_temp_new(); > + TCGv src, dst, addr; > + > + src = gen_load(s, opsize, AREG(insn, 0), 1); > + /* delay the update after the second gen_load() */ > + tcg_gen_addi_i32(tmp, AREG(insn, 0), opsize_bytes(opsize)); > + > + /* but if the we use the same address register to > + * read the second value, we must use the updated address > + */ > + if (REG(insn, 0) == REG(insn, 9)) { > + addr = tmp; > + } else { > + addr = AREG(insn, 9); > + } > + > + dst = gen_load(s, opsize, addr, 1); > + tcg_gen_mov_i32(AREG(insn, 0), tmp); > + tcg_gen_addi_i32(AREG(insn, 9), addr, opsize_bytes(opsize)); I wonder if we should make this more generic. I'm thinking of transparently fixing case 3: /* Indirect postincrement. */ reg = AREG(insn, 0); result = gen_ldst(s, opsize, reg, val, what); /* ??? This is not exception safe. The instruction may still fault after this point. */ if (what == EA_STORE || !addrp) tcg_gen_addi_i32(reg, reg, opsize_bytes(opsize)); return result; If we add to DisasContext unsigned writeback_mask; TCGv writeback[8]; Then static TCGv get_areg(DisasContext *s, unsigned regno) { if (s->writeback_mask & (1 << regno)) { return s->writeback[regno]; } else { return cpu_aregs[regno]; } } static void delay_set_areg(DisasContext *s, unsigned regno, TCGv val, bool give_temp) { if (s->writeback_mask & (1 << regno)) { tcg_gen_mov_i32(s->writeback[regno], val); if (give_temp) { tcg_temp_free(val); } } else { s->writeback_mask |= 1 << regno; if (give_temp) { s->writeback[regno] = val; } else { TCGv tmp = tcg_temp_new(); tcg_gen_mov_i32(tmp, val); s->writeback[regno] = tmp; } } } static void do_writebacks(DisasContext *s) { unsigned mask = s->writeback_mask; if (mask) { s->writeback_mask = 0; do { unsigned regno = ctz32(mask); tcg_gen_mov_i32(cpu_aregs[regno], s->writeback[regno]); tcg_temp_free(s->writeback[regno]); mask &= mask - 1; } while (mask); } } where get_areg is used within the AREG macro, delay_set_areg is used in those cases where pre- and post-increment are needed, and do_writebacks is invoked at the end of disas_m68k_insn. This all pre-supposes that cmpm is not a special-case within the ISA, that any time one reuses a register after modification one sees the new value. Given the existing code within gen_ea, this would seem to be the case. Thoughts? r~
Le 01/11/2016 à 18:48, Richard Henderson a écrit : > On 10/27/2016 03:43 PM, Laurent Vivier wrote: >> +DISAS_INSN(cmpm) >> +{ >> + int opsize = insn_opsize(insn); >> + TCGv tmp = tcg_temp_new(); >> + TCGv src, dst, addr; >> + >> + src = gen_load(s, opsize, AREG(insn, 0), 1); >> + /* delay the update after the second gen_load() */ >> + tcg_gen_addi_i32(tmp, AREG(insn, 0), opsize_bytes(opsize)); >> + >> + /* but if the we use the same address register to >> + * read the second value, we must use the updated address >> + */ >> + if (REG(insn, 0) == REG(insn, 9)) { >> + addr = tmp; >> + } else { >> + addr = AREG(insn, 9); >> + } >> + >> + dst = gen_load(s, opsize, addr, 1); >> + tcg_gen_mov_i32(AREG(insn, 0), tmp); >> + tcg_gen_addi_i32(AREG(insn, 9), addr, opsize_bytes(opsize)); > > I wonder if we should make this more generic. > I'm thinking of transparently fixing > > case 3: /* Indirect postincrement. */ > reg = AREG(insn, 0); > result = gen_ldst(s, opsize, reg, val, what); > /* ??? This is not exception safe. The instruction may still > fault after this point. */ > if (what == EA_STORE || !addrp) > tcg_gen_addi_i32(reg, reg, opsize_bytes(opsize)); > return result; > > If we add to DisasContext > > unsigned writeback_mask; > TCGv writeback[8]; > > Then > > static TCGv get_areg(DisasContext *s, unsigned regno) > { > if (s->writeback_mask & (1 << regno)) { > return s->writeback[regno]; > } else { > return cpu_aregs[regno]; > } > } > > static void delay_set_areg(DisasContext *s, unsigned regno, > TCGv val, bool give_temp) > { > if (s->writeback_mask & (1 << regno)) { > tcg_gen_mov_i32(s->writeback[regno], val); > if (give_temp) { > tcg_temp_free(val); > } > } else { > s->writeback_mask |= 1 << regno; > if (give_temp) { > s->writeback[regno] = val; > } else { > TCGv tmp = tcg_temp_new(); > tcg_gen_mov_i32(tmp, val); > s->writeback[regno] = tmp; > } > } > } > > static void do_writebacks(DisasContext *s) > { > unsigned mask = s->writeback_mask; > if (mask) { > s->writeback_mask = 0; > do { > unsigned regno = ctz32(mask); > tcg_gen_mov_i32(cpu_aregs[regno], s->writeback[regno]); > tcg_temp_free(s->writeback[regno]); > mask &= mask - 1; > } while (mask); > } > } > > where get_areg is used within the AREG macro, delay_set_areg is used in > those cases where pre- and post-increment are needed, and do_writebacks > is invoked at the end of disas_m68k_insn. > > This all pre-supposes that cmpm is not a special-case within the ISA, > that any time one reuses a register after modification one sees the new > value. Given the existing code within gen_ea, this would seem to be the > case. > > Thoughts? I think it's really a good idea to manage this in a generic way. As you have already written 90% of the code, do you want to provide a patch? I can test this with coldfire kernel and 68020 linux-user container. Thanks, Laurent
diff --git a/target-m68k/translate.c b/target-m68k/translate.c index ee0ffe3..92e67eb 100644 --- a/target-m68k/translate.c +++ b/target-m68k/translate.c @@ -2173,6 +2173,33 @@ DISAS_INSN(cmpa) gen_update_cc_cmp(s, reg, src, opsize); } +DISAS_INSN(cmpm) +{ + int opsize = insn_opsize(insn); + TCGv tmp = tcg_temp_new(); + TCGv src, dst, addr; + + src = gen_load(s, opsize, AREG(insn, 0), 1); + /* delay the update after the second gen_load() */ + tcg_gen_addi_i32(tmp, AREG(insn, 0), opsize_bytes(opsize)); + + /* but if the we use the same address register to + * read the second value, we must use the updated address + */ + if (REG(insn, 0) == REG(insn, 9)) { + addr = tmp; + } else { + addr = AREG(insn, 9); + } + + dst = gen_load(s, opsize, addr, 1); + tcg_gen_mov_i32(AREG(insn, 0), tmp); + tcg_gen_addi_i32(AREG(insn, 9), addr, opsize_bytes(opsize)); + tcg_temp_free(tmp); + + gen_update_cc_cmp(s, dst, src, opsize); +} + DISAS_INSN(eor) { TCGv src; @@ -3414,6 +3441,7 @@ void register_m68k_insns (CPUM68KState *env) INSN(cmpa, b1c0, f1c0, CF_ISA_A); INSN(cmp, b000, f100, M68000); INSN(eor, b100, f100, M68000); + INSN(cmpm, b108, f138, M68000); INSN(cmpa, b0c0, f0c0, M68000); INSN(eor, b180, f1c0, CF_ISA_A); BASE(and, c000, f000);