Re: [Qemu-devel] [PATCH v2 1/3] target-m68k: add cmpm

[Laurent Vivier]

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