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

[Alex Bennée]

Sergey Fedorov <address@hidden> writes:

> On 29/03/16 16:26, Paolo Bonzini wrote:
>>
>> On 29/03/2016 15:19, Sergey Fedorov wrote:
>>> 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? :)
>> I don't really care. :)
>
> Alex, could you give your s-o-b for your variant of code? Or would you
> like to make a patch by yourself?

Sure here:

Signed-off-by: Alex Bennée <address@hidden>

>
> Kind regards,
> Sergey


--
Alex Bennée