On Mon, Jun 1, 2015 at 10:54 AM, Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 1 June 2015 at 15:30, Aurelio C. Remonda <aurelioremo...@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 <aurelioremo...@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
>
