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

[Sergey Fedorov]

On 22/03/16 17:59, Alex Bennée wrote:
> address@hidden writes:
>
>> From: Paolo Bonzini <address@hidden>
>>
>> Use a continue statement.
>>
>> Signed-off-by: Paolo Bonzini <address@hidden>
>> [Sergey Fedorov: Fix moving to list head in case of no TB]
>> Signed-off-by: Sergey Fedorov <address@hidden>
>> ---
>>  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.

Looks like another attempt to rewrite it. I am wondering whom to
attribute as an author, then? :)

Kind regards,
Sergey

>
>> +        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;
>>  }