Re: [Tarantool-patches] [PATCH luajit 1/2] Fix use-def analysis for BC_VARG.

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

Hi, Maxim!
Thanks for the review!

On 14.06.23, Maxim Kokryashkin wrote:
> 
> Hi, Sergey!
> Thanks for the patch!
> Please consider my comments below.
>  
> > 
> >>From: Mike Pall <mike>
> >>
> >>Reported by Ryan Lucia.
> >>
> >>(cherry-picked from commit 2801500a26084491ae035170cad4700513790890)
> >>
> >>Use-def analizis for BC_VARG has to strong limit for the top/maxslot, so
> >Typo: s/analizis/analysis/

Fixed! Thanks!

> >>no slots may considered as used. This leads to addititional SLOAD on
> >Typo: s/may/may be/
> >Typo: s/to additional/to an additional/

Fixed.

> >>trace with incorrect value used later. This patch disables the use-def
> >Typo: s/trace with/the trace with an/

Fixed.

> >>analisis for BC_VARG as NIY.
> >Typo: s/analisis/analysis/

Fixed, thanks!

> >>
> >>Sergey Kaplun:
> >>* added the description and the test for the problem
> >>
> >>Part of tarantool/tarantool#8516
> >>Relates to tarantool/tarantool#8718
> >>---
> >> src/lj_snap.c | 4 +-
> >> .../lj-704-bc-varg-use-def.test.lua | 65 +++++++++++++++++++
> >> 2 files changed, 68 insertions(+), 1 deletion(-)
> >> create mode 100644 test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
> >>

<snipped>

> >>diff --git a/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
> >>new file mode 100644
> >>index 00000000..c3ba65dd
> >>--- /dev/null
> >>+++ b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
> >>@@ -0,0 +1,65 @@
> >>+local tap = require('tap')
> >>+-- Test file to demonstrate LuaJIT misbehaviour in use-def
> >>+-- snapshot analysis for BC_VARG.
> >>+-- See also  https://github.com/LuaJIT/LuaJIT/issues/704 .
> >>+local test = tap.test('lj-704-bc-varg-use-def'):skipcond({
> >>+ ['Test requires JIT enabled'] = not jit.status(),
> >>+})
> >>+
> >>+test:plan(1)
> >>+
> >>+-- XXX: we don't really need to store this builtins, but this is
> >Typo: s/this/these/

Fixed, thanks!

> >>+-- reduces `jitdump()` output for reader significantly.
> >>+local fmod = math.fmod
> >>+local pcall = pcall
> >>+
> >>+-- Use the 2 values for `fmod()` to produce non-zero value for
> >>+-- the call on trace (the last one call).
> >>+local ARG_ON_RECORDING = 6
> >>+local ON_TRACE_VALUE = ARG_ON_RECORDING + 1
> >Why are they exactly 6 and 7? Please drop a comment.

No special meaning, added a comment.

> >>+
> >>+-- The `jitdump()` output was like the following before the patch:
> >>+-- 0003 > num SLOAD #1 T
> >>+-- .... SNAP #1 [`wrap()`|---- pcall|`varg()`|----]
> >>+-- 0004 } tab TNEW #3 #0
> >>+-- 0005 > num SLOAD #4 T
> >>+-- 0006 p32 FLOAD 0004 tab.array
> >>+-- 0007 p32 AREF 0006 +1
> >>+-- 0008 } num ASTORE 0007 0005
> >>+-- .... SNAP #2 [`wrap()`|---- pcall|math.fmod|+6 0005]
> >>+--
> >>+-- The first snapshot misses the 0003 IR in the last slot to be
> >>+-- used in the `fmod()` later, so it leads to the additional
> >>+-- 0005 SLOAD #4, and storing it in the second snapshot.
> >>+--
> >>+-- The correct snapshot content after the patch is the following:
> >>+-- .... SNAP #1 [`wrap()`|---- pcall|`varg()`|0003]
> >>+-- ....
> >>+-- .... SNAP #2 [`wrap()`|---- pcall|math.fmod|+6 0003]
> >>+local function varg(...)
> >>+ -- Generate snapshot after `pcall()` with missing slot.
> >>+ -- The snapshot is generated before each TNEW after the commit
> >>+ -- 7505e78bd6c24cac6e93f5163675021734801b65 ("Handle on-trace
> >>+ -- OOM errors from helper functions.")
> >>+ local slot = ({...})[1]
> >>+ -- Forcify stitch and usage of vararg slot.
> >>+ return fmod(ARG_ON_RECORDING, slot)
> >Are there any reasons behind the `fmod` choice? If so, please drop a comment.

No, added the comment.

> >>+end
> >>+
> >>+jit.opt.start('hotloop=1')
> >>+
> >>+local _, result
> >>+local function wrap(arg)
> >>+ -- `pcall()` is needed to emit snapshot to handle on-trace
> >>+ -- errors.
> >Maybe it is worth mentioning Mike’s original comment[1] here.
> >Feel free to ignore.

I just added the reference to the issue in the header, the comment above
is about the same as Mike's but more verbose.

===================================================================
diff --git a/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
index c3ba65dd..3608ea4e 100644
--- a/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
+++ b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
@@ -8,13 +8,13 @@ local test = tap.test('lj-704-bc-varg-use-def'):skipcond({
 
 test:plan(1)
 
--- XXX: we don't really need to store this builtins, but this is
+-- XXX: we don't really need to store these builtins, but this is
 -- reduces `jitdump()` output for reader significantly.
 local fmod = math.fmod
 local pcall = pcall
 
 -- Use the 2 values for `fmod()` to produce non-zero value for
--- the call on trace (the last one call).
+-- the call on trace (the last one call). No special meaning.
 local ARG_ON_RECORDING = 6
 local ON_TRACE_VALUE = ARG_ON_RECORDING + 1
 
@@ -42,23 +42,23 @@ local function varg(...)
   -- 7505e78bd6c24cac6e93f5163675021734801b65 ("Handle on-trace
   -- OOM errors from helper functions.")
   local slot = ({...})[1]
-  -- Forcify stitch and usage of vararg slot.
+  -- Forcify stitch and usage of vararg slot. Any NIY is OK here.
   return fmod(ARG_ON_RECORDING, slot)
 end
 
 jit.opt.start('hotloop=1')
 
 local _, result
-local function wrap(arg)
+local function wrap(func, arg)
   -- `pcall()` is needed to emit snapshot to handle on-trace
   -- errors.
-  _, result = pcall(varg, arg)
+  _, result = pcall(func, arg)
 end
 -- Record trace with the 0 result.
-wrap(ARG_ON_RECORDING)
-wrap(ARG_ON_RECORDING)
+wrap(varg, ARG_ON_RECORDING)
+wrap(varg, ARG_ON_RECORDING)
 -- Record trace with the non-zero result.
-wrap(ON_TRACE_VALUE)
+wrap(varg, ON_TRACE_VALUE)
 
 test:ok(result ~= 0, 'use-def analysis for BC_VARG')
===================================================================

I also modify `wrap()` function to get the function to call considering
your comments in the next patch.

> >>+ _, result = pcall(varg, arg)
> >>+end
> >>+-- Record trace with the 0 result.
> >>+wrap(ARG_ON_RECORDING)
> >>+wrap(ARG_ON_RECORDING)
> >>+-- Record trace with the non-zero result.
> >>+wrap(ON_TRACE_VALUE)
> >>+
> >>+test:ok(result ~= 0, 'use-def analysis for BC_VARG')
> >>+
> >>+os.exit(test:check() and 0 or 1)
> >>--
> >>2.34.1
> >[1]:  https://github.com/LuaJIT/LuaJIT/issues/704
> >--
> >Best regards,
> >Maxim Kokryashkin

-- 
Best regards,
Sergey Kaplun