Re: [PATCH for-6.1 2/2] accel/tcg: Remove unnecessary check on icount_extra in cpu_loop_exec_tb()

[Peter Maydell]

On Sun, 25 Jul 2021 at 18:44, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> In cpu_loop_exec_tb(), we decide whether to look for a TB with
> exactly insns_left instructions in it using the condition
>  (!cpu->icount_extra && insns_left > 0 && insns_left < tb->icount)
>
> The check for icount_extra == 0 is unnecessary, because we just set
>   insns_left = MIN(0xffff, cpu->icount_budget);
>   icount_extra = icount_budget - insns_left;
> and so icount_extra can only be non-zero if icount_budget > 0xffff
> and insns_left == 0xffff. But in that case insns_left >= tb->icount
> because 0xffff is much larger than TCG_MAX_INSNS, so the condition
> will be false anyway.
>
> Remove the unnecessary check, and instead assert:
>  * that we are only going to execute a partial TB here if the
>    icount budget has run out (ie icount_extra == 0)
>  * that the number of insns we're going to execute does fit into
>    the CF_COUNT_MASK
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> You could argue that we don't need the asserts, if you like.
> The first one would have caught the bug fixed in the previous
> commit, though.

"first" in the bulleted list, "second" in the order I put them in the code...

> ---
>  accel/tcg/cpu-exec.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index 6e8dc291197..5aa42fbff35 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -843,7 +843,9 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, 
> TranslationBlock *tb,
>       * execute we need to ensure we find/generate a TB with exactly
>       * insns_left instructions in it.
>       */
> -    if (!cpu->icount_extra && insns_left > 0 && insns_left < tb->icount)  {
> +    if (insns_left > 0 && insns_left < tb->icount)  {
> +        assert(insns_left <= CF_COUNT_MASK);
> +        assert(cpu->icount_extra == 0);
>          cpu->cflags_next_tb = (tb->cflags & ~CF_COUNT_MASK) | insns_left;
>      }
>  #endif

-- PMM