| Message ID | 20190726175032.6769-11-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: Convert aa32 base isa to decodetree | expand |
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
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~
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
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) {
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