Re: [Qemu-devel] [PATCH 11/60] AArch64: Add STP instruction emulation

[Richard Henderson]

On 09/26/2013 05:48 PM, Alexander Graf wrote:
> +static int get_reg(uint32_t inst)
> +{
> +    return get_bits(inst, 0, 5);
> +}

Surely get_rt or some such related to the actual field name, not something
so generic as "reg" which applies equally to any of several fields.

> +static void ldst_do_vec(DisasContext *s, int dest, TCGv_i64 tcg_addr_real,
> +                        int size, bool is_store)
> +{
> +    TCGv_i64 tcg_addr = tcg_temp_new_i64();
> +    int freg_offs = offsetof(CPUARMState, vfp.regs[dest * 2]);
> +
> +    /* we don't want to modify the caller's tcg_addr */
> +    tcg_gen_mov_i64(tcg_addr, tcg_addr_real);

Surely merge with ldst_do_vec_int, for the single case of 16 byte size?

> +
> +    if (!is_store) {
> +        /* normal ldst clears non-loaded bits */
> +        clear_fpreg(dest);
> +    }

Surely merge with ldst_do_vec_int, to avoid redundant stores into the
vfp register set?

> +static void handle_stp(DisasContext *s, uint32_t insn)
> +{

This handles both ldp and stp though, right?

> +    ldst_do(s, rt, tcg_addr, size, is_store, is_signed, is_vector);
> +    tcg_gen_addi_i64(tcg_addr, tcg_addr, 1 << size);
> +    ldst_do(s, rt2, tcg_addr, size, is_store, is_signed, is_vector);
> +    tcg_gen_subi_i64(tcg_addr, tcg_addr, 1 << size);

Why subtract instead of using another temporary?

> +    /* Typical major opcode encoding */
>      switch ((insn >> 24) & 0x1f) {
> +    case 0x08:
> +    case 0x09:
> +        if (get_bits(insn, 29, 1)) {
> +            handle_stp(s, insn);
> +        } else {
> +            unallocated_encoding(s);
> +        }
> +        break;
> +    case 0x0c:
> +        if (get_bits(insn, 29, 1)) {
> +            handle_stp(s, insn);
> +        } else {
> +            unallocated_encoding(s);
> +        }
> +        break;
> +    case 0x0d:
> +        if (get_bits(insn, 29, 1)) {
> +            handle_stp(s, insn);
> +        } else {
> +            unallocated_encoding(s);
> +        }
> +        break;

This leads me to believe that this isn't the best arrangement of the decoder.
I would expect the structure to look more like the arrangement in chapter C3,
especially the major encoding groups in table C3-1.


r~