Re: [Qemu-devel] [PATCH v2 02/27] target/sh4: Consolidate end-of-TB tests

[Aurelien Jarno]

On 2017-07-06 16:20, Richard Henderson wrote:
> We can fold 3 different tests within the decode loop
> into a more accurate computation of max_insns to start.
> 
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  target/sh4/translate.c | 29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/target/sh4/translate.c b/target/sh4/translate.c
> index 82d4d69..663b5c0 100644
> --- a/target/sh4/translate.c
> +++ b/target/sh4/translate.c
> @@ -1848,7 +1848,6 @@ void gen_intermediate_code(CPUSH4State * env, struct 
> TranslationBlock *tb)
>      ctx.features = env->features;
>      ctx.has_movcal = (ctx.tbflags & TB_FLAG_PENDING_MOVCA);
>  
> -    num_insns = 0;
>      max_insns = tb->cflags & CF_COUNT_MASK;
>      if (max_insns == 0) {
>          max_insns = CF_COUNT_MASK;
> @@ -1856,9 +1855,23 @@ void gen_intermediate_code(CPUSH4State * env, struct 
> TranslationBlock *tb)
>      if (max_insns > TCG_MAX_INSNS) {
>          max_insns = TCG_MAX_INSNS;
>      }
> +    /* Since the ISA is fixed-width, we can bound by the number
> +       of instructions remaining on the page.  */
> +    num_insns = (TARGET_PAGE_SIZE - (ctx.pc & (TARGET_PAGE_SIZE - 1))) / 2;

This could be written as num_insn = -(ctx.pc | TARGET_PAGE_MASK) / 2;

> +    if (max_insns > num_insns) {
> +        max_insns = num_insns;
> +    }

You are following the existing pattern, so I can really blame you, but
maybe it's the moment to change the existing code into something like:

    max_insns = MIN(max_insn, ...)


> +    /* Single stepping means just that.  */
> +    if (ctx.singlestep_enabled || singlestep) {
> +        max_insns = 1;
> +    }
>  
>      gen_tb_start(tb);
> -    while (ctx.bstate == BS_NONE && !tcg_op_buf_full()) {
> +    num_insns = 0;
> +
> +    while (ctx.bstate == BS_NONE
> +           && num_insns < max_insns
> +           && !tcg_op_buf_full()) {
>          tcg_gen_insn_start(ctx.pc, ctx.envflags);
>          num_insns++;
>  
> @@ -1882,18 +1895,10 @@ void gen_intermediate_code(CPUSH4State * env, struct 
> TranslationBlock *tb)
>          ctx.opcode = cpu_lduw_code(env, ctx.pc);
>       decode_opc(&ctx);
>       ctx.pc += 2;
> -     if ((ctx.pc & (TARGET_PAGE_SIZE - 1)) == 0)
> -         break;
> -        if (cs->singlestep_enabled) {
> -         break;
> -        }
> -        if (num_insns >= max_insns)
> -            break;
> -        if (singlestep)
> -            break;
>      }
> -    if (tb->cflags & CF_LAST_IO)
> +    if (tb->cflags & CF_LAST_IO) {
>          gen_io_end();
> +    }
>      if (cs->singlestep_enabled) {
>          gen_save_cpu_state(&ctx, true);
>          gen_helper_debug(cpu_env);

Besides the minor nitpicks above:

Reviewed-by: Aurelien Jarno <address@hidden>

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
address@hidden                 http://www.aurel32.net