[10/67] target/arm: Move test for AL into arm_skip_unless

Message ID 20190726175032.6769-11-richard.henderson@linaro.org
State New
Headers show
Series
  • target/arm: Convert aa32 base isa to decodetree
Related show

Commit Message

Richard Henderson July 26, 2019, 5:49 p.m.
We will shortly be calling this function much more often.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

---
 target/arm/translate.c | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

-- 
2.17.1

Comments

Peter Maydell July 29, 2019, 2:32 p.m. | #1
On Fri, 26 Jul 2019 at 18:50, Richard Henderson
<richard.henderson@linaro.org> wrote:
>

> We will shortly be calling this function much more often.

>

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

> ---

>  target/arm/translate.c | 28 ++++++++++++----------------

>  1 file changed, 12 insertions(+), 16 deletions(-)

>

> diff --git a/target/arm/translate.c b/target/arm/translate.c

> index 53c46fcdc4..36419025db 100644

> --- a/target/arm/translate.c

> +++ b/target/arm/translate.c

> @@ -7705,8 +7705,14 @@ static void arm_gen_condlabel(DisasContext *s)

>  /* Skip this instruction if the ARM condition is false */

>  static void arm_skip_unless(DisasContext *s, uint32_t cond)

>  {

> -    arm_gen_condlabel(s);

> -    arm_gen_test_cc(cond ^ 1, s->condlabel);

> +    /*

> +     * Conditionally skip the insn. Note that both 0xe and 0xf mean

> +     * "always"; 0xf is not "never".

> +     */

> +    if (cond < 0xe) {

> +        arm_gen_condlabel(s);

> +        arm_gen_test_cc(cond ^ 1, s->condlabel);

> +    }

>  }

>

>  static void disas_arm_insn(DisasContext *s, unsigned int insn)

> @@ -7944,11 +7950,9 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)

>          }

>          goto illegal_op;

>      }

> -    if (cond != 0xe) {

> -        /* if not always execute, we generate a conditional jump to

> -           next instruction */

> -        arm_skip_unless(s, cond);

> -    }

> +

> +    arm_skip_unless(s, cond);

> +

>      if ((insn & 0x0f900000) == 0x03000000) {

>          if ((insn & (1 << 21)) == 0) {

>              ARCH(6T2);

> @@ -12095,15 +12099,7 @@ static void thumb_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)

>      dc->insn = insn;

>

>      if (dc->condexec_mask && !thumb_insn_is_unconditional(dc, insn)) {

> -        uint32_t cond = dc->condexec_cond;

> -

> -        /*

> -         * Conditionally skip the insn. Note that both 0xe and 0xf mean

> -         * "always"; 0xf is not "never".

> -         */

> -        if (cond < 0x0e) {

> -            arm_skip_unless(dc, cond);

> -        }

> +        arm_skip_unless(dc, dc->condexec_cond);

>      }


In the other callsites for arm_skip_unless() the cond argument
can never be 0xe or 0xf.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>


thanks
-- PMM
Richard Henderson July 30, 2019, 12:57 a.m. | #2
On 7/29/19 7:32 AM, Peter Maydell wrote:
> On Fri, 26 Jul 2019 at 18:50, Richard Henderson

> <richard.henderson@linaro.org> wrote:

>>

>> We will shortly be calling this function much more often.

>>

>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

>> ---

>>  target/arm/translate.c | 28 ++++++++++++----------------

>>  1 file changed, 12 insertions(+), 16 deletions(-)

>>

>> diff --git a/target/arm/translate.c b/target/arm/translate.c

>> index 53c46fcdc4..36419025db 100644

>> --- a/target/arm/translate.c

>> +++ b/target/arm/translate.c

>> @@ -7705,8 +7705,14 @@ static void arm_gen_condlabel(DisasContext *s)

>>  /* Skip this instruction if the ARM condition is false */

>>  static void arm_skip_unless(DisasContext *s, uint32_t cond)

>>  {

>> -    arm_gen_condlabel(s);

>> -    arm_gen_test_cc(cond ^ 1, s->condlabel);

>> +    /*

>> +     * Conditionally skip the insn. Note that both 0xe and 0xf mean

>> +     * "always"; 0xf is not "never".

>> +     */

>> +    if (cond < 0xe) {

>> +        arm_gen_condlabel(s);

>> +        arm_gen_test_cc(cond ^ 1, s->condlabel);

>> +    }

>>  }

>>

>>  static void disas_arm_insn(DisasContext *s, unsigned int insn)

>> @@ -7944,11 +7950,9 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)

>>          }

>>          goto illegal_op;

>>      }

>> -    if (cond != 0xe) {

>> -        /* if not always execute, we generate a conditional jump to

>> -           next instruction */

>> -        arm_skip_unless(s, cond);

>> -    }

>> +

>> +    arm_skip_unless(s, cond);

>> +

>>      if ((insn & 0x0f900000) == 0x03000000) {

>>          if ((insn & (1 << 21)) == 0) {

>>              ARCH(6T2);

>> @@ -12095,15 +12099,7 @@ static void thumb_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)

>>      dc->insn = insn;

>>

>>      if (dc->condexec_mask && !thumb_insn_is_unconditional(dc, insn)) {

>> -        uint32_t cond = dc->condexec_cond;

>> -

>> -        /*

>> -         * Conditionally skip the insn. Note that both 0xe and 0xf mean

>> -         * "always"; 0xf is not "never".

>> -         */

>> -        if (cond < 0x0e) {

>> -            arm_skip_unless(dc, cond);

>> -        }

>> +        arm_skip_unless(dc, dc->condexec_cond);

>>      }

> 

> In the other callsites for arm_skip_unless() the cond argument

> can never be 0xe or 0xf.

> 

> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>


In my original version I included cond in the fields collected by decodetree,
and so every single trans_* function called arm_skip_unless, so that would not
be the case there.

I discarded that in the version posted here, but I still think it might be a
cleaner design overall.

In the short term, maybe I should just discard this patch?


r~
Peter Maydell July 30, 2019, 8:49 a.m. | #3
On Tue, 30 Jul 2019 at 01:57, Richard Henderson
<richard.henderson@linaro.org> wrote:
>

> On 7/29/19 7:32 AM, Peter Maydell wrote:

> > On Fri, 26 Jul 2019 at 18:50, Richard Henderson

> > <richard.henderson@linaro.org> wrote:

> >>

> >> We will shortly be calling this function much more often.

> >>

> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

> >> ---


> >

> > In the other callsites for arm_skip_unless() the cond argument

> > can never be 0xe or 0xf.

> >

> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

>

> In my original version I included cond in the fields collected by decodetree,

> and so every single trans_* function called arm_skip_unless, so that would not

> be the case there.


That remark was more a note about why the change is ok and doesn't
change behaviour for the other callsites that the patch doesn't touch.
(It's the kind of thing it's helpful to note in a commit message to
show that you've thought about it.)

> I discarded that in the version posted here, but I still think it might be a

> cleaner design overall.

>

> In the short term, maybe I should just discard this patch?


I don't have a strong opinion either way. Putting the cond check
inside the function seems cleaner even if we're only calling it in
a few places, I think.

thanks
-- PMM

Patch

diff --git a/target/arm/translate.c b/target/arm/translate.c
index 53c46fcdc4..36419025db 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -7705,8 +7705,14 @@  static void arm_gen_condlabel(DisasContext *s)
 /* Skip this instruction if the ARM condition is false */
 static void arm_skip_unless(DisasContext *s, uint32_t cond)
 {
-    arm_gen_condlabel(s);
-    arm_gen_test_cc(cond ^ 1, s->condlabel);
+    /*
+     * Conditionally skip the insn. Note that both 0xe and 0xf mean
+     * "always"; 0xf is not "never".
+     */
+    if (cond < 0xe) {
+        arm_gen_condlabel(s);
+        arm_gen_test_cc(cond ^ 1, s->condlabel);
+    }
 }
 
 static void disas_arm_insn(DisasContext *s, unsigned int insn)
@@ -7944,11 +7950,9 @@  static void disas_arm_insn(DisasContext *s, unsigned int insn)
         }
         goto illegal_op;
     }
-    if (cond != 0xe) {
-        /* if not always execute, we generate a conditional jump to
-           next instruction */
-        arm_skip_unless(s, cond);
-    }
+
+    arm_skip_unless(s, cond);
+
     if ((insn & 0x0f900000) == 0x03000000) {
         if ((insn & (1 << 21)) == 0) {
             ARCH(6T2);
@@ -12095,15 +12099,7 @@  static void thumb_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
     dc->insn = insn;
 
     if (dc->condexec_mask && !thumb_insn_is_unconditional(dc, insn)) {
-        uint32_t cond = dc->condexec_cond;
-
-        /*
-         * Conditionally skip the insn. Note that both 0xe and 0xf mean
-         * "always"; 0xf is not "never".
-         */
-        if (cond < 0x0e) {
-            arm_skip_unless(dc, cond);
-        }
+        arm_skip_unless(dc, dc->condexec_cond);
     }
 
     if (is_16bit) {