[v2] accel/tcg: Introduce translator_use_goto_tb

[PATCH v2 18/28] target/openrisc: Use translator_use_goto_tb

Richard Henderson posted 28 patches 4 years, 7 months ago

Diff against v1 v3
Download mbox

Maintainers: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>, David Hildenbrand <david@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Greg Kurz <groug@kaod.org>, Paolo Bonzini <pbonzini@redhat.com>, Laurent Vivier <laurent@vivier.eu>, Marek Vasut <marex@denx.de>, Bastian Koppelmann <kbastian@mail.uni-paderborn.de>, Eduardo Habkost <ehabkost@redhat.com>, "Philippe Mathieu-Daudé" <f4bug@amsat.org>, Thomas Huth <thuth@redhat.com>, Palmer Dabbelt <palmer@dabbelt.com>, David Gibson <david@gibson.dropbear.id.au>, Yoshinori Sato <ysato@users.sourceforge.jp>, Michael Rolnik <mrolnik@gmail.com>, Aurelien Jarno <aurelien@aurel32.net>, Chris Wulff <crwulff@gmail.com>, Artyom Tarasenko <atar4qemu@gmail.com>, Max Filippov <jcmvbkbc@gmail.com>, Peter Maydell <peter.maydell@linaro.org>, Cornelia Huck <cohuck@redhat.com>, Jiaxun Yang <jiaxun.yang@flygoat.com>, Stafford Horne <shorne@gmail.com>, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>, Alistair Francis <alistair.francis@wdc.com>, Aleksandar Rikalo <aleksandar.rikalo@syrmia.com>, Bin Meng <bin.meng@windriver.com>

There is a newer version of this series

Expand all Fold all

[PATCH v2 18/28] target/openrisc: Use translator_use_goto_tb

Posted by Richard Henderson 4 years, 7 months ago

Reorder the cases in openrisc_tr_tb_stop to make this easier to read.

Cc: Stafford Horne <shorne@gmail.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/openrisc/translate.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/target/openrisc/translate.c b/target/openrisc/translate.c
index a9c81f8bd5..2d142d8577 100644
--- a/target/openrisc/translate.c
+++ b/target/openrisc/translate.c
@@ -1720,16 +1720,17 @@ static void openrisc_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
         /* fallthru */
 
     case DISAS_TOO_MANY:
-        if (unlikely(dc->base.singlestep_enabled)) {
-            tcg_gen_movi_tl(cpu_pc, jmp_dest);
-            gen_exception(dc, EXCP_DEBUG);
-        } else if ((dc->base.pc_first ^ jmp_dest) & TARGET_PAGE_MASK) {
-            tcg_gen_movi_tl(cpu_pc, jmp_dest);
-            tcg_gen_lookup_and_goto_ptr();
-        } else {
+        if (translator_use_goto_tb(&dc->base, jmp_dest)) {
             tcg_gen_goto_tb(0);
             tcg_gen_movi_tl(cpu_pc, jmp_dest);
             tcg_gen_exit_tb(dc->base.tb, 0);
+            break;
+        }
+        tcg_gen_movi_tl(cpu_pc, jmp_dest);
+        if (unlikely(dc->base.singlestep_enabled)) {
+            gen_exception(dc, EXCP_DEBUG);
+        } else {
+            tcg_gen_lookup_and_goto_ptr();
         }
         break;
 
-- 
2.25.1

Re: [PATCH v2 18/28] target/openrisc: Use translator_use_goto_tb

Posted by Stafford Horne 4 years, 7 months ago

On Wed, Jun 30, 2021 at 11:32:16AM -0700, Richard Henderson wrote:
> Reorder the cases in openrisc_tr_tb_stop to make this easier to read.

Hi Richard,

For me the description is a bit misleading.  I don't see it as a simple
reorder for making things easier to read, because we need to understand
what is inside of translator_use_goto_tb now.

From the patch basically translator_use_goto_tb indicates that a jump is in
the same page and we are not single stepping.

The old code was:

  If single step
   DEBUG
  else if not same page
   tcg_gen_lookup_and_goto_ptr()
  else *same page*
   jump same page

Now:

  If translator_use_goto_tb() (same page & not single step)
    jump same page

  If single step
    DEBUG
  else *not same page*
    tcg_gen_lookup_and_goto_ptr()

It would be a bit easier to understand if the description was something like,
Reorder the control statements to allow using the page boundary check from
translator_use_goto_tb().

That said it looks correct so:

Reviewed-by: Stafford Horne <shorne@gmail.com>


> Cc: Stafford Horne <shorne@gmail.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/openrisc/translate.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/target/openrisc/translate.c b/target/openrisc/translate.c
> index a9c81f8bd5..2d142d8577 100644
> --- a/target/openrisc/translate.c
> +++ b/target/openrisc/translate.c
> @@ -1720,16 +1720,17 @@ static void openrisc_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
>          /* fallthru */
>  
>      case DISAS_TOO_MANY:
> -        if (unlikely(dc->base.singlestep_enabled)) {
> -            tcg_gen_movi_tl(cpu_pc, jmp_dest);
> -            gen_exception(dc, EXCP_DEBUG);
> -        } else if ((dc->base.pc_first ^ jmp_dest) & TARGET_PAGE_MASK) {
> -            tcg_gen_movi_tl(cpu_pc, jmp_dest);
> -            tcg_gen_lookup_and_goto_ptr();
> -        } else {
> +        if (translator_use_goto_tb(&dc->base, jmp_dest)) {
>              tcg_gen_goto_tb(0);
>              tcg_gen_movi_tl(cpu_pc, jmp_dest);
>              tcg_gen_exit_tb(dc->base.tb, 0);
> +            break;
> +        }
> +        tcg_gen_movi_tl(cpu_pc, jmp_dest);
> +        if (unlikely(dc->base.singlestep_enabled)) {
> +            gen_exception(dc, EXCP_DEBUG);
> +        } else {
> +            tcg_gen_lookup_and_goto_ptr();
>          }
>          break;
>  
> -- 
> 2.25.1
>