From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45504) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aiNmJ-00081F-I7 for qemu-devel@nongnu.org; Tue, 22 Mar 2016 10:59:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aiNmF-0002HK-VM for qemu-devel@nongnu.org; Tue, 22 Mar 2016 10:59:27 -0400 Received: from mail-wm0-x22e.google.com ([2a00:1450:400c:c09::22e]:34797) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aiNmF-0002H8-Ko for qemu-devel@nongnu.org; Tue, 22 Mar 2016 10:59:23 -0400 Received: by mail-wm0-x22e.google.com with SMTP id p65so196490192wmp.1 for ; Tue, 22 Mar 2016 07:59:23 -0700 (PDT) References: <1458222382-6498-1-git-send-email-sergey.fedorov@linaro.org> <1458222382-6498-3-git-send-email-sergey.fedorov@linaro.org> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: <1458222382-6498-3-git-send-email-sergey.fedorov@linaro.org> Date: Tue, 22 Mar 2016 14:59:20 +0000 Message-ID: <87d1qmsgmv.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH 2/5] tcg: reorganize tb_find_physical loop List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: sergey.fedorov@linaro.org Cc: Sergey Fedorov , Peter Crosthwaite , qemu-devel@nongnu.org, Paolo Bonzini , Richard Henderson sergey.fedorov@linaro.org writes: > From: Paolo Bonzini > > Use a continue statement. > > Signed-off-by: Paolo Bonzini > [Sergey Fedorov: Fix moving to list head in case of no TB] > Signed-off-by: Sergey Fedorov > --- > 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