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

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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


Hi!
Thanks for the fixes!
LGTM, except for a single nit below.
 
> 
>>On 21.06.23, Sergey Kaplun via Tarantool-patches wrote:
>>> 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!
>Typo: s/this is/this/
>>>
>>> > >>+-- 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)
>>
>>Brr, acturally, we need to separate two `wrap()` functions - to prevent
>>compilation for the `wrap()` itself as non pcall-ed fixed-arg function.
>>Added the comment.
>>
>>>
>>> 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
>>
>>--
>>Best regards,
>>Sergey Kaplun
>--
>Best regards,
>Maxim Kokryashkin
> 

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