On 29/03/2016 15:19, Sergey Fedorov wrote:
> 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? :)
I don't really care. :)
Paolo
