Re: [Qemu-devel] [PATCH 15/60] AArch64: Add add instruction family emulation

[Richard Henderson]

On 09/26/2013 05:48 PM, Alexander Graf wrote:
> +    tcg_gen_mov_i64(tcg_src, cpu_reg(source));
> +    tcg_dst = cpu_reg(dest);
> +    if (extend) {
> +        if ((shift_amount & 0x7) > 4) {
> +            /* reserved value */
> +            unallocated_encoding(s);
> +        }
> +        if (!setflags) {
> +            tcg_gen_mov_i64(tcg_src, cpu_reg_sp(source));
> +            tcg_dst = cpu_reg_sp(dest);
> +        }
> +    } else {
> +        if (shift_type == 3) {
> +            /* reserved value */
> +            unallocated_encoding(s);
> +        }
> +        if (is_32bit && (shift_amount < 0)) {
> +            /* reserved value */
> +            unallocated_encoding(s);
> +        }
> +    }

You'd do better to load up the source and destination TCGv values in that IF
sequence, and emit one tcg_gen_mov_i64 afterward.

At the moment you're emitting two for the extend && !setflags case.

> +    if (extend) {
> +        tcg_op2 = tcg_temp_new_i64();
> +        reg_extend(tcg_op2, shift_amount >> 3, shift_amount & 0x7, rm);
> +    } else {
> +        tcg_op2 = get_shifti(rm, shift_type, shift_amount, is_32bit);
> +    }

Why does get_shifti return a temp, but reg_extend requires one to be passed in?

> +    if (is_32bit) {
> +        tcg_gen_ext32s_i64(tcg_src, tcg_src);
> +        tcg_gen_ext32s_i64(tcg_op2, tcg_op2);
> +    }

Why?  You'll zero-extend the result, and the flags setting will truncate the
inputs itself.

> +    if (sub_op) {
> +        tcg_gen_sub_i64(tcg_result, tcg_src, tcg_op2);
> +    } else {
> +        tcg_gen_add_i64(tcg_result, tcg_src, tcg_op2);
> +    }
> +
> +    if (is_carry) {
> +        TCGv_i64 tcg_carry = tcg_temp_new_i64();
> +        tcg_gen_shri_i64(tcg_carry, pstate, PSTATE_C_SHIFT);
> +        tcg_gen_andi_i64(tcg_carry, tcg_carry, 1);
> +        tcg_gen_add_i64(tcg_result, tcg_result, tcg_carry);
> +        if (sub_op) {
> +            tcg_gen_subi_i64(tcg_result, tcg_result, 1);
> +        }
> +        tcg_temp_free_i64(tcg_carry);
> +    }

For sub_op && is_carry, it's probably better to do exactly what the manual
says, rd = rn + ~rm + C, as opposed to rd = rn - rm + c - 1 as you do here.

This will be especially true if you eventually split up the flags as is done on
the A32 side.  One can compute rd plus the new carry via add2.


r~