Re: [Qemu-devel] [PATCH 10/67] target/arm: Move test for AL into arm_skip_unless
On Tue, 30 Jul 2019 at 01:57, Richard Henderson wrote: > > On 7/29/19 7:32 AM, Peter Maydell wrote: > > On Fri, 26 Jul 2019 at 18:50, Richard Henderson > > wrote: > >> > >> We will shortly be calling this function much more often. > >> > >> Signed-off-by: Richard Henderson > >> --- > > > > In the other callsites for arm_skip_unless() the cond argument > > can never be 0xe or 0xf. > > > > Reviewed-by: Peter Maydell > > 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
Re: [Qemu-devel] [PATCH 10/67] target/arm: Move test for AL into arm_skip_unless
On 7/29/19 7:32 AM, Peter Maydell wrote:
> On Fri, 26 Jul 2019 at 18:50, Richard Henderson
> wrote:
>>
>> We will shortly be calling this function much more often.
>>
>> Signed-off-by: Richard Henderson
>> ---
>> 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 & 0x0f90) == 0x0300) {
>> 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
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~
Re: [Qemu-devel] [PATCH 10/67] target/arm: Move test for AL into arm_skip_unless
On Fri, 26 Jul 2019 at 18:50, Richard Henderson
wrote:
>
> We will shortly be calling this function much more often.
>
> Signed-off-by: Richard Henderson
> ---
> 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 & 0x0f90) == 0x0300) {
> 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
thanks
-- PMM
[Qemu-devel] [PATCH 10/67] target/arm: Move test for AL into arm_skip_unless
We will shortly be calling this function much more often.
Signed-off-by: Richard Henderson
---
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 & 0x0f90) == 0x0300) {
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) {
--
2.17.1
