Re: [PATCH v2 0/8] target/sparc: Use tcg_gen_lookup_and_goto_ptr

[Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>]

On 22/06/2023 13:26, Mark Cave-Ayland wrote:

> On 21/06/2023 19:05, Richard Henderson wrote:
> 
>> Changes from v1:
>>    * Split into teeny weeny pieces.
>>
>>    * It turns out the sparc_tr_tb_stop hunk of v1 was buggy,
>>      in that things that are not simple branches use DYNAMIC_PC,
>>      e.g. the RETT (return from trap) instruction.
>>
>>      Introduce DYNAMIC_PC_LOOKUP to distinguish the couple of
>>      places where we have a dynamic pc, but no other change
>>      of state (conditional branches, JMPL, RETURN).
>>
>>    * Drop the change for WRFPRS, because it's too infrequent.
>>      The WRASI change affects memcpy/memset, so that's more important.
>>
>> Boots Mark's sol8 install cdrom.  :-)
>>
>> Top of the profile changes from
>>
>>      41.55%  qemu-system-sparc              [.] cpu_exec_loop
>>      14.02%  qemu-system-sparc              [.] cpu_tb_exec
>>       8.74%  qemu-system-sparc              [.] tb_lookup
>>       2.11%  qemu-system-sparc              [.] tcg_splitwx_to_rw
>>       1.63%  memfd:tcg-jit (deleted)        [.] 0x0000000000000004
>>
>> to
>>
>>      31.59%  qemu-system-sparc              [.] helper_lookup_tb_ptr
>>      17.79%  qemu-system-sparc              [.] tb_lookup
>>       5.38%  qemu-system-sparc              [.] compute_all_sub
>>       2.38%  qemu-system-sparc              [.] helper_compute_psr
>>       2.36%  qemu-system-sparc              [.] helper_check_align
>>       1.79%  memfd:tcg-jit (deleted)        [.] 0x000000000063fc8e
>>
>> This probably indicates that cpu_get_tb_cpu_state could be
>> improved to not consume so much overhead.
> 
> Nice! I've just run this through all of my sun4m/sun4u/sun4v test images and I don't 
> see any regressions with v2. The guests feel noticeably more responsive too :)
> 
> Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> I've skimmed the patches and without looking in too much detail they seem to be okay 
> so I'm happy to give:
> 
> Acked-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> Side note: the niagara tests require the patch at 
> https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg03537.html which still 
> hasn't been merged yet.
> 
>> Richard Henderson (8):
>>    target/sparc: Use tcg_gen_lookup_and_goto_ptr in gen_goto_tb
>>    target/sparc: Fix npc comparison in sparc_tr_insn_start
>>    target/sparc: Drop inline markers from translate.c
>>    target/sparc: Introduce DYNAMIC_PC_LOOKUP
>>    target/sparc: Use DYNAMIC_PC_LOOKUP for conditional branches
>>    target/sparc: Use DYNAMIC_PC_LOOKUP for JMPL
>>    target/sparc: Use DYNAMIC_PC_LOOKUP for v9 RETURN
>>    target/sparc: Use tcg_gen_lookup_and_goto_ptr for v9 WRASI
>>
>>   target/sparc/translate.c | 410 ++++++++++++++++++++++-----------------
>>   1 file changed, 233 insertions(+), 177 deletions(-)

I've just noticed during testing there is an issue with this series when used with a 
real SS-5 PROM image (I was using OpenBIOS for my previous tests) which causes it to 
assert() almost immediately on startup:

$ ./qemu-system-sparc -bios ss5.bin
ERROR:../target/sparc/translate.c:5695:sparc_tr_tb_stop: code should not be reached
Bail out! ERROR:../target/sparc/translate.c:5695:sparc_tr_tb_stop: code should not be 
reached
Aborted


ATB,

Mark.