| Message ID | 20210621013439.1791385-2-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | accel/tcg: Introduce translator_use_goto_tb | expand |
On Sun, Jun 20, 2021 at 6:36 PM Richard Henderson <richard.henderson@linaro.org> wrote: > > Add a generic version of the common use_goto_tb test. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > include/exec/translator.h | 10 ++++++++++ > accel/tcg/translator.c | 11 +++++++++++ > 2 files changed, 21 insertions(+) Reviewed-by: Max Filippov <jcmvbkbc@gmail.com> -- Thanks. -- Max
Hi Richard, On 6/21/21 3:34 AM, Richard Henderson wrote: > Add a generic version of the common use_goto_tb test. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > include/exec/translator.h | 10 ++++++++++ > accel/tcg/translator.c | 11 +++++++++++ > 2 files changed, 21 insertions(+) > > diff --git a/include/exec/translator.h b/include/exec/translator.h > index 24232ead41..dd9c06d40d 100644 > --- a/include/exec/translator.h > +++ b/include/exec/translator.h > @@ -145,6 +145,16 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db, > > void translator_loop_temp_check(DisasContextBase *db); > > +/** > + * translator_use_goto_tb > + * @db: Disassembly context > + * @dest: target pc of the goto > + * > + * Return true if goto_tb is allowed between the current TB > + * and the destination PC. > + */ > +bool translator_use_goto_tb(DisasContextBase *db, target_ulong dest); > + > /* > * Translator Load Functions > * > diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c > index 1d32732198..59804af37b 100644 > --- a/accel/tcg/translator.c > +++ b/accel/tcg/translator.c > @@ -31,6 +31,17 @@ void translator_loop_temp_check(DisasContextBase *db) > } > } > > +bool translator_use_goto_tb(DisasContextBase *db, target_ulong dest) > +{ > + /* Suppress goto_tb in the case of single-steping. */ > + if (db->singlestep_enabled || singlestep) { > + return false; > + } > + I notice various targets do: #ifdef CONFIG_USER_ONLY return true; #else > + /* Check for the dest on the same page as the start of the TB. */ > + return ((db->pc_first ^ dest) & TARGET_PAGE_MASK) == 0; #endif > +} Is that OK to remove this case? If so, it might be worth a comment somewhere. Regards, Phil.
From: Richard Henderson <richard.henderson@linaro.org> > Add a generic version of the common use_goto_tb test. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > include/exec/translator.h | 10 ++++++++++ > accel/tcg/translator.c | 11 +++++++++++ > 2 files changed, 21 insertions(+) Reviewed-by: Luis Pires <luis.pires@eldorado.org.br> -- Luis Pires Instituto de Pesquisas ELDORADO Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>
On 6/21/21 5:50 AM, Philippe Mathieu-Daudé wrote: > I notice various targets do: > > #ifdef CONFIG_USER_ONLY > return true; > #else > >> + /* Check for the dest on the same page as the start of the TB. */ >> + return ((db->pc_first ^ dest) & TARGET_PAGE_MASK) == 0; > #endif > >> +} > Is that OK to remove this case? If so, it might be worth a comment > somewhere. I mentioned it in the cover letter. r~
On 6/21/21 3:47 PM, Richard Henderson wrote: > On 6/21/21 5:50 AM, Philippe Mathieu-Daudé wrote: >> I notice various targets do: >> >> #ifdef CONFIG_USER_ONLY >> return true; >> #else >> >>> + /* Check for the dest on the same page as the start of the TB. */ >>> + return ((db->pc_first ^ dest) & TARGET_PAGE_MASK) == 0; >> #endif >> >>> +} >> Is that OK to remove this case? If so, it might be worth a comment >> somewhere. > > I mentioned it in the cover letter. But the commit letter will vanish, so preferably mentioning the change in the commit description (copying the cover letter comment could work too): Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
diff --git a/include/exec/translator.h b/include/exec/translator.h index 24232ead41..dd9c06d40d 100644 --- a/include/exec/translator.h +++ b/include/exec/translator.h @@ -145,6 +145,16 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db, void translator_loop_temp_check(DisasContextBase *db); +/** + * translator_use_goto_tb + * @db: Disassembly context + * @dest: target pc of the goto + * + * Return true if goto_tb is allowed between the current TB + * and the destination PC. + */ +bool translator_use_goto_tb(DisasContextBase *db, target_ulong dest); + /* * Translator Load Functions * diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c index 1d32732198..59804af37b 100644 --- a/accel/tcg/translator.c +++ b/accel/tcg/translator.c @@ -31,6 +31,17 @@ void translator_loop_temp_check(DisasContextBase *db) } } +bool translator_use_goto_tb(DisasContextBase *db, target_ulong dest) +{ + /* Suppress goto_tb in the case of single-steping. */ + if (db->singlestep_enabled || singlestep) { + return false; + } + + /* Check for the dest on the same page as the start of the TB. */ + return ((db->pc_first ^ dest) & TARGET_PAGE_MASK) == 0; +} + void translator_loop(const TranslatorOps *ops, DisasContextBase *db, CPUState *cpu, TranslationBlock *tb, int max_insns) {
Add a generic version of the common use_goto_tb test. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- include/exec/translator.h | 10 ++++++++++ accel/tcg/translator.c | 11 +++++++++++ 2 files changed, 21 insertions(+) -- 2.25.1