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