Re: [Qemu-devel] [PATCH] target-arm: implement ARMv8 VSEL instruction

[Peter Maydell]

On 18 June 2013 15:30, Mans Rullgard <address@hidden> wrote:
> This adds support for the VSEL instruction introduced in ARMv8.
> It resides along with other new VFP instructions under the CDP2
> encoding which was previously unused.
>
> Signed-off-by: Mans Rullgard <address@hidden>

So I found this pretty confusing, which I think is an indication
that we need to start by cleaning up the existing v7 VFP/Neon
decode.

Specifically, currently we handle all Neon decode by just calling
the neon decode functions directly from the disas_arm_insn
and disas_thumb2_insn functions. We should move VFP to work
in the same way (ie take it out of disas_coproc_insn()).
Basically, the architecture manual treats them as part of the
core instruction set, and we should make our decoder do the same.

The (existing) coproc decode is also confusing, and would benefit
a lot from a comment at the top of disas_coproc_insn specifying
the opcode patterns that can reach it.

> +    if (((insn >> 23) & 1) == 0) {
> +        /* vsel */
> +        int cc = (insn >> 20) & 3;
> +        int cond = (cc << 2) | (((cc << 1) ^ cc) & 2);
> +        int pass_label = gen_new_label();
> +
> +        gen_mov_F0_vreg(dp, rn);
> +        gen_mov_vreg_F0(dp, rd);
> +        gen_test_cc(cond, pass_label);
> +        gen_mov_F0_vreg(dp, rm);
> +        gen_mov_vreg_F0(dp, rd);
> +        gen_set_label(pass_label);

You can generate better code with the TCG movcond op.
Luckily you don't actually have to duplicate the whole of
gen_test_cc only doing movconds, because there are only actually
4 encodable conditions here (3 of which turn into a single
movcond; the fourth requires two consecutive movcond ops).

Also I don't think we should introduce any new uses of F0/F1.
You can just load a VFP register into a TCG temp like this:

    ftmp = tcg_temp_new_i32();
    tcg_gen_ld_f32(ftmp, cpu_env, vfp_reg_offset(0, rd));

operate on it as usual, and store:
    tcg_gen_st_f32(ftmp, cpu_env, vfp_reg_offset(0, rd));
    tcg_temp_free_i32(ftmp);

(similarly for double).

> @@ -6699,6 +6742,12 @@ static void disas_arm_insn(CPUARMState * env, 
> DisasContext *s)
>              }
>              return; /* v7MP: Unallocated memory hint: must NOP */
>          }
> +        if ((insn & 0x0f000010) == 0x0e000000) {
> +            /* cdp2 */
> +            if (disas_coproc_insn(env, s, insn))
> +                goto illegal_op;
> +            return;
> +        }

This hunk is oddly placed, because it's neither next to the neon
decode (which is further up) nor the mrc2/mcr2 decode (which is
further down).

thanks
-- PMM