Bug in x86_64

[Laurent Michel]

Hi Paolo!

I am porting on the 64bit jit now. And I did stumle on a bug.

I have a code sequence as follows:

jit_prolog(2);                             // tell you have two args

int ofs1 = jit_arg_p();  

int ofs2 = jit_arg_p();  

jit_getarg_p(CJIT_V1, ofs1);    // V1 <- ColAllocator* (arg0) 

jit_getarg_p(CJIT_V2, ofs2);           // V2 <- ColEnvI* (arg1)

Which works smoothing in 32-bit. As you guessed this is the start of a function that takes two pointers as arguments.

Now, on x86_64, the arguments are passed in registers. Specifically

A0 -> RDI

A1 -> RSI

A2 -> RDX

A3 -> RCX

A4 -> R8

A5 -> R9

The code calling the above is a fragment written in C++. I checked its disassembly and it indeed conforms to this protocol. By the time we call, the two pointers (say A,B) are in RDI and RSI

Now here is the intel assembly emitted for the above by lightning:

0x1000362c45: push   %rbx

0x1000362c46: push   %r12

0x1000362c48: push   %r13

0x1000362c4a: push   %rbp

0x1000362c4b: mov    %rsp,%rbp

0x1000362c4e: mov    %rdi,%rsi

0x1000362c51: mov    %rsi,%rdi

The first 4 push operations are part of the callee-saved logic. All good. The instruction

mov %rsp,%rbp  

makes sense as well. It sets up the frame pointer. 

Now the next two essentially correspond to the two getarg_p macros.

Since V1 is mapped to RSI and V2 is mapped to RDI, the first instruction picks up the first argument (say A, currently held in RDI, the register for A0) and moves it to V1 ... which is RSI.

Boom: I just disintegrated A1

The next instruction moves what should have been A1 (but is a copy of A0's content) into V2, aka RDI 

So now A0 == A1, not the intended effect ;-)

Unfortunately, I don't know the 64bit port of lightning well enough to fix it directly. It seems that reusing RSI/RDI that are registers holding Arg1 / Arg0 for the GPR of lightning may be a bad idea. Maybe using two from the range R8..R11 (the caller saved ones) would be better? [I do not know whether lightning uses all the available registers or not] a move from an input register to anything that lightning supports (i.e.,  R0..R2 or V0..V2) is going to be incorrect when the target is also an input register (i.e., R1,R2,V1,V2). 

What are your thoughts?

--

  Laurent

_______________________________________________
Lightning mailing list
Lightning@...
http://lists.gnu.org/mailman/listinfo/lightning

[Paolo Bonzini-2]

> Unfortunately, I don't know the 64bit port of lightning well enough to
> fix it directly. It seems that reusing RSI/RDI that are registers
> holding Arg1 / Arg0 for the GPR of lightning may be a bad idea. Maybe
> using two from the range R8..R11 (the caller saved ones) would be
> better? [I do not know whether lightning uses all the available
> registers or not] a move from an input register to anything that
> lightning supports (i.e.,  R0..R2 or V0..V2) is going to be incorrect
> when the target is also an input register (i.e., R1,R2,V1,V2).
>
> What are your thoughts?

Oops.  Probably it should use those, or use R12/R13 for V1/V2.  I'll
check it out.

Paolo


_______________________________________________
Lightning mailing list
Lightning@...
http://lists.gnu.org/mailman/listinfo/lightning

[Paolo Bonzini-2]

> What are your thoughts?

What about the attached patch?  It just relocates V1 and V2 to R12 and R13.

Paolo

diff --git a/lightning/i386/core-32.h b/lightning/i386/core-32.h
index d93967f..9775fc8 100644
--- a/lightning/i386/core-32.h
+++ b/lightning/i386/core-32.h
@@ -36,6 +36,8 @@
 
 #define JIT_CAN_16 1
 #define JIT_AP _EBP
+#define JIT_V_NUM 3
+#define JIT_V(i) ((i) == 0 ? _EBX : _ESI + (i) - 1)
 
 struct jit_local_state {
   int framesize;
diff --git a/lightning/i386/core-64.h b/lightning/i386/core-64.h
index d2ab96a..da391ae 100644
--- a/lightning/i386/core-64.h
+++ b/lightning/i386/core-64.h
@@ -39,6 +39,9 @@
 #define JIT_CALLTMPSTART 0x48
 #define JIT_REXTMP       0x4B
 
+#define JIT_V_NUM               3
+#define JIT_V(i)                ((i) == 0 ? _EBX : _R11D + (i))
+
 struct jit_local_state {
   int   long_jumps;
   int   nextarg_getfp;
@@ -127,17 +130,13 @@ struct jit_local_state {
 #define jit_pusharg_i(rs) (_jitl.argssize++, MOVQrr(rs, JIT_CALLTMPSTART + _jitl.argssize - 1))
 #define jit_finish(sub)         (MOVQir((long) (sub), JIT_REXTMP), \
  jit_shift_args(), \
- CALLsr(JIT_REXTMP), \
- jit_restore_locals())
+ CALLsr(JIT_REXTMP))
 #define jit_reg_is_arg(reg)     ((reg == _EDI) || (reg ==_ESI) || (reg == _EDX))
 #define jit_finishr(reg) ((jit_reg_is_arg((reg)) ? MOVQrr(reg, JIT_REXTMP) : (void)0), \
                                  jit_shift_args(), \
-                                 CALLsr(jit_reg_is_arg((reg)) ? JIT_REXTMP : (reg)), \
-                                 jit_restore_locals())
+                                 CALLsr(jit_reg_is_arg((reg)) ? JIT_REXTMP : (reg)))
 
-/* R12 and R13 are callee-save, instead of EDI and ESI.  Can be improved. */
 #define jit_shift_args() \
-   (MOVQrr(_ESI, _R12), MOVQrr(_EDI, _R13), \
    (_jitl.argssize--  \
     ? (MOVQrr(JIT_CALLTMPSTART + _jitl.argssize, jit_arg_reg_order[0]),  \
        (_jitl.argssize--  \
@@ -146,10 +145,7 @@ struct jit_local_state {
             ? MOVQrr(JIT_CALLTMPSTART, jit_arg_reg_order[2])  \
             : (void)0)) \
         : (void)0)) \
-    : (void)0))
-
-#define jit_restore_locals() \
-    (MOVQrr(_R12, _ESI), MOVQrr(_R13, _EDI))
+    : (void)0)
 
 #define jit_retval_l(rd) ((void)jit_movr_l ((rd), _EAX))
 #define jit_arg_c()        (jit_arg_reg_order[_jitl.nextarg_geti++])
diff --git a/lightning/i386/core.h b/lightning/i386/core.h
index 3ed6730..ad99d4d 100644
--- a/lightning/i386/core.h
+++ b/lightning/i386/core.h
@@ -39,9 +39,7 @@
 #define JIT_RET _EAX
 
 #define JIT_R_NUM 3
-#define JIT_V_NUM 3
 #define JIT_R(i) (_EAX + (i))
-#define JIT_V(i) ((i) == 0 ? _EBX : _ESI + (i) - 1)
 
 
 /* 3-parameter operation */

_______________________________________________
Lightning mailing list
Lightning@...
http://lists.gnu.org/mailman/listinfo/lightning

[Laurent Michel]

I'll try this morning.

Will keep you posted!

Thanks!

On Jun 13, 2008, at 1:45 AM, Paolo Bonzini wrote:

What are your thoughts?

What about the attached patch?  It just relocates V1 and V2 to R12 and R13.

Paolo
diff --git a/lightning/i386/core-32.h b/lightning/i386/core-32.h
index d93967f..9775fc8 100644
--- a/lightning/i386/core-32.h
+++ b/lightning/i386/core-32.h
@@ -36,6 +36,8 @@

#define JIT_CAN_16 1
#define JIT_AP _EBP
+#define JIT_V_NUM 3
+#define JIT_V(i) ((i) == 0 ? _EBX : _ESI + (i) - 1)

struct jit_local_state {
  int framesize;
diff --git a/lightning/i386/core-64.h b/lightning/i386/core-64.h
index d2ab96a..da391ae 100644
--- a/lightning/i386/core-64.h
+++ b/lightning/i386/core-64.h
@@ -39,6 +39,9 @@
#define JIT_CALLTMPSTART 0x48
#define JIT_REXTMP       0x4B

+#define JIT_V_NUM               3
+#define JIT_V(i)                ((i) == 0 ? _EBX : _R11D + (i))
+
struct jit_local_state {
  int   long_jumps;
  int   nextarg_getfp;
@@ -127,17 +130,13 @@ struct jit_local_state {
#define jit_pusharg_i(rs) (_jitl.argssize++, MOVQrr(rs, JIT_CALLTMPSTART + _jitl.argssize - 1))
#define jit_finish(sub)         (MOVQir((long) (sub), JIT_REXTMP), \
jit_shift_args(), \
- CALLsr(JIT_REXTMP), \
- jit_restore_locals())
+ CALLsr(JIT_REXTMP))
#define jit_reg_is_arg(reg)     ((reg == _EDI) || (reg ==_ESI) || (reg == _EDX))
#define jit_finishr(reg) ((jit_reg_is_arg((reg)) ? MOVQrr(reg, JIT_REXTMP) : (void)0), \
                                 jit_shift_args(), \
-                                 CALLsr(jit_reg_is_arg((reg)) ? JIT_REXTMP : (reg)), \
-                                 jit_restore_locals())
+                                 CALLsr(jit_reg_is_arg((reg)) ? JIT_REXTMP : (reg)))

-/* R12 and R13 are callee-save, instead of EDI and ESI.  Can be improved. */
#define jit_shift_args() \
-   (MOVQrr(_ESI, _R12), MOVQrr(_EDI, _R13), \
   (_jitl.argssize--  \
    ? (MOVQrr(JIT_CALLTMPSTART + _jitl.argssize, jit_arg_reg_order[0]),  \
       (_jitl.argssize--  \
@@ -146,10 +145,7 @@ struct jit_local_state {
            ? MOVQrr(JIT_CALLTMPSTART, jit_arg_reg_order[2])  \
            : (void)0)) \
        : (void)0)) \
-    : (void)0))
-
-#define jit_restore_locals() \
-    (MOVQrr(_R12, _ESI), MOVQrr(_R13, _EDI))
+    : (void)0)

#define jit_retval_l(rd) ((void)jit_movr_l ((rd), _EAX))
#define jit_arg_c()        (jit_arg_reg_order[_jitl.nextarg_geti++])
diff --git a/lightning/i386/core.h b/lightning/i386/core.h
index 3ed6730..ad99d4d 100644
--- a/lightning/i386/core.h
+++ b/lightning/i386/core.h
@@ -39,9 +39,7 @@
#define JIT_RET _EAX

#define JIT_R_NUM 3
-#define JIT_V_NUM 3
#define JIT_R(i) (_EAX + (i))
-#define JIT_V(i) ((i) == 0 ? _EBX : _ESI + (i) - 1)


/* 3-parameter operation */

--

  Laurent

_______________________________________________
Lightning mailing list
Lightning@...
http://lists.gnu.org/mailman/listinfo/lightning