Re: [Lightning] jit_qdivr_u trashes JIT_R0 on x86_64

[Paulo César Pereira de Andrade]

Em sex, 30 de ago de 2019 às 17:59, Paul Cercueil <address@hidden> escreveu:

  Hi Paul,

> > [...]
> >>  I tried, but I could not succeed to make a test case that shows the
> >>  issue... All I tried have been working just fine.
> >>
> >>  The actual code that triggers the issue is this one:
> >>  https://gist.github.com/pcercuei/8db789b415f6ced73abb01a6504b64c7
> >>  As it's generated, it's not the easiest to understand, sorry...
> >>
> >>  The thing to see, is that line 26 I write register 0 (rax), which is
> >>  then untouched until line 58 or 71.
> >>
> >>  This is what Lightning generates:
> >>  https://gist.github.com/pcercuei/6b4afef692682b139a46449665454681
> >>
> >>  As you can see, line 30 %rax is written to, and %eax is written
> >> back to
> >>  memory on lines 69 or 84.
> >>  But also, line 49 %rax is unconditionally overwritten.
> >
> >   The problem is that after the code does JIT_R0 = JIT_V1 + JIT_V2,
> > that
> > is %rax = %r13 + %r14 -- lea 0x0(%r13,%r14,1),%rax -- there is a
> > function
> > call. %rax is not callee save, you need to save it to memory, and
> > restore
> > once the function returns. The called function is free to change %rax.
> > this is normal for all JIT_Rx. Only JIT_Vx registers are not modified.
>
> Yes, this is pretty clear to me; but the code it's jumping to is
> guaranteed to leave all registers untouched.

  I might have missed some context, but at
https://gist.github.com/pcercuei/8db789b415f6ced73abb01a6504b64c7
it clearly shows %rax is used as a result of an jit_addr, that is shortly
followed by a function call. The construct for the function call is very
clever :) and ensures that %rax is not clobbered by lightning; only the
called function can do it. But, once returning from the function, lightning
considers its value dead. Well, it is the return register, so, if the return
value is required, it must be used just after the function call.

> > If you are 100% sure the called function does not change JIT_R0, you
> > could
> > add jit_live(JIT_R0) after the jit_callr(JIT_V0), but that would not
> > work
> > because it would normally choose %rax as a function pointer (this is
> > a long
> > TODO to generate %rip displacement calls), or, you could cause it to
> > choose
> > another register as the function pointer, by creating a jump over the
> > function call or some other construct, but that also would not work if
> > calling a varargs function, because the abi requires adding the
> > number of
> > FPR register/arguments in %rax.
> >   Basically, if there is a function call, jit_jmpi to unknown
> > location (not
> > to a jit_label_t) or a jit_jmpr, all non callee save registers are
> > considered dead in the next instruction. It does this also for
> > jit_jmpi
> > (to unknown location) and jit_jmpr because otherwise the register
> > allocator
> > would run out of registers, as it cannot follow where the jump will
> > land,
> > and what will happen there.
>
> Ok, makes sense. So in my case, I use jit_movi(reg, addr) then
> jit_callr(reg), as I know "reg" is a free register, that's why it
> doesn't choose %rax as the function pointer (unless %rax is a free
> register).

  Here the problem is that qdivr needs %rax and %rdx. And, because
lightning does not know it is live, it could have been used as a
scratch register in some instruction that required a temporary, and
there was no encoding for an immediate value.

> I didn't know about jit_live(), thanks. Do I need a jit_dead() too, to
> mark known dead registers as dead? Or is Lightning smart enough to
> detect it? (e.g. JIT_V0 being written 10 instructions after being read
> last, can be marked as dead in between)

  There isn't a jit_dead(v) call, it should be trivial to implement.
For the moment I would prefer to not implement it, as it could cause
some very hard to debug bugs if used incorrectly. But would be still
very useful, as would create faster jit generation, due to avoiding some
possible time consuming scans in the jit IR, before native code
generation.

  Lightning detects dead registers if the value is not used, for example,
a very simple case:

movi %r0 10 // this is optional, just an example of unknown/unused state
[lots of instructions that do not use %r0]
movi %r0 11

lightning will consider %r0 dead in the [lots of instructions that do
not use %r0]
and freely use it as a scratch register.
  If in the [lots...] there is a function call, a jmpr or a jmpi to a
non jit_label_t it
will also consider it dead, after the jmpr, jmpi or function call. But
on the later
case, if it is a JIT_Vx, it will be considered live in all the range, as JIT_Vx
maps to callee save registers, while JIT_Rx maps to call save registers.
There is also jit_callee_save_p(reg), that is helpful, as in a few
ports, the JIT_Rx
and JIT_Fx, are actually mapped to registers that have value preserved across
function calls, and then it is not required to spill/reload the value.

> >   The closest/minimal reproducer for the issue does not even need the
> > qdivr, because once jit_qdivr is called, %rax is already considered
> > dead, is:
> >
> > """
> > .data    32
> > fmt:
> > .c    "%d\n"
> > .code
> >     jmpi main
> > func:
> >     prolog
> >     //reti 2
> >     epilog
> > main:
> >     prolog
> >     movi %v1 2
> >     movi %v2 3
> >     addr %r0 %v1 %v2
> >     movi %v(3) func
> >     callr %v(3)
> >     //movi %r0 5
> >     //qdivr %v1 %v2 %v2 %v1
> >     prepare
> >         pushargi fmt
> >         ellipsis
> >         pushargr %r0
> >     finishi @printf
> >     ret
> >     epilog
> > """
> >   If you uncomment the 'reti 2' it will print 5, otherwise it will
> > print 2.

  I got this reversed :) uncommenting reti2 will print 2, otherwise, because
%rax is not touched, it would still print 5.

> Very insightful, thanks.
>
> >   BTW, I see you found another way to workaround the issues with %rax
> > described above by manually using callr. So, even if no code touchs
> > %rax,
> > it will be considered dead in jit_qdivr and it will not save/restore
> > it.
> >   If you uncomment only the qdivr above, %rax will be 1, and if you
> > also
> > uncomment the 'movi %r0 5' (to simulate quickly a reload from memory)
> > you will notice that the qdvir will not clobber %rax, because then it
> > understands it is live.
>
> Got it.
>
> Thanks!
> -Paul

Thanks,
Paulo