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

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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