* [Qemu-devel] [PATCH] Target-arm: Add the THUMB_DSP feature
@ 2015-06-01 14:30 Aurelio C. Remonda
2015-06-01 17:54 ` Peter Maydell
0 siblings, 1 reply; 3+ messages in thread
From: Aurelio C. Remonda @ 2015-06-01 14:30 UTC (permalink / raw)
To: qemu-devel, ilg, peter.maydell, martin.galvan, daniel.gutson
I created an ARM_FEATURE_THUMB_DSP to be added to any non-M
thumb2-compatible CPU that uses DSP instructions.
There are 85 DSP instructions (all of them thumb2). On disas_thumb2_insn
the DSP feature is tested before the instruction is generated; if it's not
enabled then its an illegal op.
Signed-off-by: Aurelio C. Remonda <aurelioremonda@gmail.com>
---
target-arm/cpu.h | 1 +
target-arm/translate.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 107 insertions(+), 4 deletions(-)
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 21b5b8e..2e03d8e 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -890,6 +890,7 @@ enum arm_features {
ARM_FEATURE_V8_SHA1, /* implements SHA1 part of v8 Crypto Extensions */
ARM_FEATURE_V8_SHA256, /* implements SHA256 part of v8 Crypto Extensions */
ARM_FEATURE_V8_PMULL, /* implements PMULL part of v8 Crypto Extensions */
+ ARM_FEATURE_THUMB_DSP, /* DSP insns supported in the Thumb encodings */
};
static inline int arm_feature(CPUARMState *env, int feature)
diff --git a/target-arm/translate.c b/target-arm/translate.c
index 39692d7..2d14a2c 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -9444,6 +9444,10 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
op = (insn >> 21) & 0xf;
if (op == 6) {
+ if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
+ /* pkhtb, pkfbt are DSP instructions */
+ goto illegal_op;
+ }
/* Halfword pack. */
tmp = load_reg(s, rn);
tmp2 = load_reg(s, rm);
@@ -9518,13 +9522,35 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
switch (op) {
case 0: gen_sxth(tmp); break;
case 1: gen_uxth(tmp); break;
- case 2: gen_sxtb16(tmp); break;
- case 3: gen_uxtb16(tmp); break;
+ case 2:
+ if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
+ /* sxtab16, sxtb16 are DSP instructions */
+ tcg_temp_free_i32(tmp);
+ goto illegal_op;
+ }
+ gen_sxtb16(tmp);
+ break;
+ case 3:
+ if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
+ /* uxtb16, uxtab16 are DSP instructions */
+ tcg_temp_free_i32(tmp);
+ goto illegal_op;
+ }
+ gen_uxtb16(tmp);
+ break;
case 4: gen_sxtb(tmp); break;
case 5: gen_uxtb(tmp); break;
default: goto illegal_op;
}
if (rn != 15) {
+ if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
+ /* sxtab, sxtah, uxtab, uxtah are DSP instructions.
+ * sxtb, sxth, uxtb, uxth are not DSP according to
+ * ARMv7-M Architecture Reference Manual
+ */
+ tcg_temp_free_i32(tmp);
+ goto illegal_op;
+ }
tmp2 = load_reg(s, rn);
if ((op >> 1) == 1) {
gen_add16(tmp, tmp2);
@@ -9537,6 +9563,12 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
break;
case 2: /* SIMD add/subtract. */
op = (insn >> 20) & 7;
+ if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
+ /* add16, sub16, asx, sax, add8, sub8 (with q, s, sh, u, uh,
+ * and uq variants) and usad8, usada8
+ */
+ goto illegal_op;
+ }
shift = (insn >> 4) & 7;
if ((op & 3) == 3 || (shift & 3) == 3)
goto illegal_op;
@@ -9550,6 +9582,10 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
op = ((insn >> 17) & 0x38) | ((insn >> 4) & 7);
if (op < 4) {
/* Saturating add/subtract. */
+ if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
+ /* qsub, qadd, qdadd, qdsub are DSP instructions. */
+ goto illegal_op;
+ }
tmp = load_reg(s, rn);
tmp2 = load_reg(s, rm);
if (op & 1)
@@ -9575,6 +9611,11 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
gen_revsh(tmp);
break;
case 0x10: /* sel */
+ if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
+ /* sel is a DSP instruction. */
+ tcg_temp_free_i32(tmp);
+ goto illegal_op;
+ }
tmp2 = load_reg(s, rm);
tmp3 = tcg_temp_new_i32();
tcg_gen_ld_i32(tmp3, cpu_env, offsetof(CPUARMState, GE));
@@ -9640,6 +9681,14 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
}
break;
case 1: /* 16 x 16 -> 32 */
+ if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
+ /* smlabb, smlabt, smlatb, smlatt, smulbb, smulbt, smultt
+ * and smultb are DSP instructions
+ */
+ tcg_temp_free_i32(tmp);
+ tcg_temp_free_i32(tmp2);
+ goto illegal_op;
+ }
gen_mulxy(tmp, tmp2, op & 2, op & 1);
tcg_temp_free_i32(tmp2);
if (rs != 15) {
@@ -9650,6 +9699,12 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
break;
case 2: /* Dual multiply add. */
case 4: /* Dual multiply subtract. */
+ if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
+ /* smlad, smladx, smlsd, smusd are DSP instructions */
+ tcg_temp_free_i32(tmp);
+ tcg_temp_free_i32(tmp2);
+ goto illegal_op;
+ }
if (op)
gen_swap_half(tmp2);
gen_smul_dual(tmp, tmp2);
@@ -9672,6 +9727,12 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
}
break;
case 3: /* 32 * 16 -> 32msb */
+ if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
+ /* smlawb, smlawt, smulwt, smulwb are DSP instructions */
+ tcg_temp_free_i32(tmp);
+ tcg_temp_free_i32(tmp2);
+ goto illegal_op;
+ }
if (op)
tcg_gen_sari_i32(tmp2, tmp2, 16);
else
@@ -9689,6 +9750,14 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
}
break;
case 5: case 6: /* 32 * 32 -> 32msb (SMMUL, SMMLA, SMMLS) */
+ if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
+ /* smmla, smmls, smmul, smuad, smmlar,
+ * smmlsr, smmulr are DSP instructions
+ */
+ tcg_temp_free_i32(tmp);
+ tcg_temp_free_i32(tmp2);
+ goto illegal_op;
+ }
tmp64 = gen_muls_i64_i32(tmp, tmp2);
if (rs != 15) {
tmp = load_reg(s, rs);
@@ -9735,6 +9804,12 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
store_reg(s, rd, tmp);
} else if ((op & 0xe) == 0xc) {
/* Dual multiply accumulate long. */
+ if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
+ /* smlald, smlsld are DSP instructions */
+ tcg_temp_free_i32(tmp);
+ tcg_temp_free_i32(tmp2);
+ goto illegal_op;
+ }
if (op & 1)
gen_swap_half(tmp2);
gen_smul_dual(tmp, tmp2);
@@ -9758,6 +9833,14 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
} else {
if (op & 8) {
/* smlalxy */
+ if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
+ /* smlalbb, smlalbt, smlaltb, smlaltt
+ * are DSP instructions
+ */
+ tcg_temp_free_i32(tmp2);
+ tcg_temp_free_i32(tmp);
+ goto illegal_op;
+ }
gen_mulxy(tmp, tmp2, op & 2, op & 1);
tcg_temp_free_i32(tmp2);
tmp64 = tcg_temp_new_i64();
@@ -9770,6 +9853,11 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
}
if (op & 4) {
/* umaal */
+ if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
+ /* ummal is a DSP instruction */
+ tcg_temp_free_i64(tmp64);
+ goto illegal_op;
+ }
gen_addq_lo(s, tmp64, rs);
gen_addq_lo(s, tmp64, rd);
} else if (op & 0x40) {
@@ -10034,14 +10122,28 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
tmp2 = tcg_const_i32(imm);
if (op & 4) {
/* Unsigned. */
- if ((op & 1) && shift == 0)
+ if ((op & 1) && shift == 0){
+ if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
+ /* usat16 is a DSP instruction */
+ tcg_temp_free_i32(tmp);
+ tcg_temp_free_i32(tmp2);
+ goto illegal_op;
+ }
gen_helper_usat16(tmp, cpu_env, tmp, tmp2);
+ }
else
gen_helper_usat(tmp, cpu_env, tmp, tmp2);
} else {
/* Signed. */
- if ((op & 1) && shift == 0)
+ if ((op & 1) && shift == 0){
+ if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
+ /* ssat16 is a DSP instruction */
+ tcg_temp_free_i32(tmp);
+ tcg_temp_free_i32(tmp2);
+ goto illegal_op;
+ }
gen_helper_ssat16(tmp, cpu_env, tmp, tmp2);
+ }
else
gen_helper_ssat(tmp, cpu_env, tmp, tmp2);
}
1.9.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] Target-arm: Add the THUMB_DSP feature
2015-06-01 14:30 [Qemu-devel] [PATCH] Target-arm: Add the THUMB_DSP feature Aurelio C. Remonda
@ 2015-06-01 17:54 ` Peter Maydell
2015-06-01 18:36 ` Peter Crosthwaite
0 siblings, 1 reply; 3+ messages in thread
From: Peter Maydell @ 2015-06-01 17:54 UTC (permalink / raw)
To: Aurelio C. Remonda
Cc: Liviu Ionescu, Daniel Gutson, QEMU Developers, Martin Galvan
On 1 June 2015 at 15:30, Aurelio C. Remonda <aurelioremonda@gmail.com> wrote:
Thanks for sending this patch. I have a few comments below.
> I created an ARM_FEATURE_THUMB_DSP to be added to any non-M
> thumb2-compatible CPU that uses DSP instructions.
> There are 85 DSP instructions (all of them thumb2). On disas_thumb2_insn
> the DSP feature is tested before the instruction is generated; if it's not
> enabled then its an illegal op.
Our general style for commit messages tends to be a bit more
impersonal, so for instance "Create an ARM_FEATURE_THUMB_DSP"
rather than "I created...".
> Signed-off-by: Aurelio C. Remonda <aurelioremonda@gmail.com>
As I noted in the other patch, the code that sets the feature bit
for non-M-profile Thumb2 CPUs needs to go in this patch.
> ---
> target-arm/cpu.h | 1 +
> target-arm/translate.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 107 insertions(+), 4 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 21b5b8e..2e03d8e 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -890,6 +890,7 @@ enum arm_features {
> ARM_FEATURE_V8_SHA1, /* implements SHA1 part of v8 Crypto Extensions */
> ARM_FEATURE_V8_SHA256, /* implements SHA256 part of v8 Crypto Extensions */
> ARM_FEATURE_V8_PMULL, /* implements PMULL part of v8 Crypto Extensions */
> + ARM_FEATURE_THUMB_DSP, /* DSP insns supported in the Thumb encodings */
> };
>
> static inline int arm_feature(CPUARMState *env, int feature)
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 39692d7..2d14a2c 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -9444,6 +9444,10 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
>
> op = (insn >> 21) & 0xf;
> if (op == 6) {
> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
> + /* pkhtb, pkfbt are DSP instructions */
These comments aren't really necessary I think -- it's obvious from
the feature bit we're testing and from the code below what's
going on.
> + goto illegal_op;
> + }
> /* Halfword pack. */
> tmp = load_reg(s, rn);
> tmp2 = load_reg(s, rm);
> @@ -9518,13 +9522,35 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
> switch (op) {
> case 0: gen_sxth(tmp); break;
> case 1: gen_uxth(tmp); break;
> - case 2: gen_sxtb16(tmp); break;
> - case 3: gen_uxtb16(tmp); break;
> + case 2:
> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
> + /* sxtab16, sxtb16 are DSP instructions */
> + tcg_temp_free_i32(tmp);
> + goto illegal_op;
> + }
> + gen_sxtb16(tmp);
> + break;
> + case 3:
> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
> + /* uxtb16, uxtab16 are DSP instructions */
> + tcg_temp_free_i32(tmp);
> + goto illegal_op;
> + }
> + gen_uxtb16(tmp);
> + break;
> case 4: gen_sxtb(tmp); break;
> case 5: gen_uxtb(tmp); break;
> default: goto illegal_op;
> }
> if (rn != 15) {
> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
> + /* sxtab, sxtah, uxtab, uxtah are DSP instructions.
> + * sxtb, sxth, uxtb, uxth are not DSP according to
> + * ARMv7-M Architecture Reference Manual
> + */
> + tcg_temp_free_i32(tmp);
> + goto illegal_op;
> + }
It looks like it would be fairly easy to hoist the illegal_op checks
up above the load_reg() call, which then means we don't need to do
a temp_free. You just need an extra switch on op like
switch (op) {
case 0: /* SXTAH, SXTH */
case 1: /* UXTAH, UXTH */
case 4: /* SXTAB, SXTB */
case 5: /* UXTAB, UXTB */
break;
case 2: /* SXTAB16, SXTB16 */
case 3: /* UXTAB16, UXTB16 */
if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
goto illegal_op;
}
break;
default:
goto illegal_op;
}
(The default: case in the original switch then becomes a
g_assert_not_reached();)
You can check for rn != 15 up here too.
> tmp2 = load_reg(s, rn);
> if ((op >> 1) == 1) {
> gen_add16(tmp, tmp2);
> @@ -9537,6 +9563,12 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
> break;
> case 2: /* SIMD add/subtract. */
> op = (insn >> 20) & 7;
> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
> + /* add16, sub16, asx, sax, add8, sub8 (with q, s, sh, u, uh,
> + * and uq variants) and usad8, usada8
> + */
If we want to document the instructions handled by this case then
the comment belongs at the start of it, before the assignment to 'op'.
> + goto illegal_op;
> + }
> shift = (insn >> 4) & 7;
> if ((op & 3) == 3 || (shift & 3) == 3)
> goto illegal_op;
> @@ -9550,6 +9582,10 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
> op = ((insn >> 17) & 0x38) | ((insn >> 4) & 7);
> if (op < 4) {
> /* Saturating add/subtract. */
> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
> + /* qsub, qadd, qdadd, qdsub are DSP instructions. */
> + goto illegal_op;
> + }
> tmp = load_reg(s, rn);
> tmp2 = load_reg(s, rm);
> if (op & 1)
> @@ -9575,6 +9611,11 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
> gen_revsh(tmp);
> break;
> case 0x10: /* sel */
> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
> + /* sel is a DSP instruction. */
> + tcg_temp_free_i32(tmp);
> + goto illegal_op;
> + }
This check could also be hoisted up to above the allocation of tmp.
Your indentation on this section seems to have gone wrong.
If you run your patches through scripts/checkpatch.pl they will catch
this kind of style error for you.
> tmp2 = load_reg(s, rm);
> tmp3 = tcg_temp_new_i32();
> tcg_gen_ld_i32(tmp3, cpu_env, offsetof(CPUARMState, GE));
> @@ -9640,6 +9681,14 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
> }
> break;
> case 1: /* 16 x 16 -> 32 */
> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
> + /* smlabb, smlabt, smlatb, smlatt, smulbb, smulbt, smultt
> + * and smultb are DSP instructions
> + */
> + tcg_temp_free_i32(tmp);
> + tcg_temp_free_i32(tmp2);
> + goto illegal_op;
> + }
> gen_mulxy(tmp, tmp2, op & 2, op & 1);
> tcg_temp_free_i32(tmp2);
> if (rs != 15) {
> @@ -9650,6 +9699,12 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
> break;
> case 2: /* Dual multiply add. */
> case 4: /* Dual multiply subtract. */
> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
> + /* smlad, smladx, smlsd, smusd are DSP instructions */
> + tcg_temp_free_i32(tmp);
> + tcg_temp_free_i32(tmp2);
> + goto illegal_op;
> + }
> if (op)
> gen_swap_half(tmp2);
> gen_smul_dual(tmp, tmp2);
> @@ -9672,6 +9727,12 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
> }
> break;
> case 3: /* 32 * 16 -> 32msb */
> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
> + /* smlawb, smlawt, smulwt, smulwb are DSP instructions */
> + tcg_temp_free_i32(tmp);
> + tcg_temp_free_i32(tmp2);
> + goto illegal_op;
> + }
> if (op)
> tcg_gen_sari_i32(tmp2, tmp2, 16);
> else
> @@ -9689,6 +9750,14 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
> }
> break;
> case 5: case 6: /* 32 * 32 -> 32msb (SMMUL, SMMLA, SMMLS) */
> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
> + /* smmla, smmls, smmul, smuad, smmlar,
> + * smmlsr, smmulr are DSP instructions
> + */
> + tcg_temp_free_i32(tmp);
> + tcg_temp_free_i32(tmp2);
> + goto illegal_op;
> + }
> tmp64 = gen_muls_i64_i32(tmp, tmp2);
> if (rs != 15) {
> tmp = load_reg(s, rs);
Again, all these checks inside this switch() would be more cleanly handled
by having an initial switch() before we allocate the temporaries that deals
with the illegal_op cases.
> @@ -9735,6 +9804,12 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
> store_reg(s, rd, tmp);
> } else if ((op & 0xe) == 0xc) {
> /* Dual multiply accumulate long. */
> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
> + /* smlald, smlsld are DSP instructions */
> + tcg_temp_free_i32(tmp);
> + tcg_temp_free_i32(tmp2);
> + goto illegal_op;
> + }
> if (op & 1)
> gen_swap_half(tmp2);
> gen_smul_dual(tmp, tmp2);
> @@ -9758,6 +9833,14 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
> } else {
> if (op & 8) {
> /* smlalxy */
> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
> + /* smlalbb, smlalbt, smlaltb, smlaltt
> + * are DSP instructions
> + */
> + tcg_temp_free_i32(tmp2);
> + tcg_temp_free_i32(tmp);
> + goto illegal_op;
> + }
> gen_mulxy(tmp, tmp2, op & 2, op & 1);
> tcg_temp_free_i32(tmp2);
> tmp64 = tcg_temp_new_i64();
Doing the illegal_op checks early for these two would require
more painful surgery to the code so I think they're OK like this.
> @@ -9770,6 +9853,11 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
> }
> if (op & 4) {
> /* umaal */
> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
> + /* ummal is a DSP instruction */
> + tcg_temp_free_i64(tmp64);
> + goto illegal_op;
> + }
> gen_addq_lo(s, tmp64, rs);
> gen_addq_lo(s, tmp64, rd);
> } else if (op & 0x40) {
> @@ -10034,14 +10122,28 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
> tmp2 = tcg_const_i32(imm);
> if (op & 4) {
> /* Unsigned. */
> - if ((op & 1) && shift == 0)
> + if ((op & 1) && shift == 0){
You need a space before the '{' (again, checkpatch will tell you this).
> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
> + /* usat16 is a DSP instruction */
> + tcg_temp_free_i32(tmp);
> + tcg_temp_free_i32(tmp2);
> + goto illegal_op;
> + }
> gen_helper_usat16(tmp, cpu_env, tmp, tmp2);
> + }
> else
> gen_helper_usat(tmp, cpu_env, tmp, tmp2);
...and since you're touching the if() statement you need to also add
braces on the else half of it. (Checkpatch again.)
> } else {
> /* Signed. */
> - if ((op & 1) && shift == 0)
> + if ((op & 1) && shift == 0){
> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
> + /* ssat16 is a DSP instruction */
> + tcg_temp_free_i32(tmp);
> + tcg_temp_free_i32(tmp2);
> + goto illegal_op;
> + }
> gen_helper_ssat16(tmp, cpu_env, tmp, tmp2);
> + }
> else
> gen_helper_ssat(tmp, cpu_env, tmp, tmp2);
> }
Same comments apply for this if..else.
> 1.9.1
>
thanks
-- PMM
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] Target-arm: Add the THUMB_DSP feature
2015-06-01 17:54 ` Peter Maydell
@ 2015-06-01 18:36 ` Peter Crosthwaite
0 siblings, 0 replies; 3+ messages in thread
From: Peter Crosthwaite @ 2015-06-01 18:36 UTC (permalink / raw)
To: Peter Maydell
Cc: Daniel Gutson, Liviu Ionescu, Aurelio C. Remonda, QEMU Developers,
Martin Galvan
On Mon, Jun 1, 2015 at 10:54 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 1 June 2015 at 15:30, Aurelio C. Remonda <aurelioremonda@gmail.com> wrote:
>
> Thanks for sending this patch. I have a few comments below.
>
>> I created an ARM_FEATURE_THUMB_DSP to be added to any non-M
>> thumb2-compatible CPU that uses DSP instructions.
>> There are 85 DSP instructions (all of them thumb2). On disas_thumb2_insn
>> the DSP feature is tested before the instruction is generated; if it's not
>> enabled then its an illegal op.
>
> Our general style for commit messages tends to be a bit more
> impersonal, so for instance "Create an ARM_FEATURE_THUMB_DSP"
> rather than "I created...".
>
>> Signed-off-by: Aurelio C. Remonda <aurelioremonda@gmail.com>
>
> As I noted in the other patch, the code that sets the feature bit
> for non-M-profile Thumb2 CPUs needs to go in this patch.
>
>> ---
>> target-arm/cpu.h | 1 +
>> target-arm/translate.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++--
>> 2 files changed, 107 insertions(+), 4 deletions(-)
>>
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index 21b5b8e..2e03d8e 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -890,6 +890,7 @@ enum arm_features {
>> ARM_FEATURE_V8_SHA1, /* implements SHA1 part of v8 Crypto Extensions */
>> ARM_FEATURE_V8_SHA256, /* implements SHA256 part of v8 Crypto Extensions */
>> ARM_FEATURE_V8_PMULL, /* implements PMULL part of v8 Crypto Extensions */
>> + ARM_FEATURE_THUMB_DSP, /* DSP insns supported in the Thumb encodings */
>> };
>>
>> static inline int arm_feature(CPUARMState *env, int feature)
>> diff --git a/target-arm/translate.c b/target-arm/translate.c
>> index 39692d7..2d14a2c 100644
>> --- a/target-arm/translate.c
>> +++ b/target-arm/translate.c
>> @@ -9444,6 +9444,10 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
>>
>> op = (insn >> 21) & 0xf;
>> if (op == 6) {
>> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
>> + /* pkhtb, pkfbt are DSP instructions */
>
> These comments aren't really necessary I think -- it's obvious from
> the feature bit we're testing and from the code below what's
> going on.
>
>> + goto illegal_op;
>> + }
>> /* Halfword pack. */
>> tmp = load_reg(s, rn);
>> tmp2 = load_reg(s, rm);
>> @@ -9518,13 +9522,35 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
>> switch (op) {
>> case 0: gen_sxth(tmp); break;
>> case 1: gen_uxth(tmp); break;
>> - case 2: gen_sxtb16(tmp); break;
>> - case 3: gen_uxtb16(tmp); break;
>> + case 2:
>> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
>> + /* sxtab16, sxtb16 are DSP instructions */
>> + tcg_temp_free_i32(tmp);
>> + goto illegal_op;
>> + }
>> + gen_sxtb16(tmp);
>> + break;
>> + case 3:
>> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
>> + /* uxtb16, uxtab16 are DSP instructions */
>> + tcg_temp_free_i32(tmp);
>> + goto illegal_op;
>> + }
>> + gen_uxtb16(tmp);
>> + break;
>> case 4: gen_sxtb(tmp); break;
>> case 5: gen_uxtb(tmp); break;
>> default: goto illegal_op;
>> }
>> if (rn != 15) {
>> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
>> + /* sxtab, sxtah, uxtab, uxtah are DSP instructions.
>> + * sxtb, sxth, uxtb, uxth are not DSP according to
>> + * ARMv7-M Architecture Reference Manual
>> + */
>> + tcg_temp_free_i32(tmp);
>> + goto illegal_op;
>> + }
>
> It looks like it would be fairly easy to hoist the illegal_op checks
> up above the load_reg() call, which then means we don't need to do
> a temp_free. You just need an extra switch on op like
>
> switch (op) {
> case 0: /* SXTAH, SXTH */
> case 1: /* UXTAH, UXTH */
> case 4: /* SXTAB, SXTB */
> case 5: /* UXTAB, UXTB */
> break;
> case 2: /* SXTAB16, SXTB16 */
> case 3: /* UXTAB16, UXTB16 */
> if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
> goto illegal_op;
> }
> break;
> default:
> goto illegal_op;
> }
> (The default: case in the original switch then becomes a
> g_assert_not_reached();)
>
> You can check for rn != 15 up here too.
>
>
>> tmp2 = load_reg(s, rn);
>> if ((op >> 1) == 1) {
>> gen_add16(tmp, tmp2);
>> @@ -9537,6 +9563,12 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
>> break;
>> case 2: /* SIMD add/subtract. */
>> op = (insn >> 20) & 7;
>> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
>> + /* add16, sub16, asx, sax, add8, sub8 (with q, s, sh, u, uh,
>> + * and uq variants) and usad8, usada8
>> + */
>
> If we want to document the instructions handled by this case then
> the comment belongs at the start of it, before the assignment to 'op'.
>
>> + goto illegal_op;
>> + }
>> shift = (insn >> 4) & 7;
>> if ((op & 3) == 3 || (shift & 3) == 3)
>> goto illegal_op;
>> @@ -9550,6 +9582,10 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
>> op = ((insn >> 17) & 0x38) | ((insn >> 4) & 7);
>> if (op < 4) {
>> /* Saturating add/subtract. */
>> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
>> + /* qsub, qadd, qdadd, qdsub are DSP instructions. */
>> + goto illegal_op;
>> + }
>> tmp = load_reg(s, rn);
>> tmp2 = load_reg(s, rm);
>> if (op & 1)
>> @@ -9575,6 +9611,11 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
>> gen_revsh(tmp);
>> break;
>> case 0x10: /* sel */
>> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
>> + /* sel is a DSP instruction. */
>> + tcg_temp_free_i32(tmp);
>> + goto illegal_op;
>> + }
>
> This check could also be hoisted up to above the allocation of tmp.
>
> Your indentation on this section seems to have gone wrong.
>
> If you run your patches through scripts/checkpatch.pl they will catch
> this kind of style error for you.
>
>> tmp2 = load_reg(s, rm);
>> tmp3 = tcg_temp_new_i32();
>> tcg_gen_ld_i32(tmp3, cpu_env, offsetof(CPUARMState, GE));
>> @@ -9640,6 +9681,14 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
>> }
>> break;
>> case 1: /* 16 x 16 -> 32 */
>> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
>> + /* smlabb, smlabt, smlatb, smlatt, smulbb, smulbt, smultt
>> + * and smultb are DSP instructions
>> + */
>> + tcg_temp_free_i32(tmp);
>> + tcg_temp_free_i32(tmp2);
>> + goto illegal_op;
>> + }
>> gen_mulxy(tmp, tmp2, op & 2, op & 1);
>> tcg_temp_free_i32(tmp2);
>> if (rs != 15) {
>> @@ -9650,6 +9699,12 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
>> break;
>> case 2: /* Dual multiply add. */
>> case 4: /* Dual multiply subtract. */
>> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
>> + /* smlad, smladx, smlsd, smusd are DSP instructions */
>> + tcg_temp_free_i32(tmp);
>> + tcg_temp_free_i32(tmp2);
>> + goto illegal_op;
>> + }
>> if (op)
>> gen_swap_half(tmp2);
>> gen_smul_dual(tmp, tmp2);
>> @@ -9672,6 +9727,12 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
>> }
>> break;
>> case 3: /* 32 * 16 -> 32msb */
>> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
>> + /* smlawb, smlawt, smulwt, smulwb are DSP instructions */
>> + tcg_temp_free_i32(tmp);
>> + tcg_temp_free_i32(tmp2);
>> + goto illegal_op;
>> + }
>> if (op)
>> tcg_gen_sari_i32(tmp2, tmp2, 16);
>> else
>> @@ -9689,6 +9750,14 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
>> }
>> break;
>> case 5: case 6: /* 32 * 32 -> 32msb (SMMUL, SMMLA, SMMLS) */
>> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
>> + /* smmla, smmls, smmul, smuad, smmlar,
>> + * smmlsr, smmulr are DSP instructions
>> + */
>> + tcg_temp_free_i32(tmp);
>> + tcg_temp_free_i32(tmp2);
>> + goto illegal_op;
>> + }
>> tmp64 = gen_muls_i64_i32(tmp, tmp2);
>> if (rs != 15) {
>> tmp = load_reg(s, rs);
>
> Again, all these checks inside this switch() would be more cleanly handled
> by having an initial switch() before we allocate the temporaries that deals
> with the illegal_op cases.
>
>> @@ -9735,6 +9804,12 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
>> store_reg(s, rd, tmp);
>> } else if ((op & 0xe) == 0xc) {
>> /* Dual multiply accumulate long. */
>> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
>> + /* smlald, smlsld are DSP instructions */
>> + tcg_temp_free_i32(tmp);
>> + tcg_temp_free_i32(tmp2);
>> + goto illegal_op;
>> + }
>> if (op & 1)
>> gen_swap_half(tmp2);
>> gen_smul_dual(tmp, tmp2);
>
>> @@ -9758,6 +9833,14 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
>> } else {
>> if (op & 8) {
>> /* smlalxy */
>> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
>> + /* smlalbb, smlalbt, smlaltb, smlaltt
>> + * are DSP instructions
>> + */
>> + tcg_temp_free_i32(tmp2);
>> + tcg_temp_free_i32(tmp);
>> + goto illegal_op;
>> + }
>> gen_mulxy(tmp, tmp2, op & 2, op & 1);
>> tcg_temp_free_i32(tmp2);
>> tmp64 = tcg_temp_new_i64();
>
> Doing the illegal_op checks early for these two would require
> more painful surgery to the code so I think they're OK like this.
>
>> @@ -9770,6 +9853,11 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
>> }
>> if (op & 4) {
>> /* umaal */
>> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
>> + /* ummal is a DSP instruction */
>> + tcg_temp_free_i64(tmp64);
>> + goto illegal_op;
>> + }
>> gen_addq_lo(s, tmp64, rs);
>> gen_addq_lo(s, tmp64, rd);
>> } else if (op & 0x40) {
>> @@ -10034,14 +10122,28 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
>> tmp2 = tcg_const_i32(imm);
>> if (op & 4) {
>> /* Unsigned. */
>> - if ((op & 1) && shift == 0)
>> + if ((op & 1) && shift == 0){
>
> You need a space before the '{' (again, checkpatch will tell you this).
>
>> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
>> + /* usat16 is a DSP instruction */
>> + tcg_temp_free_i32(tmp);
>> + tcg_temp_free_i32(tmp2);
>> + goto illegal_op;
>> + }
>> gen_helper_usat16(tmp, cpu_env, tmp, tmp2);
>> + }
>> else
>> gen_helper_usat(tmp, cpu_env, tmp, tmp2);
>
> ...and since you're touching the if() statement you need to also add
> braces on the else half of it. (Checkpatch again.)
>
>> } else {
>> /* Signed. */
>> - if ((op & 1) && shift == 0)
>> + if ((op & 1) && shift == 0){
>> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
>> + /* ssat16 is a DSP instruction */
>> + tcg_temp_free_i32(tmp);
>> + tcg_temp_free_i32(tmp2);
>> + goto illegal_op;
>> + }
>> gen_helper_ssat16(tmp, cpu_env, tmp, tmp2);
>> + }
>> else
Also note that the } is on same line as the else. End result will be:
} else {
Regards,
Peter
>> gen_helper_ssat(tmp, cpu_env, tmp, tmp2);
>> }
>
> Same comments apply for this if..else.
>
>> 1.9.1
>>
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-06-01 18:36 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-06-01 14:30 [Qemu-devel] [PATCH] Target-arm: Add the THUMB_DSP feature Aurelio C. Remonda
2015-06-01 17:54 ` Peter Maydell
2015-06-01 18:36 ` Peter Crosthwaite
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).