[Tarantool-patches] [PATCH luajit 0/2] Fix unsinking TNEW with huge asize

[Tarantool-patches] [PATCH luajit 0/2] Fix unsinking TNEW with huge asize

[Sergey Kaplun via Tarantool-patches]

The first patch isn't necessary for the problem reproducer, but it
simplifies it a lot. Plus, it's an easy optimization, so whatever.
The second patch fixed the mentioned problem.

Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1132-bad-snap-refs

The Tarantool's CI is broken for macOS for now, so just ignore these red
crosses.

Tarantool PR: https://github.com/tarantool/tarantool/pull/9618
Related issues:
* https://github.com/tarantool/tarantool/issues/9595
* https://github.com/LuaJIT/LuaJIT/issues/1128
* https://github.com/LuaJIT/LuaJIT/issues/1132


Mike Pall (2):
  Optimize table.new() with constant args to (sinkable) IR_TNEW.
  Only emit proper parent references in snapshot replay.

 src/lj_ffrecord.c                             |   9 ++
 src/lj_snap.c                                 |  12 +-
 .../lj-1128-table-new-opt-tnew.test.lua       | 112 ++++++++++++++++++
 .../lj-1132-bad-snap-refs.test.lua            |  36 ++++++
 4 files changed, 165 insertions(+), 4 deletions(-)
 create mode 100644 test/tarantool-tests/lj-1128-table-new-opt-tnew.test.lua
 create mode 100644 test/tarantool-tests/lj-1132-bad-snap-refs.test.lua

-- 
2.43.0

[Tarantool-patches] [PATCH luajit 1/2] Optimize table.new() with constant args to (sinkable) IR_TNEW.

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

Thanks to Peter Cawley.

(cherry picked from commit d1236a4caa999b29e774ef5103df3b424d821d9b)

This patch adds optimization for calls of `table.new()` with constant
argument refs when asize is in the proper range (i.e., is less than IR
operand width). The call is replaced with IR TNEW. This opens up
opportunities for other optimizations in the pipeline.

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

Part of tarantool/tarantool#9595
---
 src/lj_ffrecord.c                             |   9 ++
 .../lj-1128-table-new-opt-tnew.test.lua       | 112 ++++++++++++++++++
 2 files changed, 121 insertions(+)
 create mode 100644 test/tarantool-tests/lj-1128-table-new-opt-tnew.test.lua

diff --git a/src/lj_ffrecord.c b/src/lj_ffrecord.c
index 99a6b918..7c6e3590 100644
--- a/src/lj_ffrecord.c
+++ b/src/lj_ffrecord.c
@@ -1065,6 +1065,15 @@ static void LJ_FASTCALL recff_table_new(jit_State *J, RecordFFData *rd)
 {
   TRef tra = lj_opt_narrow_toint(J, J->base[0]);
   TRef trh = lj_opt_narrow_toint(J, J->base[1]);
+  if (tref_isk(tra) && tref_isk(trh)) {
+    int32_t a = IR(tref_ref(tra))->i;
+    if (a < 0x7fff) {
+      uint32_t hbits = hsize2hbits(IR(tref_ref(trh))->i);
+      a = a > 0 ? a+1 : 0;
+      J->base[0] = emitir(IRTG(IR_TNEW, IRT_TAB), (uint32_t)a, hbits);
+      return;
+    }
+  }
   J->base[0] = lj_ir_call(J, IRCALL_lj_tab_new_ah, tra, trh);
   UNUSED(rd);
 }
diff --git a/test/tarantool-tests/lj-1128-table-new-opt-tnew.test.lua b/test/tarantool-tests/lj-1128-table-new-opt-tnew.test.lua
new file mode 100644
index 00000000..805e6de6
--- /dev/null
+++ b/test/tarantool-tests/lj-1128-table-new-opt-tnew.test.lua
@@ -0,0 +1,112 @@
+local tap = require('tap')
+local test = tap.test('lj-1128-table-new-opt-tnew'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+  ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
+})
+
+-- Test LuaJIT optimization when a call to the `lj_tab_new_ah()`
+-- is replaced with the corresponding TNEW IR.
+-- See also https://github.com/LuaJIT/LuaJIT/issues/1128.
+
+local jparse = require('utils').jit.parse
+
+-- API follows the semantics of `lua_createtable()`.
+local table_new = require('table.new')
+
+-- `hbits` for different `hsizes`, see <lj_tab.h> for details.
+local HBITS = {
+  [1] = 1,
+  [3] = 2,
+}
+
+-- XXX: Avoid any other traces compilation due to hotcount
+-- collisions for predictable results.
+jit.off()
+jit.flush()
+
+test:plan(10)
+
+jit.on()
+jit.opt.start('hotloop=1')
+jparse.start('i')
+
+local anchor
+
+for _ = 1, 4 do
+  anchor = table_new(1, 1)
+end
+
+local traces = jparse.finish()
+jit.off()
+
+test:ok(type(anchor) == 'table', 'base result')
+test:ok(traces[1]:has_ir(('TNEW.*#2.*#%d'):format(HBITS[1])), 'base IR value')
+
+jit.flush()
+jit.on()
+-- XXX: Reset hotcounters.
+jit.opt.start('hotloop=1')
+jparse.start('i')
+
+for _ = 1, 4 do
+  anchor = table_new(0, 0)
+end
+
+traces = jparse.finish()
+jit.off()
+
+test:ok(type(anchor) == 'table', 'base result')
+test:ok(traces[1]:has_ir('TNEW.*#0.*#0'), '0 asize, 0 hsize')
+
+jit.flush()
+jit.on()
+-- XXX: Reset hotcounters.
+jit.opt.start('hotloop=1')
+jparse.start('i')
+
+for _ = 1, 4 do
+  anchor = table_new(0, 3)
+end
+
+traces = jparse.finish()
+jit.off()
+
+test:ok(type(anchor) == 'table', 'base result')
+test:ok(traces[1]:has_ir(('TNEW.*#0.*#%d'):format(HBITS[3])),
+        '3 hsize -> 2 hbits')
+
+jit.flush()
+jit.on()
+-- XXX: Reset hotcounters.
+jit.opt.start('hotloop=1')
+jparse.start('i')
+
+for _ = 1, 4 do
+  anchor = table_new(-1, 0)
+end
+
+traces = jparse.finish()
+jit.off()
+
+test:ok(type(anchor) == 'table', 'base result')
+test:ok(traces[1]:has_ir('TNEW.*#0.*#0'), 'negative asize')
+
+jit.flush()
+jit.on()
+-- XXX: Reset hotcounters.
+jit.opt.start('hotloop=1')
+jparse.start('i')
+
+for _ = 1, 4 do
+  anchor = table_new(0xffff, 0)
+end
+
+traces = jparse.finish()
+jit.off()
+
+test:ok(type(anchor) == 'table', 'base result')
+-- Test that TNEW isn't emitted for `asize` bigger than the IR
+-- operand width (>0x8000).
+test:ok(not traces[1]:has_ir('TNEW'), 'asize out of range')
+
+test:done(true)
-- 
2.43.0

[Tarantool-patches] [PATCH luajit 2/2] Only emit proper parent references in snapshot replay.

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

Thanks to Peter Cawley.

(cherry picked from commit 9bdfd34dccb913777be0efcc6869b6eeb5b9b43b)

Assume we have a trace containing the IR instruction:
| {sink}  tab TNEW   #32762  #0

`lj_snap_replay()` assumes that 32762 (0x7ffa) (op1 of TNEW) is a
constant reference. It is passed to the `snap_replay_const()` lookup to
the IR constant in the 0x7ffa slot. If this slot contains the second
part of the IR constant number 0.5029296875 (step of the cycle) in its
raw form (0x3fe0180000000000). The 0x18 part is treated as IROp
(IR_KGC), and JIT is trying to continue with a store of an invalid GC
object, which leads to a crash.

This patch checks that only the IRMref IR operand is needed to restore.

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

Part of tarantool/tarantool#9595
---
 src/lj_snap.c                                 | 12 ++++---
 .../lj-1132-bad-snap-refs.test.lua            | 36 +++++++++++++++++++
 2 files changed, 44 insertions(+), 4 deletions(-)
 create mode 100644 test/tarantool-tests/lj-1132-bad-snap-refs.test.lua

diff --git a/src/lj_snap.c b/src/lj_snap.c
index 3f0fccec..3eb0cd28 100644
--- a/src/lj_snap.c
+++ b/src/lj_snap.c
@@ -516,13 +516,15 @@ void lj_snap_replay(jit_State *J, GCtrace *T)
       IRRef refp = snap_ref(sn);
       IRIns *ir = &T->ir[refp];
       if (regsp_reg(ir->r) == RID_SUNK) {
+	uint8_t m;
 	if (J->slot[snap_slot(sn)] != snap_slot(sn)) continue;
 	pass23 = 1;
 	lj_assertJ(ir->o == IR_TNEW || ir->o == IR_TDUP ||
 		   ir->o == IR_CNEW || ir->o == IR_CNEWI,
 		   "sunk parent IR %04d has bad op %d", refp - REF_BIAS, ir->o);
-	if (ir->op1 >= T->nk) snap_pref(J, T, map, nent, seen, ir->op1);
-	if (ir->op2 >= T->nk) snap_pref(J, T, map, nent, seen, ir->op2);
+	m = lj_ir_mode[ir->o];
+	if (irm_op1(m) == IRMref) snap_pref(J, T, map, nent, seen, ir->op1);
+	if (irm_op2(m) == IRMref) snap_pref(J, T, map, nent, seen, ir->op2);
 	if (LJ_HASFFI && ir->o == IR_CNEWI) {
 	  if (LJ_32 && refp+1 < T->nins && (ir+1)->o == IR_HIOP)
 	    snap_pref(J, T, map, nent, seen, (ir+1)->op2);
@@ -550,14 +552,16 @@ void lj_snap_replay(jit_State *J, GCtrace *T)
       IRIns *ir = &T->ir[refp];
       if (regsp_reg(ir->r) == RID_SUNK) {
 	TRef op1, op2;
+	uint8_t m;
 	if (J->slot[snap_slot(sn)] != snap_slot(sn)) {  /* De-dup allocs. */
 	  J->slot[snap_slot(sn)] = J->slot[J->slot[snap_slot(sn)]];
 	  continue;
 	}
 	op1 = ir->op1;
-	if (op1 >= T->nk) op1 = snap_pref(J, T, map, nent, seen, op1);
+	m = lj_ir_mode[ir->o];
+	if (irm_op1(m) == IRMref) op1 = snap_pref(J, T, map, nent, seen, op1);
 	op2 = ir->op2;
-	if (op2 >= T->nk) op2 = snap_pref(J, T, map, nent, seen, op2);
+	if (irm_op2(m) == IRMref) op2 = snap_pref(J, T, map, nent, seen, op2);
 	if (LJ_HASFFI && ir->o == IR_CNEWI) {
 	  if (LJ_32 && refp+1 < T->nins && (ir+1)->o == IR_HIOP) {
 	    lj_needsplit(J);  /* Emit joining HIOP. */
diff --git a/test/tarantool-tests/lj-1132-bad-snap-refs.test.lua b/test/tarantool-tests/lj-1132-bad-snap-refs.test.lua
new file mode 100644
index 00000000..1f2b5400
--- /dev/null
+++ b/test/tarantool-tests/lj-1132-bad-snap-refs.test.lua
@@ -0,0 +1,36 @@
+local tap = require('tap')
+
+-- Test file to demonstrate LuaJIT's crash in cases of sunk
+-- restore for huge tables.
+-- See also https://github.com/LuaJIT/LuaJIT/issues/1132.
+
+local test = tap.test('lj-1132-bad-snap-refs'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+test:plan(1)
+
+local table_new = require('table.new')
+
+jit.opt.start('hotloop=1', 'hotexit=1')
+
+local result_tab
+-- Create a trace containing the IR instruction:
+-- | {sink}  tab TNEW   #32762  #0
+-- `lj_snap_replay()` assumes that 32762 (0x7ffa) (op1 of TNEW) is
+-- a constant reference. It is passed to the `snap_replay_const()`
+-- lookup to the IR constant in the 0x7ffa slot.
+-- This slot contains the second part of the IR constant
+-- number 0.5029296875 (step of the cycle) in its raw form
+-- (0x3fe0180000000000). The 0x18 part is treated as IROp
+-- (IR_KGC), and JIT is trying to continue with a store of an
+-- invalid GC object, which leads to a crash.
+for i = 1, 2.5, 0.5029296875 do
+  local sunk_tab = table_new(0x7ff9, 0)
+  -- Force the side exit with restoration of the sunk table.
+  if i > 2 then result_tab = sunk_tab end
+end
+
+test:ok(type(result_tab) == 'table', 'no crash during sunk restore')
+
+test:done(true)
-- 
2.43.0

Re: [Tarantool-patches] [PATCH luajit 1/2] Optimize table.new() with constant args to (sinkable) IR_TNEW.

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!

Very thorough testing!

LGTM, except for a minor comment below.

On Wed, Jan 24, 2024 at 05:11:08PM +0300, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Thanks to Peter Cawley.
>
> (cherry picked from commit d1236a4caa999b29e774ef5103df3b424d821d9b)
>
> This patch adds optimization for calls of `table.new()` with constant
> argument refs when asize is in the proper range (i.e., is less than IR
> operand width). The call is replaced with IR TNEW. This opens up
> opportunities for other optimizations in the pipeline.
>
> Sergey Kaplun:
> * added the description and the test for the feature
>
> Part of tarantool/tarantool#9595

<snipped>

> diff --git a/test/tarantool-tests/lj-1128-table-new-opt-tnew.test.lua b/test/tarantool-tests/lj-1128-table-new-opt-tnew.test.lua
> new file mode 100644
> index 00000000..805e6de6
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1128-table-new-opt-tnew.test.lua
> @@ -0,0 +1,112 @@
> +local tap = require('tap')
> +local test = tap.test('lj-1128-table-new-opt-tnew'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +  ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
> +})
> +
> +-- Test LuaJIT optimization when a call to the `lj_tab_new_ah()`
> +-- is replaced with the corresponding TNEW IR.
> +-- See also https://github.com/LuaJIT/LuaJIT/issues/1128.
> +
> +local jparse = require('utils').jit.parse
> +
> +-- API follows the semantics of `lua_createtable()`.
> +local table_new = require('table.new')
> +
> +-- `hbits` for different `hsizes`, see <lj_tab.h> for details.
> +local HBITS = {
> +  [1] = 1,
> +  [3] = 2,
> +}
> +
> +-- XXX: Avoid any other traces compilation due to hotcount
> +-- collisions for predictable results.
> +jit.off()
> +jit.flush()
> +
> +test:plan(10)
> +
> +jit.on()
> +jit.opt.start('hotloop=1')
> +jparse.start('i')
> +
> +local anchor
> +
> +for _ = 1, 4 do
> +  anchor = table_new(1, 1)
> +end
> +
> +local traces = jparse.finish()
> +jit.off()
> +
> +test:ok(type(anchor) == 'table', 'base result')
I would prefer the assertions for the type to have different
descritions for each test case. This way it is easier to tell
which test you need to focus on, if one of them fails. Here
and below.
> +test:ok(traces[1]:has_ir(('TNEW.*#2.*#%d'):format(HBITS[1])), 'base IR value')
> +
> +jit.flush()
> +jit.on()
> +-- XXX: Reset hotcounters.
> +jit.opt.start('hotloop=1')
> +jparse.start('i')
> +
> +for _ = 1, 4 do
> +  anchor = table_new(0, 0)
> +end
> +
> +traces = jparse.finish()
> +jit.off()
> +
> +test:ok(type(anchor) == 'table', 'base result')
> +test:ok(traces[1]:has_ir('TNEW.*#0.*#0'), '0 asize, 0 hsize')
> +
> +jit.flush()
> +jit.on()
> +-- XXX: Reset hotcounters.
> +jit.opt.start('hotloop=1')
> +jparse.start('i')
> +
> +for _ = 1, 4 do
> +  anchor = table_new(0, 3)
> +end
> +
> +traces = jparse.finish()
> +jit.off()
> +
> +test:ok(type(anchor) == 'table', 'base result')
> +test:ok(traces[1]:has_ir(('TNEW.*#0.*#%d'):format(HBITS[3])),
> +        '3 hsize -> 2 hbits')
> +
> +jit.flush()
> +jit.on()
> +-- XXX: Reset hotcounters.
> +jit.opt.start('hotloop=1')
> +jparse.start('i')
> +
> +for _ = 1, 4 do
> +  anchor = table_new(-1, 0)
> +end
> +
> +traces = jparse.finish()
> +jit.off()
> +
> +test:ok(type(anchor) == 'table', 'base result')
> +test:ok(traces[1]:has_ir('TNEW.*#0.*#0'), 'negative asize')
> +
> +jit.flush()
> +jit.on()
> +-- XXX: Reset hotcounters.
> +jit.opt.start('hotloop=1')
> +jparse.start('i')
> +
> +for _ = 1, 4 do
> +  anchor = table_new(0xffff, 0)
> +end
> +
> +traces = jparse.finish()
> +jit.off()
> +
> +test:ok(type(anchor) == 'table', 'base result')
> +-- Test that TNEW isn't emitted for `asize` bigger than the IR
> +-- operand width (>0x8000).
> +test:ok(not traces[1]:has_ir('TNEW'), 'asize out of range')
> +
> +test:done(true)
> --
> 2.43.0
>

Re: [Tarantool-patches] [PATCH luajit 2/2] Only emit proper parent references in snapshot replay.

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
LGTM

On Wed, Jan 24, 2024 at 05:11:09PM +0300, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Thanks to Peter Cawley.
>
> (cherry picked from commit 9bdfd34dccb913777be0efcc6869b6eeb5b9b43b)
>
> Assume we have a trace containing the IR instruction:
> | {sink}  tab TNEW   #32762  #0
>
> `lj_snap_replay()` assumes that 32762 (0x7ffa) (op1 of TNEW) is a
> constant reference. It is passed to the `snap_replay_const()` lookup to
> the IR constant in the 0x7ffa slot. If this slot contains the second
> part of the IR constant number 0.5029296875 (step of the cycle) in its
> raw form (0x3fe0180000000000). The 0x18 part is treated as IROp
> (IR_KGC), and JIT is trying to continue with a store of an invalid GC
> object, which leads to a crash.
>

Oh, god...
> This patch checks that only the IRMref IR operand is needed to restore.
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#9595
> ---
>  src/lj_snap.c                                 | 12 ++++---
>  .../lj-1132-bad-snap-refs.test.lua            | 36 +++++++++++++++++++
>  2 files changed, 44 insertions(+), 4 deletions(-)
>  create mode 100644 test/tarantool-tests/lj-1132-bad-snap-refs.test.lua
>
> diff --git a/src/lj_snap.c b/src/lj_snap.c
> index 3f0fccec..3eb0cd28 100644
> --- a/src/lj_snap.c
> +++ b/src/lj_snap.c
> @@ -516,13 +516,15 @@ void lj_snap_replay(jit_State *J, GCtrace *T)
>        IRRef refp = snap_ref(sn);
>        IRIns *ir = &T->ir[refp];
>        if (regsp_reg(ir->r) == RID_SUNK) {
> +	uint8_t m;
>  	if (J->slot[snap_slot(sn)] != snap_slot(sn)) continue;
>  	pass23 = 1;
>  	lj_assertJ(ir->o == IR_TNEW || ir->o == IR_TDUP ||
>  		   ir->o == IR_CNEW || ir->o == IR_CNEWI,
>  		   "sunk parent IR %04d has bad op %d", refp - REF_BIAS, ir->o);
> -	if (ir->op1 >= T->nk) snap_pref(J, T, map, nent, seen, ir->op1);
> -	if (ir->op2 >= T->nk) snap_pref(J, T, map, nent, seen, ir->op2);
> +	m = lj_ir_mode[ir->o];
> +	if (irm_op1(m) == IRMref) snap_pref(J, T, map, nent, seen, ir->op1);
> +	if (irm_op2(m) == IRMref) snap_pref(J, T, map, nent, seen, ir->op2);
>  	if (LJ_HASFFI && ir->o == IR_CNEWI) {
>  	  if (LJ_32 && refp+1 < T->nins && (ir+1)->o == IR_HIOP)
>  	    snap_pref(J, T, map, nent, seen, (ir+1)->op2);
> @@ -550,14 +552,16 @@ void lj_snap_replay(jit_State *J, GCtrace *T)
>        IRIns *ir = &T->ir[refp];
>        if (regsp_reg(ir->r) == RID_SUNK) {
>  	TRef op1, op2;
> +	uint8_t m;
>  	if (J->slot[snap_slot(sn)] != snap_slot(sn)) {  /* De-dup allocs. */
>  	  J->slot[snap_slot(sn)] = J->slot[J->slot[snap_slot(sn)]];
>  	  continue;
>  	}
>  	op1 = ir->op1;
> -	if (op1 >= T->nk) op1 = snap_pref(J, T, map, nent, seen, op1);
> +	m = lj_ir_mode[ir->o];
> +	if (irm_op1(m) == IRMref) op1 = snap_pref(J, T, map, nent, seen, op1);
>  	op2 = ir->op2;
> -	if (op2 >= T->nk) op2 = snap_pref(J, T, map, nent, seen, op2);
> +	if (irm_op2(m) == IRMref) op2 = snap_pref(J, T, map, nent, seen, op2);
>  	if (LJ_HASFFI && ir->o == IR_CNEWI) {
>  	  if (LJ_32 && refp+1 < T->nins && (ir+1)->o == IR_HIOP) {
>  	    lj_needsplit(J);  /* Emit joining HIOP. */
> diff --git a/test/tarantool-tests/lj-1132-bad-snap-refs.test.lua b/test/tarantool-tests/lj-1132-bad-snap-refs.test.lua
> new file mode 100644
> index 00000000..1f2b5400
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1132-bad-snap-refs.test.lua
> @@ -0,0 +1,36 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate LuaJIT's crash in cases of sunk
> +-- restore for huge tables.
> +-- See also https://github.com/LuaJIT/LuaJIT/issues/1132.
> +
> +local test = tap.test('lj-1132-bad-snap-refs'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +test:plan(1)
> +
> +local table_new = require('table.new')
> +
> +jit.opt.start('hotloop=1', 'hotexit=1')
> +
> +local result_tab
> +-- Create a trace containing the IR instruction:
> +-- | {sink}  tab TNEW   #32762  #0
> +-- `lj_snap_replay()` assumes that 32762 (0x7ffa) (op1 of TNEW) is
> +-- a constant reference. It is passed to the `snap_replay_const()`
> +-- lookup to the IR constant in the 0x7ffa slot.
> +-- This slot contains the second part of the IR constant
> +-- number 0.5029296875 (step of the cycle) in its raw form
> +-- (0x3fe0180000000000). The 0x18 part is treated as IROp
> +-- (IR_KGC), and JIT is trying to continue with a store of an
> +-- invalid GC object, which leads to a crash.
> +for i = 1, 2.5, 0.5029296875 do
> +  local sunk_tab = table_new(0x7ff9, 0)
> +  -- Force the side exit with restoration of the sunk table.
> +  if i > 2 then result_tab = sunk_tab end
> +end
> +
> +test:ok(type(result_tab) == 'table', 'no crash during sunk restore')
> +
> +test:done(true)
> --
> 2.43.0
>

Re: [Tarantool-patches] [PATCH luajit 1/2] Optimize table.new() with constant args to (sinkable) IR_TNEW.

[Sergey Kaplun via Tarantool-patches]

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

On 31.01.24, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the patch!
> 
> Very thorough testing!
> 
> LGTM, except for a minor comment below.
> 
> On Wed, Jan 24, 2024 at 05:11:08PM +0300, Sergey Kaplun wrote:
> > From: Mike Pall <mike>
> >
> > Thanks to Peter Cawley.
> >
> > (cherry picked from commit d1236a4caa999b29e774ef5103df3b424d821d9b)
> >
> > This patch adds optimization for calls of `table.new()` with constant
> > argument refs when asize is in the proper range (i.e., is less than IR
> > operand width). The call is replaced with IR TNEW. This opens up
> > opportunities for other optimizations in the pipeline.
> >
> > Sergey Kaplun:
> > * added the description and the test for the feature
> >
> > Part of tarantool/tarantool#9595
> 
> <snipped>
> 
> > diff --git a/test/tarantool-tests/lj-1128-table-new-opt-tnew.test.lua b/test/tarantool-tests/lj-1128-table-new-opt-tnew.test.lua
> > new file mode 100644
> > index 00000000..805e6de6
> > --- /dev/null
> > +++ b/test/tarantool-tests/lj-1128-table-new-opt-tnew.test.lua
> > @@ -0,0 +1,112 @@
> > +local tap = require('tap')
> > +local test = tap.test('lj-1128-table-new-opt-tnew'):skipcond({
> > +  ['Test requires JIT enabled'] = not jit.status(),
> > +  ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
> > +})
> > +
> > +-- Test LuaJIT optimization when a call to the `lj_tab_new_ah()`
> > +-- is replaced with the corresponding TNEW IR.
> > +-- See also https://github.com/LuaJIT/LuaJIT/issues/1128.
> > +
> > +local jparse = require('utils').jit.parse
> > +
> > +-- API follows the semantics of `lua_createtable()`.
> > +local table_new = require('table.new')
> > +
> > +-- `hbits` for different `hsizes`, see <lj_tab.h> for details.
> > +local HBITS = {
> > +  [1] = 1,
> > +  [3] = 2,
> > +}
> > +
> > +-- XXX: Avoid any other traces compilation due to hotcount
> > +-- collisions for predictable results.
> > +jit.off()
> > +jit.flush()
> > +
> > +test:plan(10)
> > +
> > +jit.on()
> > +jit.opt.start('hotloop=1')
> > +jparse.start('i')
> > +
> > +local anchor
> > +
> > +for _ = 1, 4 do
> > +  anchor = table_new(1, 1)
> > +end
> > +
> > +local traces = jparse.finish()
> > +jit.off()
> > +
> > +test:ok(type(anchor) == 'table', 'base result')
> I would prefer the assertions for the type to have different
> descritions for each test case. This way it is easier to tell
> which test you need to focus on, if one of them fails. Here
> and below.

Yes, sure!
See the iterative patch below. Branch is force-pushed.

===================================================================
diff --git a/test/tarantool-tests/lj-1128-table-new-opt-tnew.test.lua b/test/tarantool-tests/lj-1128-table-new-opt-tnew.test.lua
index 805e6de6..bba160ea 100644
--- a/test/tarantool-tests/lj-1128-table-new-opt-tnew.test.lua
+++ b/test/tarantool-tests/lj-1128-table-new-opt-tnew.test.lua
@@ -39,7 +39,7 @@ end
 local traces = jparse.finish()
 jit.off()
 
-test:ok(type(anchor) == 'table', 'base result')
+test:ok(type(anchor) == 'table', 'type check base result')
 test:ok(traces[1]:has_ir(('TNEW.*#2.*#%d'):format(HBITS[1])), 'base IR value')
 
 jit.flush()
@@ -55,7 +55,7 @@ end
 traces = jparse.finish()
 jit.off()
 
-test:ok(type(anchor) == 'table', 'base result')
+test:ok(type(anchor) == 'table', 'type check 0 asize, 0 hsize')
 test:ok(traces[1]:has_ir('TNEW.*#0.*#0'), '0 asize, 0 hsize')
 
 jit.flush()
@@ -71,7 +71,7 @@ end
 traces = jparse.finish()
 jit.off()
 
-test:ok(type(anchor) == 'table', 'base result')
+test:ok(type(anchor) == 'table', 'type check 3 hsize -> 2 hbits')
 test:ok(traces[1]:has_ir(('TNEW.*#0.*#%d'):format(HBITS[3])),
         '3 hsize -> 2 hbits')
 
@@ -88,7 +88,7 @@ end
 traces = jparse.finish()
 jit.off()
 
-test:ok(type(anchor) == 'table', 'base result')
+test:ok(type(anchor) == 'table', 'type check negative asize')
 test:ok(traces[1]:has_ir('TNEW.*#0.*#0'), 'negative asize')
 
 jit.flush()
@@ -104,7 +104,7 @@ end
 traces = jparse.finish()
 jit.off()
 
-test:ok(type(anchor) == 'table', 'base result')
+test:ok(type(anchor) == 'table', 'type check asize out of range')
 -- Test that TNEW isn't emitted for `asize` bigger than the IR
 -- operand width (>0x8000).
 test:ok(not traces[1]:has_ir('TNEW'), 'asize out of range')
===================================================================

> > +test:ok(traces[1]:has_ir(('TNEW.*#2.*#%d'):format(HBITS[1])), 'base IR value')
> > +
> > +jit.flush()
> > +jit.on()
> > +-- XXX: Reset hotcounters.
> > +jit.opt.start('hotloop=1')
> > +jparse.start('i')
> > +
> > +for _ = 1, 4 do
> > +  anchor = table_new(0, 0)
> > +end
> > +
> > +traces = jparse.finish()
> > +jit.off()
> > +
> > +test:ok(type(anchor) == 'table', 'base result')
> > +test:ok(traces[1]:has_ir('TNEW.*#0.*#0'), '0 asize, 0 hsize')
> > +
> > +jit.flush()
> > +jit.on()
> > +-- XXX: Reset hotcounters.
> > +jit.opt.start('hotloop=1')
> > +jparse.start('i')
> > +
> > +for _ = 1, 4 do
> > +  anchor = table_new(0, 3)
> > +end
> > +
> > +traces = jparse.finish()
> > +jit.off()
> > +
> > +test:ok(type(anchor) == 'table', 'base result')
> > +test:ok(traces[1]:has_ir(('TNEW.*#0.*#%d'):format(HBITS[3])),
> > +        '3 hsize -> 2 hbits')
> > +
> > +jit.flush()
> > +jit.on()
> > +-- XXX: Reset hotcounters.
> > +jit.opt.start('hotloop=1')
> > +jparse.start('i')
> > +
> > +for _ = 1, 4 do
> > +  anchor = table_new(-1, 0)
> > +end
> > +
> > +traces = jparse.finish()
> > +jit.off()
> > +
> > +test:ok(type(anchor) == 'table', 'base result')
> > +test:ok(traces[1]:has_ir('TNEW.*#0.*#0'), 'negative asize')
> > +
> > +jit.flush()
> > +jit.on()
> > +-- XXX: Reset hotcounters.
> > +jit.opt.start('hotloop=1')
> > +jparse.start('i')
> > +
> > +for _ = 1, 4 do
> > +  anchor = table_new(0xffff, 0)
> > +end
> > +
> > +traces = jparse.finish()
> > +jit.off()
> > +
> > +test:ok(type(anchor) == 'table', 'base result')
> > +-- Test that TNEW isn't emitted for `asize` bigger than the IR
> > +-- operand width (>0x8000).
> > +test:ok(not traces[1]:has_ir('TNEW'), 'asize out of range')
> > +
> > +test:done(true)
> > --
> > 2.43.0
> >

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 1/2] Optimize table.new() with constant args to (sinkable) IR_TNEW.

[Sergey Bronnikov via Tarantool-patches]

Hi, Sergey

thanks for the patch! LGTM

On 1/24/24 17:11, Sergey Kaplun wrote:
> From: Mike Pall <mike>
<snipped>

Re: [Tarantool-patches] [PATCH luajit 2/2] Only emit proper parent references in snapshot replay.

[Sergey Bronnikov via Tarantool-patches]

Hi, Sergey

thanks for the patch! test is passed with reverted patch.


cmake -S . -B build -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON

cmake --builddir  build

cmake --builddir build -t LuaJIT-tests


On 1/24/24 17:11, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Thanks to Peter Cawley.
>
> (cherry picked from commit 9bdfd34dccb913777be0efcc6869b6eeb5b9b43b)
>
> Assume we have a trace containing the IR instruction:
> | {sink}  tab TNEW   #32762  #0
>
> `lj_snap_replay()` assumes that 32762 (0x7ffa) (op1 of TNEW) is a
> constant reference. It is passed to the `snap_replay_const()` lookup to
> the IR constant in the 0x7ffa slot. If this slot contains the second
> part of the IR constant number 0.5029296875 (step of the cycle) in its
> raw form (0x3fe0180000000000). The 0x18 part is treated as IROp
> (IR_KGC), and JIT is trying to continue with a store of an invalid GC
> object, which leads to a crash.
>
> This patch checks that only the IRMref IR operand is needed to restore.
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#9595
> ---
>   src/lj_snap.c                                 | 12 ++++---
>   .../lj-1132-bad-snap-refs.test.lua            | 36 +++++++++++++++++++
>   2 files changed, 44 insertions(+), 4 deletions(-)
>   create mode 100644 test/tarantool-tests/lj-1132-bad-snap-refs.test.lua
>
> diff --git a/src/lj_snap.c b/src/lj_snap.c
> index 3f0fccec..3eb0cd28 100644
> --- a/src/lj_snap.c
> +++ b/src/lj_snap.c
> @@ -516,13 +516,15 @@ void lj_snap_replay(jit_State *J, GCtrace *T)
>         IRRef refp = snap_ref(sn);
>         IRIns *ir = &T->ir[refp];
>         if (regsp_reg(ir->r) == RID_SUNK) {
> +	uint8_t m;
>   	if (J->slot[snap_slot(sn)] != snap_slot(sn)) continue;
>   	pass23 = 1;
>   	lj_assertJ(ir->o == IR_TNEW || ir->o == IR_TDUP ||
>   		   ir->o == IR_CNEW || ir->o == IR_CNEWI,
>   		   "sunk parent IR %04d has bad op %d", refp - REF_BIAS, ir->o);
> -	if (ir->op1 >= T->nk) snap_pref(J, T, map, nent, seen, ir->op1);
> -	if (ir->op2 >= T->nk) snap_pref(J, T, map, nent, seen, ir->op2);
> +	m = lj_ir_mode[ir->o];
> +	if (irm_op1(m) == IRMref) snap_pref(J, T, map, nent, seen, ir->op1);
> +	if (irm_op2(m) == IRMref) snap_pref(J, T, map, nent, seen, ir->op2);
>   	if (LJ_HASFFI && ir->o == IR_CNEWI) {
>   	  if (LJ_32 && refp+1 < T->nins && (ir+1)->o == IR_HIOP)
>   	    snap_pref(J, T, map, nent, seen, (ir+1)->op2);
> @@ -550,14 +552,16 @@ void lj_snap_replay(jit_State *J, GCtrace *T)
>         IRIns *ir = &T->ir[refp];
>         if (regsp_reg(ir->r) == RID_SUNK) {
>   	TRef op1, op2;
> +	uint8_t m;
>   	if (J->slot[snap_slot(sn)] != snap_slot(sn)) {  /* De-dup allocs. */
>   	  J->slot[snap_slot(sn)] = J->slot[J->slot[snap_slot(sn)]];
>   	  continue;
>   	}
>   	op1 = ir->op1;
> -	if (op1 >= T->nk) op1 = snap_pref(J, T, map, nent, seen, op1);
> +	m = lj_ir_mode[ir->o];
> +	if (irm_op1(m) == IRMref) op1 = snap_pref(J, T, map, nent, seen, op1);
>   	op2 = ir->op2;
> -	if (op2 >= T->nk) op2 = snap_pref(J, T, map, nent, seen, op2);
> +	if (irm_op2(m) == IRMref) op2 = snap_pref(J, T, map, nent, seen, op2);
>   	if (LJ_HASFFI && ir->o == IR_CNEWI) {
>   	  if (LJ_32 && refp+1 < T->nins && (ir+1)->o == IR_HIOP) {
>   	    lj_needsplit(J);  /* Emit joining HIOP. */
> diff --git a/test/tarantool-tests/lj-1132-bad-snap-refs.test.lua b/test/tarantool-tests/lj-1132-bad-snap-refs.test.lua
> new file mode 100644
> index 00000000..1f2b5400
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1132-bad-snap-refs.test.lua
> @@ -0,0 +1,36 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate LuaJIT's crash in cases of sunk
> +-- restore for huge tables.
> +-- See also https://github.com/LuaJIT/LuaJIT/issues/1132.
> +
> +local test = tap.test('lj-1132-bad-snap-refs'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +test:plan(1)
> +
> +local table_new = require('table.new')
> +
> +jit.opt.start('hotloop=1', 'hotexit=1')
> +
> +local result_tab
> +-- Create a trace containing the IR instruction:
> +-- | {sink}  tab TNEW   #32762  #0
> +-- `lj_snap_replay()` assumes that 32762 (0x7ffa) (op1 of TNEW) is
> +-- a constant reference. It is passed to the `snap_replay_const()`
> +-- lookup to the IR constant in the 0x7ffa slot.
> +-- This slot contains the second part of the IR constant
> +-- number 0.5029296875 (step of the cycle) in its raw form
> +-- (0x3fe0180000000000). The 0x18 part is treated as IROp
> +-- (IR_KGC), and JIT is trying to continue with a store of an
> +-- invalid GC object, which leads to a crash.
> +for i = 1, 2.5, 0.5029296875 do
> +  local sunk_tab = table_new(0x7ff9, 0)
> +  -- Force the side exit with restoration of the sunk table.
> +  if i > 2 then result_tab = sunk_tab end
> +end
> +
> +test:ok(type(result_tab) == 'table', 'no crash during sunk restore')
> +
> +test:done(true)

Re: [Tarantool-patches] [PATCH luajit 2/2] Only emit proper parent references in snapshot replay.

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the review!

On 06.02.24, Sergey Bronnikov wrote:
> Hi, Sergey
> 
> thanks for the patch! test is passed with reverted patch.
> 
> 
> cmake -S . -B build -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON
> 
> cmake --builddir  build
> 
> cmake --builddir build -t LuaJIT-tests

CMake may cache LUAJIT_USE_SYSMALLOC flag, and test may not fail without
it. Was this flag set?

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 2/2] Only emit proper parent references in snapshot replay.

[Sergey Bronnikov via Tarantool-patches]

On 2/6/24 13:07, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the review!
>
> On 06.02.24, Sergey Bronnikov wrote:
>> Hi, Sergey
>>
>> thanks for the patch! test is passed with reverted patch.
>>
>>
>> cmake -S . -B build -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON
>>
>> cmake --builddir  build
>>
>> cmake --builddir build -t LuaJIT-tests
> CMake may cache LUAJIT_USE_SYSMALLOC flag, and test may not fail without
> it. Was this flag set?
>
only  -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON were used, see above

Re: [Tarantool-patches] [PATCH luajit 2/2] Only emit proper parent references in snapshot replay.

[Sergey Kaplun via Tarantool-patches]

On 06.02.24, Sergey Bronnikov wrote:
> 
> On 2/6/24 13:07, Sergey Kaplun wrote:
> > Hi, Sergey!
> > Thanks for the review!
> >
> > On 06.02.24, Sergey Bronnikov wrote:
> >> Hi, Sergey
> >>
> >> thanks for the patch! test is passed with reverted patch.

Do you revert only this patch or the previous one too (or run just on
the cloned master)?
Because without the previous patch (1/2), the repro doesn't work.

I've tried this flags locally and test is failed as expeced for GC64 and
non-GC64 builds.

> >>
> >>
> >> cmake -S . -B build -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON
> >>
> >> cmake --builddir  build
> >>
> >> cmake --builddir build -t LuaJIT-tests
> > CMake may cache LUAJIT_USE_SYSMALLOC flag, and test may not fail without
> > it. Was this flag set?
> >
> only  -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON were used, see above
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 2/2] Only emit proper parent references in snapshot replay.

[Sergey Bronnikov via Tarantool-patches]

Hi, Sergey

(spoiler - LGTM)

On 2/6/24 14:41, Sergey Kaplun wrote:
> On 06.02.24, Sergey Bronnikov wrote:
>> On 2/6/24 13:07, Sergey Kaplun wrote:
>>> Hi, Sergey!
>>> Thanks for the review!
>>>
>>> On 06.02.24, Sergey Bronnikov wrote:
>>>> Hi, Sergey
>>>>
>>>> thanks for the patch! test is passed with reverted patch.
> Do you revert only this patch or the previous one too (or run just on
> the cloned master)?
> Because without the previous patch (1/2), the repro doesn't work.
>
> I've tried this flags locally and test is failed as expeced for GC64 and
> non-GC64 builds.
>
>
Seems it was a problem on my side (probably forgot to stash patch after 
`git reset`).

The problem is reproduced by test, luajit is crashed with segmentation 
fault.

Patch LGTM, thanks!


<snipped>

Re: [Tarantool-patches] [PATCH luajit 0/2] Fix unsinking TNEW with huge asize

[Igor Munkin via Tarantool-patches]

Sergey,

I've checked the patchset into all long-term branches in
tarantool/luajit and bumped a new version in master, release/3.0 and
release/2.11.

On 24.01.24, Sergey Kaplun via Tarantool-patches wrote:
> The first patch isn't necessary for the problem reproducer, but it
> simplifies it a lot. Plus, it's an easy optimization, so whatever.
> The second patch fixed the mentioned problem.
> 
> Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1132-bad-snap-refs
> 
> The Tarantool's CI is broken for macOS for now, so just ignore these red
> crosses.
> 
> Tarantool PR: https://github.com/tarantool/tarantool/pull/9618
> Related issues:
> * https://github.com/tarantool/tarantool/issues/9595
> * https://github.com/LuaJIT/LuaJIT/issues/1128
> * https://github.com/LuaJIT/LuaJIT/issues/1132
> 
> 
> Mike Pall (2):
>   Optimize table.new() with constant args to (sinkable) IR_TNEW.
>   Only emit proper parent references in snapshot replay.
> 
>  src/lj_ffrecord.c                             |   9 ++
>  src/lj_snap.c                                 |  12 +-
>  .../lj-1128-table-new-opt-tnew.test.lua       | 112 ++++++++++++++++++
>  .../lj-1132-bad-snap-refs.test.lua            |  36 ++++++
>  4 files changed, 165 insertions(+), 4 deletions(-)
>  create mode 100644 test/tarantool-tests/lj-1128-table-new-opt-tnew.test.lua
>  create mode 100644 test/tarantool-tests/lj-1132-bad-snap-refs.test.lua
> 
> -- 
> 2.43.0
> 

-- 
Best regards,
IM