[Tarantool-patches] [PATCH luajit 1/1] x86/x64: Change ipairs_aux to match JIT backend behavior.

[Tarantool-patches] [PATCH luajit 1/1] x86/x64: Change ipairs_aux to match JIT backend behavior.

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

Thanks to Sergey Kaplun.

(cherry picked from commit 64b1f10835acc18bf8923adf248dce4894867882)

In the single-number VM, `ipairs_aux()` first adds the 1 to the given
number and only after casts it to an integer. This leads to results
different from Vanilla Lua 5.1 and inconsistent with JIT engine
recording.

This patch fixes it by casting the value to an integer before addition.

Sergey Kaplun:
* added the description and the test for the problem

Part of tarantool/tarantool#12480
---

Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1463-ipairs-aux-consistency
Related issues:
* https://github.com/LuaJIT/LuaJIT/issues/1463
* https://github.com/tarantool/tarantool/issues/12480

 src/vm_x64.dasc                               |  8 ++---
 src/vm_x86.dasc                               |  7 ++--
 .../lj-1463-ipairs-aux-consistency.test.lua   | 32 +++++++++++++++++++
 3 files changed, 38 insertions(+), 9 deletions(-)
 create mode 100644 test/tarantool-tests/lj-1463-ipairs-aux-consistency.test.lua

diff --git a/src/vm_x64.dasc b/src/vm_x64.dasc
index 6ac88d70..1bd3c855 100644
--- a/src/vm_x64.dasc
+++ b/src/vm_x64.dasc
@@ -1476,17 +1476,15 @@ static void build_subroutines(BuildCtx *ctx)
   |  checkint RA, ->fff_fallback
   |.else
   |  checknumtp [BASE+8], ->fff_fallback
-  |  movsd xmm0, qword [BASE+8]
+  |  cvttsd2si RAd, qword [BASE+8]
   |.endif
   |  mov PC, [BASE-8]
-  |.if DUALNUM
   |  add RAd, 1
+  |.if DUALNUM
   |  setint ITYPE, RA
   |  mov [BASE-16], ITYPE
   |.else
-  |  sseconst_1 xmm1, TMPR
-  |  addsd xmm0, xmm1
-  |  cvttsd2si RAd, xmm0
+  |  cvtsi2sd xmm0, RAd
   |  movsd qword [BASE-16], xmm0
   |.endif
   |  cmp RAd, TAB:RB->asize;  jae >2	// Not in array part?
diff --git a/src/vm_x86.dasc b/src/vm_x86.dasc
index d9234f3b..dcfb0a8b 100644
--- a/src/vm_x86.dasc
+++ b/src/vm_x86.dasc
@@ -1853,10 +1853,9 @@ static void build_subroutines(BuildCtx *ctx)
   |  mov dword [BASE-4], LJ_TISNUM
   |  mov dword [BASE-8], RD
   |.else
-  |  movsd xmm0, qword [BASE+8]
-  |  sseconst_1 xmm1, RBa
-  |  addsd xmm0, xmm1
-  |  cvttsd2si RD, xmm0
+  |  cvttsd2si RD, qword [BASE+8]
+  |  add RD, 1
+  |  cvtsi2sd xmm0, RD
   |  movsd qword [BASE-8], xmm0
   |.endif
   |  mov TAB:RB, [BASE]
diff --git a/test/tarantool-tests/lj-1463-ipairs-aux-consistency.test.lua b/test/tarantool-tests/lj-1463-ipairs-aux-consistency.test.lua
new file mode 100644
index 00000000..1da3a76e
--- /dev/null
+++ b/test/tarantool-tests/lj-1463-ipairs-aux-consistency.test.lua
@@ -0,0 +1,32 @@
+local tap = require('tap')
+
+-- The test file to demonstrate the inconsistent behaviour between
+-- the JIT compiler and the VM for the `ipairs_aux()` function on
+-- x86 and x86_64 arches.
+-- See also: https://github.com/LuaJIT/LuaJIT/issues/1463.
+
+local test = tap.test('lj-1463-ipairs-aux-consistency'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+test:plan(4)
+
+jit.opt.start('hotloop=1')
+
+local ipairs_aux = ipairs({})
+
+local rkeys = {}
+local rvals = {}
+
+for i = 1, 4 do
+  local key, val = ipairs_aux({[0] = 0, [1] = 1}, -0.1)
+  rkeys[i] = key
+  rvals[i] = val
+end
+
+test:is(rkeys[1], 1, 'correct key result')
+test:is(rvals[1], 1, 'correct value result')
+test:samevalues(rkeys, 'consistent JIT and VM behaviour for keys')
+test:samevalues(rvals, 'consistent JIT and VM behaviour for values')
+
+test:done(true)
-- 
2.54.0

Re: [Tarantool-patches] [PATCH luajit 1/1] x86/x64: Change ipairs_aux to match JIT backend behavior.

[Sergey Bronnikov via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 3782 bytes --]

Hi, Sergey,

thanks for the patch! LGTM with a minor comment(s).

Sergey

On 6/5/26 15:49, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Thanks to Sergey Kaplun.
>
> (cherry picked from commit 64b1f10835acc18bf8923adf248dce4894867882)
>
> In the single-number VM, `ipairs_aux()` first adds the 1 to the given
> number and only after casts it to an integer. This leads to results
> different from Vanilla Lua 5.1 and inconsistent with JIT engine

s/Vanilla/PUC Rio/

feel free to ignore

> recording.
>
> This patch fixes it by casting the value to an integer before addition.
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#12480
> ---
>
> Branch:https://github.com/tarantool/luajit/tree/skaplun/lj-1463-ipairs-aux-consistency
> Related issues:
> *https://github.com/LuaJIT/LuaJIT/issues/1463
> *https://github.com/tarantool/tarantool/issues/12480
>
>   src/vm_x64.dasc                               |  8 ++---
>   src/vm_x86.dasc                               |  7 ++--
>   .../lj-1463-ipairs-aux-consistency.test.lua   | 32 +++++++++++++++++++
>   3 files changed, 38 insertions(+), 9 deletions(-)
>   create mode 100644 test/tarantool-tests/lj-1463-ipairs-aux-consistency.test.lua
>
> diff --git a/src/vm_x64.dasc b/src/vm_x64.dasc
> index 6ac88d70..1bd3c855 100644
> --- a/src/vm_x64.dasc
> +++ b/src/vm_x64.dasc
> @@ -1476,17 +1476,15 @@ static void build_subroutines(BuildCtx *ctx)
>     |  checkint RA, ->fff_fallback
>     |.else
>     |  checknumtp [BASE+8], ->fff_fallback
> -  |  movsd xmm0, qword [BASE+8]
> +  |  cvttsd2si RAd, qword [BASE+8]
>     |.endif
>     |  mov PC, [BASE-8]
> -  |.if DUALNUM
>     |  add RAd, 1
> +  |.if DUALNUM
>     |  setint ITYPE, RA
>     |  mov [BASE-16], ITYPE
>     |.else
> -  |  sseconst_1 xmm1, TMPR
> -  |  addsd xmm0, xmm1
> -  |  cvttsd2si RAd, xmm0
> +  |  cvtsi2sd xmm0, RAd
>     |  movsd qword [BASE-16], xmm0
>     |.endif
>     |  cmp RAd,TAB:RB->asize;  jae >2	// Not in array part?
> diff --git a/src/vm_x86.dasc b/src/vm_x86.dasc
> index d9234f3b..dcfb0a8b 100644
> --- a/src/vm_x86.dasc
> +++ b/src/vm_x86.dasc
> @@ -1853,10 +1853,9 @@ static void build_subroutines(BuildCtx *ctx)
>     |  mov dword [BASE-4], LJ_TISNUM
>     |  mov dword [BASE-8], RD
>     |.else
> -  |  movsd xmm0, qword [BASE+8]
> -  |  sseconst_1 xmm1, RBa
> -  |  addsd xmm0, xmm1
> -  |  cvttsd2si RD, xmm0
> +  |  cvttsd2si RD, qword [BASE+8]
> +  |  add RD, 1
> +  |  cvtsi2sd xmm0, RD
>     |  movsd qword [BASE-8], xmm0
>     |.endif
>     |  movTAB:RB, [BASE]
> diff --git a/test/tarantool-tests/lj-1463-ipairs-aux-consistency.test.lua b/test/tarantool-tests/lj-1463-ipairs-aux-consistency.test.lua
> new file mode 100644
> index 00000000..1da3a76e
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1463-ipairs-aux-consistency.test.lua
> @@ -0,0 +1,32 @@
> +local tap = require('tap')
> +
> +-- The test file to demonstrate the inconsistent behaviour between
> +-- the JIT compiler and the VM for the `ipairs_aux()` function on
> +-- x86 and x86_64 arches.
> +-- See also:https://github.com/LuaJIT/LuaJIT/issues/1463.
> +
> +local test = tap.test('lj-1463-ipairs-aux-consistency'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +test:plan(4)
> +
> +jit.opt.start('hotloop=1')
> +
> +local ipairs_aux = ipairs({})
> +
> +local rkeys = {}
> +local rvals = {}
> +
> +for i = 1, 4 do
> +  local key, val = ipairs_aux({[0] = 0, [1] = 1}, -0.1)
> +  rkeys[i] = key
> +  rvals[i] = val
> +end
> +
> +test:is(rkeys[1], 1, 'correct key result')
> +test:is(rvals[1], 1, 'correct value result')
> +test:samevalues(rkeys, 'consistent JIT and VM behaviour for keys')
> +test:samevalues(rvals, 'consistent JIT and VM behaviour for values')
> +
> +test:done(true)

[-- Attachment #2: Type: text/html, Size: 4713 bytes --]

Re: [Tarantool-patches] [PATCH luajit 1/1] x86/x64: Change ipairs_aux to match JIT backend behavior.

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the review!
Fixed your comment and force-pushed the branch.

On 05.06.26, Sergey Bronnikov wrote:
> Hi, Sergey,
> 
> thanks for the patch! LGTM with a minor comment(s).
> 
> Sergey
> 
> On 6/5/26 15:49, Sergey Kaplun wrote:
> > From: Mike Pall <mike>
> >
> > Thanks to Sergey Kaplun.
> >
> > (cherry picked from commit 64b1f10835acc18bf8923adf248dce4894867882)
> >
> > In the single-number VM, `ipairs_aux()` first adds the 1 to the given
> > number and only after casts it to an integer. This leads to results
> > different from Vanilla Lua 5.1 and inconsistent with JIT engine
> 
> s/Vanilla/PUC Rio/
> 
> feel free to ignore

Rephrasing as you suggested. The new commit message is the following:

| x86/x64: Change ipairs_aux to match JIT backend behavior.
|
| Thanks to Sergey Kaplun.
|
| (cherry picked from commit 64b1f10835acc18bf8923adf248dce4894867882)
|
| In the single-number VM, `ipairs_aux()` first adds the 1 to the given
| number and only after casts it to an integer. This leads to results
| different from PUC Rio Lua 5.1 and inconsistent with JIT engine
| recording.
|
| This patch fixes it by casting the value to an integer before addition.
|
| Sergey Kaplun:
| * added the description and the test for the problem
|
| Part of tarantool/tarantool#12480

> 
> > recording.
> >
> > This patch fixes it by casting the value to an integer before addition.
> >
> > Sergey Kaplun:
> > * added the description and the test for the problem
> >
> > Part of tarantool/tarantool#12480
> > ---
> >
> > Branch:https://github.com/tarantool/luajit/tree/skaplun/lj-1463-ipairs-aux-consistency
> > Related issues:
> > *https://github.com/LuaJIT/LuaJIT/issues/1463
> > *https://github.com/tarantool/tarantool/issues/12480
> >

<snipped>

-- 
Best regards,
Sergey Kaplun