[Tarantool-patches] [PATCH luajit v2] Mark CONV as non-weak, to prevent elimination of its side-effect.
Maxim Kokryashkin
m.kokryashkin at tarantool.org
Fri Oct 6 13:49:18 MSK 2023
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)
> >
More information about the Tarantool-patches
mailing list