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

[Måns Rullgård]

Peter Maydell <address@hidden> writes:

> 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,

That makes two of us.

> 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).

Thanks, that sounds better.

> 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:

Great, more obsolete stuff.

>     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).

That's because it is neither.  It is CDP2, previously not decoded at all.
This seemed as logical a place as any to me.  If you disagree, please
say where you'd prefer that it go.

-- 
Måns Rullgård
address@hidden