[Lightning] Re: Updates for x86_64

[Paulo Cesar Pereira de Andrade]

On 24-08-2010 07:38, Paolo Bonzini wrote:
> On 08/24/2010 01:12 AM, Paulo César Pereira de Andrade wrote:
> > Patch1 is a clear typo and should be trivial.

Patch1 merged on patch4, to avoid one liner and another change to
the same chunk in the same patch series.

> Ok.
>
> > Patch2 changes JIT_REXTMP to %r11, so that, when calling a function
> > with 6 integer arguments, it will not clobber the 6th argument in the
> > last moment, due to putting the function pointer on it.
> >
> > /* Used to implement ldc, stc, ... */
> > #define JIT_CAN_16 0
> > -#define JIT_REXTMP _R9D
> > +#define JIT_REXTMP _R11D
> >
> > #define JIT_R_NUM 3
> > #define JIT_R(i) ((i) == 0 ? _EAX : _R9D + (i))
>
> Doesn't this mean JIT_R2 overlaps JIT_REXTMP? Please redo the patch
> using %r12, and at the same time moving V1/V2 to r13/r14.

Patch1, revious patch2, now partially merged with previous patch4,
but without increasing JIT_V_NUM, just relocating values, and now
using %r14 for JIT_V3.
Now it also does not do an extra %rbx push just to align the stack.
This should be the only possible issue in the patch, please review.

> > Patch3 defines symbolic names for extra SSE2 %xmmN registers, and
> > sets JIT_FPTMP to the topmost one.
>
> Ok.

Patch2, in this review, now it reduces JIT_FPR_NUM to avoid clobbering
JIT_FPRTMP with JIT_FPR(7), that now is no longer valid if checking
JIT_FPR_NUM.

> > Patch4 increases JIT_V_NUM to 5, and make %r14 and %r15 available,
> > at the cost of needing to also save them. They are callee save registers
> > in the abi.
>
> I'd rather skip this one, 3 callee-save register are often enough. %r14
> is going to be used anyway for V2 after you redo patch 2.

  Ok.

> > Patch5 adds safety check on number of integer or float arguments
> > being passed to a function, and also increases the number of float
> > register arguments to 8. Also, it defines JIT_RA_NUM and JIT_FA_NUM
> > that matches the number of integer and float register arguments.

Patch3 now, just rediffed.

> > Patch6 also changes the mapping of JIT_R(num) and JIT_V(num), as well
> > as JIT_REXTMP to use the 64 bits defines, so that it will not trigger
> > an JITFAIL on jit_getarg_l and jit_getarg_ul for example, due to _rC
> > telling it is a 32 bits registers. This is only an issue when
> > _ASM_SAFETY is defined.

Patch4 now. Actually, I removed the changes to treat R11, ... R15 as 64
bit registers, and to keep using the R11d ... R15d names (32 bit names),
to not modify the _ASM_SAFETY failures in this commit, as changing it
just trades one failure kind by another.

> Both look fine.
>
> Paolo

  I believe that, without possible issue with not pushing %rbx twice
in patch1, these patches should be very safe, and correct.

Paulo

0001-Change-JIT_REXTMP-to-not-clobber-6th-argument.patch
Description: Text document

0002-Define-extra-SSE2-xmmN-registers.patch
Description: Text document

0003-Add-safety-check-for-number-of-integer-and-float-reg.patch
Description: Text document

0004-Change-jit_getarg_-c-uc-s-us-i-ui-to-sign-zero-exten.patch
Description: Text document