Re: [PATCH v2 0/9] mips: Fill delay slots v2

[Paul Cercueil]

Hi Paulo,

Le mardi 10 janvier 2023 à 13:30 -0300, Paulo César Pereira de Andrade
a écrit :
> Em seg., 9 de jan. de 2023 às 20:04, Paul Cercueil
> <paul@crapouillou.net> escreveu:
> > 
> > Hi Paulo,
> 
>   Hi Paul,
> 
> > I switched to a mips64 toolchain and I could reproduce the build
> > issues.
> > Here's a revised patchset with most of them addressed.
> > 
> > There was a combination of problems: the first one being that my
> > opcode_writes_register() was incomplete (missed all of the MIPS64
> > opcodes and some MIPS32 opcodes as well).
> > 
> > The second problem was that my "no_flag" was incorrectly set after
> > a
> > jit_code_note, jit_code_name or jit_code_prolog.
> > 
> > With this patchset applied I now have all the checks passing
> > successfully, except for the "clobber" one. I am not sure what's
> > going
> > on with this one, and it's hard to debug - the disassembly results
> > in a
> > 30 MiB file, with too many differences to "diff" - because filling
> > the
> > delay slots causes the code to be smaller.
> 
>   I suggest attempting to bisect the problem by editing clobber.tst

I found the problem:

 ori    at,at,0x4
 dsll   at,at,0x10
 ori    at,at,0x1370
-lb     v0,0(at)
 lui    at,0x1
 ori    at,at,0x1
 beq    v1,at
-nop
+lb     v0,0(at)

Here the code sees that the LB only writes v0, which is not read by the
BEQ, so the LB can be moved to the delay slot. However the 'at'
register is still considered dead after the removed LB, while it should
actually be marked live, which means jit_beqi() wouldn't use 'at' as
its temporary.

I'm not sure what's the best way to fix that, I could add a
'op_reads_register' function and mark all input registers of the opcode
as live at the point where it's removed, but maybe there is a better
solution? I'm already under the impression that I'm reinventing the
wheel with op_writes_register().

Cheers,
-Paul


> 
>   You can also edit the macros to add an ending new line, and run
> something like:
> 
> $ gcc -E -x c -D__WORDSIZE=64 -D__LITTLE_ENDIAN -D__mips__=1
> check/clobber.tst > check/clobber.i
> 
> then, run:
> 
> $ cd check
> $ make debug
> ...
> (gdb) b _jit_emit
> (gdb) run -v clobber.i >& clobber.txt
> ...
> (gdb) finish
> 
> assuming you have a build with --enable-devel-disassembler, then, you
> can also add breakpoints in addresses, for example.:
> 
> (gdb) b *0xdeadbeef
> 
> and do single step to debug states. Could also just add a breakpoint
> in
> 'abort' and check from the assembly output the problem (might have
> registers overwritten at abort entry).
> 
>   The clobber test case must pass.
> 
>   It validates that JIT_Rx, JIT_Vx and JIT_Fx not used in the
> operation
> are not clobbered. That is, there are no unexpected side effects.
>   Actually, clobber.tst should be extended as there are new codes
> since
> it was implemented. It also only tests the minimal set of registers,
> 3 JIT_Rx,
> 3 JIT_Vx and 3 JIT_Fx.
> 
> > Cheers,
> > -Paul
> > 
> > Paul Cercueil (8):
> >   mips: Optimize jit_eqr / jit_eqi
> >   mips: Fill delay slots of JR opcodes in jit_jmpr
> >   mips: Fill delay slots of JALR opcodes in jit_callr
> >   mips: Fill delay slots of J in jit_jmpi
> >   mips: Fill delay slots in jit_beqr / jit_beqi
> >   mips: Fill delay slots in jit_bner / jit_bnei
> >   mips: Fill delay slots in jit_bgtr, jit_bgti, jit_bler, jit_blei
> >   mips: Fill delay slots in jit_bger, jit_bgei, jit_bltr, jit_blti
> > 
> >  lib/jit_mips-cpu.c | 731 +++++++++++++++++++++++++----------------
> > ----
> >  lib/jit_mips.c     |  87 ++++--
> >  2 files changed, 466 insertions(+), 352 deletions(-)
> > 
> > --
> > 2.39.0
> 
> Thanks,
> Paulo