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

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

[Mans Rullgard]

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 <mans@mansr.com>
---
Basic testing performed but not every combination.
---
 target-arm/translate.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 49 insertions(+), 2 deletions(-)

diff --git a/target-arm/translate.c b/target-arm/translate.c
index f7089b2..eb9423b 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -2546,6 +2546,40 @@ static TCGv_i32 gen_load_and_replicate(DisasContext *s, TCGv_i32 addr, int size)
     return tmp;
 }
 
+static int disas_v8vfp_insn(CPUARMState *env, DisasContext *s, uint32_t insn)
+{
+    uint32_t rd, rn, rm;
+    int dp = (insn >> 8) & 1;
+
+    if (dp) {
+        VFP_DREG_D(rd, insn);
+        VFP_DREG_N(rn, insn);
+        VFP_DREG_M(rm, insn);
+    } else {
+        rd = VFP_SREG_D(insn);
+        rn = VFP_SREG_N(insn);
+        rm = VFP_SREG_M(insn);
+    }
+
+    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);
+
+        return 0;
+    }
+
+    return 1;
+}
+
 /* Disassemble a VFP instruction.  Returns nonzero if an error occurred
    (ie. an undefined instruction).  */
 static int disas_vfp_insn(CPUARMState * env, DisasContext *s, uint32_t insn)
@@ -2568,6 +2602,11 @@ static int disas_vfp_insn(CPUARMState * env, DisasContext *s, uint32_t insn)
             && rn != ARM_VFP_MVFR1 && rn != ARM_VFP_MVFR0)
             return 1;
     }
+    if (((insn >> 28) & 0xf) == 0xf) {
+        if (!arm_feature(env, ARM_FEATURE_V8))
+            return 1;
+        return disas_v8vfp_insn(env, s, insn);
+    }
     dp = ((insn & 0xf00) == 0xb00);
     switch ((insn >> 24) & 0xf) {
     case 0xe:
@@ -6241,6 +6280,10 @@ static int disas_coproc_insn(CPUARMState * env, DisasContext *s, uint32_t insn)
         /* cdp */
         return 1;
     }
+    if (((insn >> 28) & 0xf) == 0xf) {
+        /* cdp2 */
+        return 1;
+    }
 
     crm = insn & 0xf;
     if (is64) {
@@ -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;
+        }
 
         if ((insn & 0x0ffffdff) == 0x01010000) {
             ARCH(6);
@@ -8684,8 +8733,6 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
             if (disas_neon_data_insn(env, s, insn))
                 goto illegal_op;
         } else {
-            if (insn & (1 << 28))
-                goto illegal_op;
             if (disas_coproc_insn (env, s, insn))
                 goto illegal_op;
         }
-- 
1.8.2.1

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

[Peter Maydell]

On 18 June 2013 15:30, Mans Rullgard <mans@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 <mans@mansr.com>

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

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

[Måns Rullgård]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 18 June 2013 15:30, Mans Rullgard <mans@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 <mans@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
mans@mansr.com

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

[Peter Maydell]

On 21 June 2013 00:25, Måns Rullgård <mans@mansr.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>> On 18 June 2013 15:30, Mans Rullgard <mans@mansr.com> wrote:
>>> @@ -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.

Well, if we're treating it in the same way as Neon (ie pulling
the VFP decode out of the decode_coproc() function as I suggest),
then the logical place to put VFP decode is next to the Neon
decode.

At that point the need to actually decode CDP2 into a call
to disas_coproc_insn() goes away, but if we did want to include
it then the logical place to put that decode is next to the
MCR2/MRC2 decode, because both CDP2 and MCR2/MRC2 deal with
coprocessor instructions and are in the same table in the ARM ARM.

-- PMM

[Qemu-devel] [PATCH] target-arm: Implement ARMv8 VSEL instruction.

[Will Newton]

This adds support for the VSEL floating point selection instruction
which was added in ARMv8. It is based on the previous patch[1] from
Mans Rullgard, but attempts to addres the feedback given on that patch.

[1] http://lists.nongnu.org/archive/html/qemu-devel/2013-06/msg03117.html

Signed-off-by: Will Newton <will.newton@linaro.org>
---
 target-arm/translate.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 121 insertions(+)

diff --git a/target-arm/translate.c b/target-arm/translate.c
index 998bde2..7bfd606 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -2617,6 +2617,114 @@ static TCGv_i32 gen_load_and_replicate(DisasContext *s, TCGv_i32 addr, int size)
     return tmp;
 }

+static int disas_v8vfp_insn(CPUARMState *env, DisasContext *s, uint32_t insn)
+{
+    uint32_t rd, rn, rm, dp = (insn >> 8) & 1;
+
+    if (!s->vfp_enabled)
+      return 1;
+
+    if (dp) {
+        VFP_DREG_D(rd, insn);
+        VFP_DREG_N(rn, insn);
+        VFP_DREG_M(rm, insn);
+    } else {
+        rd = VFP_SREG_D(insn);
+        rn = VFP_SREG_N(insn);
+        rm = VFP_SREG_M(insn);
+    }
+
+    if (((insn >> 23) & 1) == 0) {
+        /* vsel */
+        uint32_t cc = (insn >> 20) & 3;
+	TCGv_i32 tmp, zero;
+
+	zero = tcg_const_tl(0);
+
+	if (dp) {
+	    TCGv_i64 ftmp1, ftmp2, ftmp3;
+
+	    ftmp1 = tcg_temp_new_i64();
+	    ftmp2 = tcg_temp_new_i64();
+	    ftmp3 = tcg_temp_new_i64();
+	    tcg_gen_ld_f64(ftmp1, cpu_env, vfp_reg_offset(dp, rn));
+	    tcg_gen_ld_f64(ftmp2, cpu_env, vfp_reg_offset(dp, rm));
+	    switch (cc) {
+	    case 0: /* eq: Z */
+		tcg_gen_movcond_i64(TCG_COND_EQ, ftmp3, cpu_ZF, zero,
+				    ftmp1, ftmp2);
+		break;
+	    case 1: /* vs: V */
+		tcg_gen_movcond_i64(TCG_COND_LT, ftmp3, cpu_VF, zero,
+				    ftmp1, ftmp2);
+		break;
+	    case 2: /* ge: N == V -> N ^ V == 0 */
+		tmp = tcg_temp_new_i32();
+		tcg_gen_xor_i32(tmp, cpu_VF, cpu_NF);
+		tcg_gen_movcond_i64(TCG_COND_GE, ftmp3, tmp, zero,
+				    ftmp1, ftmp2);
+		tcg_temp_free_i32(tmp);
+		break;
+	    case 3: /* gt: !Z && N == V */
+		tcg_gen_movcond_i64(TCG_COND_NE, ftmp3, cpu_ZF, zero,
+				    ftmp1, ftmp2);
+		tmp = tcg_temp_new_i32();
+		tcg_gen_xor_i32(tmp, cpu_VF, cpu_NF);
+		tcg_gen_movcond_i64(TCG_COND_GE, ftmp3, tmp, zero,
+				    ftmp3, ftmp2);
+		tcg_temp_free_i32(tmp);
+		break;
+	    }
+	    tcg_gen_st_f64(ftmp3, cpu_env, vfp_reg_offset(dp, rd));
+	    tcg_temp_free_i64(ftmp1);
+	    tcg_temp_free_i64(ftmp2);
+	    tcg_temp_free_i64(ftmp3);
+	} else {
+	    TCGv_i32 ftmp1, ftmp2, ftmp3;
+
+	    ftmp1 = tcg_temp_new_i32();
+	    ftmp2 = tcg_temp_new_i32();
+	    ftmp3 = tcg_temp_new_i32();
+	    tcg_gen_ld_f32(ftmp1, cpu_env, vfp_reg_offset(dp, rn));
+	    tcg_gen_ld_f32(ftmp2, cpu_env, vfp_reg_offset(dp, rm));
+	    switch (cc) {
+	    case 0: /* eq: Z */
+		tcg_gen_movcond_i32(TCG_COND_EQ, ftmp3, cpu_ZF, zero,
+				    ftmp1, ftmp2);
+		break;
+	    case 1: /* vs: V */
+		tcg_gen_movcond_i32(TCG_COND_LT, ftmp3, cpu_VF, zero,
+				    ftmp1, ftmp2);
+		break;
+	    case 2: /* ge: N == V -> N ^ V == 0 */
+		tmp = tcg_temp_new_i32();
+		tcg_gen_xor_i32(tmp, cpu_VF, cpu_NF);
+		tcg_gen_movcond_i32(TCG_COND_GE, ftmp3, tmp, zero,
+				    ftmp1, ftmp2);
+		tcg_temp_free_i32(tmp);
+		break;
+	    case 3: /* gt: !Z && N == V */
+		tcg_gen_movcond_i32(TCG_COND_NE, ftmp3, cpu_ZF, zero,
+				    ftmp1, ftmp2);
+		tmp = tcg_temp_new_i32();
+		tcg_gen_xor_i32(tmp, cpu_VF, cpu_NF);
+		tcg_gen_movcond_i32(TCG_COND_GE, ftmp3, tmp, zero,
+				    ftmp3, ftmp2);
+		tcg_temp_free_i32(tmp);
+		break;
+	    }
+	    tcg_gen_st_f32(ftmp3, cpu_env, vfp_reg_offset(dp, rd));
+	    tcg_temp_free_i32(ftmp1);
+	    tcg_temp_free_i32(ftmp2);
+	    tcg_temp_free_i32(ftmp3);
+	}
+
+        return 0;
+    }
+
+    return 1;
+}
+
 /* Disassemble a VFP instruction.  Returns nonzero if an error occurred
    (ie. an undefined instruction).  */
 static int disas_vfp_insn(CPUARMState * env, DisasContext *s, uint32_t insn)
@@ -6756,6 +6864,13 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
                 goto illegal_op;
             return;
         }
+	if ((insn & 0x0f800e50) == 0x0e000a00) {
+	    /* ARMv8 VFP.  */
+	    ARCH(8);
+
+	    if (disas_v8vfp_insn(env, s, insn))
+		goto illegal_op;
+	}
         if (((insn & 0x0f30f000) == 0x0510f000) ||
             ((insn & 0x0f30f010) == 0x0710f000)) {
             if ((insn & (1 << 22)) == 0) {
@@ -8768,6 +8883,12 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
             insn = (insn & 0xe2ffffff) | ((insn & (1 << 28)) >> 4) | (1 << 28);
             if (disas_neon_data_insn(env, s, insn))
                 goto illegal_op;
+	} else if ((insn & 0x0f800e50) == 0x0e000a00) {
+	    /* ARMv8 VFP.  */
+	    ARCH(8);
+
+	    if (disas_v8vfp_insn(env, s, insn))
+		goto illegal_op;
         } else {
             if (insn & (1 << 28))
                 goto illegal_op;
-- 
1.8.1.4

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

[Alex Bennée]

will.newton@linaro.org writes:

<snip>
> @@ -6756,6 +6864,13 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
>                  goto illegal_op;
>              return;
>          }
> +	if ((insn & 0x0f800e50) == 0x0e000a00) {
> +	    /* ARMv8 VFP.  */
> +	    ARCH(8);
> +
> +	    if (disas_v8vfp_insn(env, s, insn))
> +		goto illegal_op;
> +	}
>          if (((insn & 0x0f30f000) == 0x0510f000) ||
>              ((insn & 0x0f30f010) == 0x0710f000)) {
>              if ((insn & (1 << 22)) == 0) {
> @@ -8768,6 +8883,12 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
>              insn = (insn & 0xe2ffffff) | ((insn & (1 << 28)) >> 4) | (1 << 28);
>              if (disas_neon_data_insn(env, s, insn))
>                  goto illegal_op;
> +	} else if ((insn & 0x0f800e50) == 0x0e000a00) {
> +	    /* ARMv8 VFP.  */
> +	    ARCH(8);
> +
> +	    if (disas_v8vfp_insn(env, s, insn))
> +		goto illegal_op;
>          } else {
>              if (insn & (1 << 28))
>                  goto illegal_op;

I wonder is there a better way to deal with these masks for checking
instruction type? I only mention it because the same constant is used
twice and should it ever be changed there is a risk of one being missed
(although conceivably thumb encoding could be different?).

I appreciate the disassembler code is already a mass of magic constants
so it's not a major thing for me.

Cheers,


-- 
Alex Bennée

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

[Peter Maydell]

On 3 October 2013 20:31, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> will.newton@linaro.org writes:
>
> <snip>
>> @@ -6756,6 +6864,13 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
>>                  goto illegal_op;
>>              return;
>>          }
>> +     if ((insn & 0x0f800e50) == 0x0e000a00) {
>> +         /* ARMv8 VFP.  */
>> +         ARCH(8);
>> +
>> +         if (disas_v8vfp_insn(env, s, insn))
>> +             goto illegal_op;
>> +     }
>>          if (((insn & 0x0f30f000) == 0x0510f000) ||
>>              ((insn & 0x0f30f010) == 0x0710f000)) {
>>              if ((insn & (1 << 22)) == 0) {
>> @@ -8768,6 +8883,12 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
>>              insn = (insn & 0xe2ffffff) | ((insn & (1 << 28)) >> 4) | (1 << 28);
>>              if (disas_neon_data_insn(env, s, insn))
>>                  goto illegal_op;
>> +     } else if ((insn & 0x0f800e50) == 0x0e000a00) {
>> +         /* ARMv8 VFP.  */
>> +         ARCH(8);
>> +
>> +         if (disas_v8vfp_insn(env, s, insn))
>> +             goto illegal_op;
>>          } else {
>>              if (insn & (1 << 28))
>>                  goto illegal_op;
>
> I wonder is there a better way to deal with these masks for checking
> instruction type? I only mention it because the same constant is used
> twice and should it ever be changed there is a risk of one being missed
> (although conceivably thumb encoding could be different?).

The v8 ARM ARM actually integrates this new instruction into the
decode tables for the existing VFP insns (it's an entry in table F5-16
'three register data floating point instructions'), which makes me wonder
if we should put the decode into the existing disas_vfp_insn()...

-- PMM