Re: [Qemu-devel] [PATCH 2/5] tcg: reorganize tb_find_physical loop

["Alex Bennée" <alex.bennee@linaro.org>]

sergey.fedorov@linaro.org writes:

> From: Paolo Bonzini <pbonzini@redhat.com>
>
> Use a continue statement.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> [Sergey Fedorov: Fix moving to list head in case of no TB]
> Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
> ---
>  cpu-exec.c | 50 +++++++++++++++++++++++++-------------------------
>  1 file changed, 25 insertions(+), 25 deletions(-)
>
> diff --git a/cpu-exec.c b/cpu-exec.c
> index fd92452f16f6..f90482eff778 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -225,37 +225,37 @@ static TranslationBlock *tb_find_physical(CPUState *cpu,
>      phys_pc = get_page_addr_code(env, pc);
>      phys_page1 = phys_pc & TARGET_PAGE_MASK;
>      h = tb_phys_hash_func(phys_pc);
> -    ptb1 = &tcg_ctx.tb_ctx.tb_phys_hash[h];
> -    for(;;) {
> -        tb = *ptb1;
> -        if (!tb) {
> -            return NULL;
> +    for (ptb1 = &tcg_ctx.tb_ctx.tb_phys_hash[h];
> +         (tb = *ptb1) != NULL;
> +         ptb1 = &tb->phys_hash_next) {

I'm not sure I'm keen on the assignment in the for condition clause. I
appreciate the cleansing of the if !tb return exit though. Could we be
cleaner maybe? Here is my attempt:

    static TranslationBlock *tb_find_physical(CPUState *cpu,
                                              target_ulong pc,
                                              target_ulong cs_base,
                                              uint64_t flags)
    {
        CPUArchState *env = (CPUArchState *)cpu->env_ptr;
        TranslationBlock *tb, **tb_hash_head, **ptb1;
        unsigned int h;
        tb_page_addr_t phys_pc, phys_page1;

        /* find translated block using physical mappings */
        phys_pc = get_page_addr_code(env, pc);
        phys_page1 = phys_pc & TARGET_PAGE_MASK;
        h = tb_phys_hash_func(phys_pc);

        /* Start at head of the hash entry */
        ptb1 = tb_hash_head = &tcg_ctx.tb_ctx.tb_phys_hash[h];
        tb = *ptb1;

        while (tb) {

            if (tb->pc == pc &&
                tb->page_addr[0] == phys_page1 &&
                tb->cs_base == cs_base &&
                tb->flags == flags) {

                if (tb->page_addr[1] == -1) {
                    /* done, we have a match */
                    break;
                } else {
                    /* check next page if needed */
                    target_ulong virt_page2 = (pc & TARGET_PAGE_MASK)
                        + TARGET_PAGE_SIZE;
                    tb_page_addr_t phys_page2 = get_page_addr_code(env, virt_page2);

                    if (tb->page_addr[1] == phys_page2) {
                        break;
                    }
                }
            }

            ptb1 = &tb->phys_hash_next;
            tb = *ptb1;
        }

        if (tb) {
            /* Move the TB to the head of the list */
            *ptb1 = tb->phys_hash_next;
            tb->phys_hash_next = *tb_hash_head;
            *tb_hash_head = tb;
        }
        return tb;
    }

FWIW the compiled code is 9 bytes shorter on my machine.

> +        if (tb->pc != pc ||
> +            tb->page_addr[0] != phys_page1 ||
> +            tb->cs_base != cs_base ||
> +            tb->flags != flags) {
> +            continue;
>          }
> -        if (tb->pc == pc &&
> -            tb->page_addr[0] == phys_page1 &&
> -            tb->cs_base == cs_base &&
> -            tb->flags == flags) {
> -            /* check next page if needed */
> -            if (tb->page_addr[1] != -1) {
> -                tb_page_addr_t phys_page2;
> -
> -                virt_page2 = (pc & TARGET_PAGE_MASK) +
> -                    TARGET_PAGE_SIZE;
> -                phys_page2 = get_page_addr_code(env, virt_page2);
> -                if (tb->page_addr[1] == phys_page2) {
> -                    break;
> -                }
> -            } else {
> +
> +        /* check next page if needed */
> +        if (tb->page_addr[1] != -1) {
> +            tb_page_addr_t phys_page2;
> +
> +            virt_page2 = (pc & TARGET_PAGE_MASK) +
> +                TARGET_PAGE_SIZE;
> +            phys_page2 = get_page_addr_code(env, virt_page2);
> +            if (tb->page_addr[1] == phys_page2) {
>                  break;
>              }
> +        } else {
> +            break;
>          }
> -        ptb1 = &tb->phys_hash_next;
>      }
>
> -    /* Move the TB to the head of the list */
> -    *ptb1 = tb->phys_hash_next;
> -    tb->phys_hash_next = tcg_ctx.tb_ctx.tb_phys_hash[h];
> -    tcg_ctx.tb_ctx.tb_phys_hash[h] = tb;
> +    if (tb) {
> +        /* Move the TB to the head of the list */
> +        *ptb1 = tb->phys_hash_next;
> +        tb->phys_hash_next = tcg_ctx.tb_ctx.tb_phys_hash[h];
> +        tcg_ctx.tb_ctx.tb_phys_hash[h] = tb;
> +    }
>      return tb;
>  }


--
Alex Bennée