Re: [Tarantool-patches] [PATCH luajit v2] Mark CONV as non-weak, to prevent elimination of its side-effect.

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

On Tue, Oct 03, 2023 at 07:24:03PM +0300, Sergey Bronnikov wrote:
> Hi, Max
> 
> 
> thanks for the patch!
> 
> On 10/3/23 16:37, Maksim Kokryashkin wrote:
> 
> 
> <snipped>
> 
> > --- /dev/null
> > +++ b/test/tarantool-tests/mark-conv-non-weak.test.lua
> > @@ -0,0 +1,115 @@
> > +local tap = require('tap')
> > +local test = tap.test('mark-conv-non-weak'):skipcond({
> > +    ['Test requires JIT enabled'] = not jit.status(),
> > +})
> > +
> > +test:plan(1)
> > +
> > +local data = {0.1, 0, 0.1, 0, 0 / 0}
> 
> Is it possible to reduce a number of elements in the table?
> 
> I would add a comment where describe why exactly these magic values were
> chosen.
> 
> > +local sum = 0
> > +
> > +jit.opt.start('hotloop=2', 'hotexit=2')
> Why values are equal to 2 and not 1, that we usually set in tests?

After a bit of tinkering with the repro I've managed to reduce it further.
See the diff below.
> > +
> > +-- XXX: The test fails before the patch only
> > +-- for `DUALNUM` mode. All of the IRs below are
> > +-- produced by the corresponding LuaJIT build.
> > +
> > +-- When the trace is recorded, the IR
> > +-- is the following before the patch:
> > +---- TRACE 1 IR
> > +-- ....        SNAP   #0   [ ---- ---- ---- ---- ---- ---- ---- ---- ---- ]
> > +-- 0001    u8  XLOAD  [0x100dac521]  V
> > +-- 0002    int BAND   0001  +12
> > +-- 0003 >  int EQ     0002  +0
> > +-- 0004 >  int SLOAD  #8    T
> > +-- ....        SNAP   #1   [ ---- ---- ---- ---- ---- ---- ---- ---- ---- ]
> > +-- 0005 >  num SLOAD  #3    T
> > +-- 0006    num CONV   0004  num.int
> > +-- 0007  + num ADD    0006  0005
> > +-- 0008 >  fun SLOAD  #4    T
> > +-- 0009 >  tab SLOAD  #5    T
> > +-- 0010 >  int SLOAD  #6    T
> > +-- 0011 >  fun EQ     0008  ipairs_aux
> > +-- 0012  + int ADD    0010  +1
> > +-- 0013    int FLOAD  0009  tab.asize
> > +-- 0014 >  int ABC    0013  0012
> > +-- 0015    p64 FLOAD  0009  tab.array
> > +-- 0016    p64 AREF   0015  0012
> > +-- 0017 >+ num ALOAD  0016
> > +-- ....        SNAP   #2   [ ---- ---- ---- 0007 ---- ---- 0012 0012 0017 ]
> > +-- 0018 ------ LOOP ------------
> > +-- 0019    u8  XLOAD  [0x100dac521]  V
> > +-- 0020    int BAND   0019  +12
> > +-- 0021 >  int EQ     0020  +0
> > +-- 0022 >  int CONV   0017  int.num
> > +-- ....        SNAP   #3   [ ---- ---- ---- 0007 ---- ---- 0012 0012 0017 ]
> > +-- 0023  + num ADD    0017  0007
> > +-- 0024  + int ADD    0012  +1
> > +-- 0025 >  int ABC    0013  0024
> > +-- 0026    p64 AREF   0015  0024
> > +-- 0027 >+ num ALOAD  0026
> > +-- 0028    num PHI    0017  0027
> > +-- 0029    num PHI    0007  0023
> > +-- 0030    int PHI    0012  0024
> > +---- TRACE 1 stop -> loop
> > +
> > +---- TRACE 1 exit 0
> > +---- TRACE 1 exit 3
> > +--
> > +-- And the following after the patch:
> > +---- TRACE 1 IR
> > +-- ....        SNAP   #0   [ ---- ---- ---- ---- ---- ---- ---- ---- ---- ]
> > +-- 0001    u8  XLOAD  [0x102438521]  V
> > +-- 0002    int BAND   0001  +12
> > +-- 0003 >  int EQ     0002  +0
> > +-- 0004 >  int SLOAD  #8    T
> > +-- ....        SNAP   #1   [ ---- ---- ---- ---- ---- ---- ---- ---- ---- ]
> > +-- 0005 >  num SLOAD  #3    T
> > +-- 0006    num CONV   0004  num.int
> > +-- 0007  + num ADD    0006  0005
> > +-- 0008 >  fun SLOAD  #4    T
> > +-- 0009 >  tab SLOAD  #5    T
> > +-- 0010 >  int SLOAD  #6    T
> > +-- 0011 >  fun EQ     0008  ipairs_aux
> > +-- 0012  + int ADD    0010  +1
> > +-- 0013    int FLOAD  0009  tab.asize
> > +-- 0014 >  int ABC    0013  0012
> > +-- 0015    p64 FLOAD  0009  tab.array
> > +-- 0016    p64 AREF   0015  0012
> > +-- 0017 >+ num ALOAD  0016
> > +-- ....        SNAP   #2   [ ---- ---- ---- 0007 ---- ---- 0012 0012 0017 ]
> > +-- 0018 ------ LOOP ------------
> > +-- 0019    u8  XLOAD  [0x102438521]  V
> > +-- 0020    int BAND   0019  +12
> > +-- 0021 >  int EQ     0020  +0
> > +-- 0022 >  int CONV   0017  int.num
> > +-- ....        SNAP   #3   [ ---- ---- ---- 0007 ---- ---- 0012 0012 0017 ]
> > +-- 0023  + num ADD    0017  0007
> > +-- 0024  + int ADD    0012  +1
> > +-- 0025 >  int ABC    0013  0024
> > +-- 0026    p64 AREF   0015  0024
> > +-- 0027 >+ num ALOAD  0026
> > +-- 0028    num PHI    0017  0027
> > +-- 0029    num PHI    0007  0023
> > +-- 0030    int PHI    0012  0024
> > +---- TRACE 1 stop -> loop
> > +
> > +---- TRACE 1 exit 0
> > +---- TRACE 1 exit 2
> > +--
> > +-- Before the patch, the `0022 >  int CONV   0017  int.num`
> I see that IR "0022 > int CONV ..." is present in both IR traces...
Yep, they are omitted due to DCE and it happens on the trace assembly
stage. Dropped a comment.
> > +-- instruction is omitted due to DCE, which results in the
> > +-- third side exit being taken, instead of the second,
> > +-- and, hence, incorrect summation. After the patch, `CONV`
> > +-- is left intact and is not omitted; it remains as a guarded
> > +-- instruction, so the second side exit is taken and sum is
> > +-- performed correctly.
> > +
> > +for _, val in ipairs(data) do
> > +    if val == val then
> > +        sum = sum + val
> > +    end
> > +end
Here is the diff with changes. Branch is force-pushed:
===
diff --git a/test/tarantool-tests/mark-conv-non-weak.test.lua b/test/tarantool-tests/mark-conv-non-weak.test.lua
index f54f30ba..b71be4da 100644
--- a/test/tarantool-tests/mark-conv-non-weak.test.lua
+++ b/test/tarantool-tests/mark-conv-non-weak.test.lua
@@ -4,11 +4,13 @@ local test = tap.test('mark-conv-non-weak'):skipcond({
 })
 
 test:plan(1)
+-- XXX: These values were chosen to create type instability
+-- in the loop-carried dependency, so the checked `CONV int.num`
+-- instruction is emitted. See `loop_unrool` in `lj_opt_loop.c`.
+local data = {0, 0.1, 0, 0 / 0}
+local sum = 0.1
 
-local data = {0.1, 0, 0.1, 0, 0 / 0}
-local sum = 0
-
-jit.opt.start('hotloop=2', 'hotexit=2')
+jit.opt.start('hotloop=1')
 
 -- XXX: The test fails before the patch only
 -- for `DUALNUM` mode. All of the IRs below are
@@ -104,6 +106,9 @@ jit.opt.start('hotloop=2', 'hotexit=2')
 -- is left intact and is not omitted; it remains as a guarded
 -- instruction, so the second side exit is taken and sum is
 -- performed correctly.
+--
+-- Note that DCE happens on the assembly part of the trace
+-- compilation. That is why `CONV` is present in both IRs.
 
 for _, val in ipairs(data) do
     if val == val then
===
> > +
> > +test:ok(sum == sum, 'NaN check was not omitted')
> > +test:done(true)
> > --
> > 2.39.3 (Apple Git-145)
> >