Peter Maydell <peter.mayd...@linaro.org> writes:
> On 18 June 2013 15:30, Mans Rullgard <m...@mansr.com> 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 <m...@mansr.com>
>
> 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
m...@mansr.com
