diff mbox

[2/5] tcg: reorganize tb_find_physical loop

Message ID 1458222382-6498-3-git-send-email-sergey.fedorov@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

sergey.fedorov@linaro.org March 17, 2016, 1:46 p.m. UTC
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(-)

Comments

Peter Maydell March 17, 2016, 2:59 p.m. UTC | #1
On 17 March 2016 at 13:46,  <sergey.fedorov@linaro.org> wrote:
> 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>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

(though it would be nice if the commit message gave some
motivation for the change)

thanks
-- PMM
Alex Bennée March 22, 2016, 2:59 p.m. UTC | #2
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
Paolo Bonzini March 22, 2016, 3 p.m. UTC | #3
On 22/03/2016 15:59, Alex Bennée wrote:
>> > +    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:

Sure, that would be just fine.

Paolo
Sergey Fedorov March 29, 2016, 1:19 p.m. UTC | #4
On 22/03/16 17:59, Alex Bennée wrote:
> 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.

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;
>>  }
Paolo Bonzini March 29, 2016, 1:26 p.m. UTC | #5
On 29/03/2016 15:19, Sergey Fedorov wrote:
> On 22/03/16 17:59, Alex Bennée wrote:
>> 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.
> 
> Looks like another attempt to rewrite it. I am wondering whom to
> attribute as an author, then? :)

I don't really care. :)

Paolo
Sergey Fedorov March 29, 2016, 2:05 p.m. UTC | #6
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:
>>> 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.
>> 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?

Kind regards,
Sergey
Alex Bennée March 29, 2016, 2:26 p.m. UTC | #7
Sergey Fedorov <serge.fdrv@gmail.com> 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:
>>>> 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.
>>> 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 <alex.bennee@linaro.org>

>
> Kind regards,
> Sergey


--
Alex Bennée
Sergey Fedorov March 29, 2016, 2:37 p.m. UTC | #8
On 29/03/16 17:26, Alex Bennée wrote:
>> 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 <alex.bennee@linaro.org>
>

So I'm going to respin the series quickly dropping the last two
contentious patches so far. It would give this series a chance for to
get in 2.6.

Kind regards,
Sergey
diff mbox

Patch

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