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
