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

[Richard Henderson]

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~