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

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

[Maksim Kokryashkin via Tarantool-patches]

From: Mike Pall <mike>

An unused guarded CONV int.num cannot be omitted in general.

(cherry-picked from commit 881d02d3117838acaf4fb844332c8e33cc95c8c5)

In some cases, an unused `CONV int.num` omission in `DUALNUM` mode
may lead to a guard absence, resulting in invalid control flow
branching and undefined behavior. For a comprehensive example of
the described situation, please refer to the comment
in `test/tarantool-tests/mark-conv-non-weak.test.lua`.

Maxim Kokryashkin:
* added the description and the test for the problem

Part of tarantool/tarantool#9145
---
Branch: https://github.com/tarantool/luajit/tree/fckxorg/mark-conv-non-weak
PR: https://github.com/tarantool/tarantool/pull/8871

Changes in v2:
- Fixed comments as per review by Sergey Kaplun
 src/lj_ir.h                                   |   2 +-
 .../mark-conv-non-weak.test.lua               | 115 ++++++++++++++++++
 2 files changed, 116 insertions(+), 1 deletion(-)
 create mode 100644 test/tarantool-tests/mark-conv-non-weak.test.lua

diff --git a/src/lj_ir.h b/src/lj_ir.h
index 46af54e4..e3ba6ff3 100644
--- a/src/lj_ir.h
+++ b/src/lj_ir.h
@@ -132,7 +132,7 @@
   _(XBAR,	S , ___, ___) \
   \
   /* Type conversions. */ \
-  _(CONV,	NW, ref, lit) \
+  _(CONV,	N , ref, lit) \
   _(TOBIT,	N , ref, ref) \
   _(TOSTR,	N , ref, lit) \
   _(STRTO,	N , ref, ___) \
diff --git a/test/tarantool-tests/mark-conv-non-weak.test.lua b/test/tarantool-tests/mark-conv-non-weak.test.lua
new file mode 100644
index 00000000..f54f30ba
--- /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}
+local sum = 0
+
+jit.opt.start('hotloop=2', 'hotexit=2')
+
+-- 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`
+-- 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
+
+test:ok(sum == sum, 'NaN check was not omitted')
+test:done(true)
--
2.39.3 (Apple Git-145)

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

[Sergey Bronnikov via Tarantool-patches]

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?
> +
> +-- 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...
> +-- 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
> +
> +test:ok(sum == sum, 'NaN check was not omitted')
> +test:done(true)
> --
> 2.39.3 (Apple Git-145)
>

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

[Maxim Kokryashkin via Tarantool-patches]

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)
> > 

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

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the patch!
LGTM, just a minor nit below.

On 06.10.23, Maxim Kokryashkin wrote:

<snipped>

> > > +-- 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

Do we really need to start the sum from 0.1, why just not 0?

>  
> -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')

<snipped>

-- 
Best regards,
Sergey Kaplun

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

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for the review!
See my replies below.

On Mon, Oct 09, 2023 at 12:27:35PM +0300, Sergey Kaplun wrote:
> Hi, Maxim!
> Thanks for the patch!
> LGTM, just a minor nit below.
> 
> On 06.10.23, Maxim Kokryashkin wrote:
> 
> <snipped>
> 
> > > > +-- 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
> 
> Do we really need to start the sum from 0.1, why just not 0?
Yep, we really do. Dropped a comment for that, branch is force-pushed.
Here is the diff:
===
diff --git a/test/tarantool-tests/mark-conv-non-weak.test.lua b/test/tarantool-tests/mark-conv-non-weak.test.lua
index b71be4da..331ca25e 100644
--- a/test/tarantool-tests/mark-conv-non-weak.test.lua
+++ b/test/tarantool-tests/mark-conv-non-weak.test.lua
@@ -8,6 +8,11 @@ test:plan(1)
 -- 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}
+
+--- XXX: The sum is required to be initialized with a non-zero
+-- floating point value; otherwise, `0023  + num ADD    0017  0007`
+-- instruction in the IR below becomes `ADDOV` and the `CONV int.num`
+-- conversion is used by it.
 local sum = 0.1
 
 jit.opt.start('hotloop=1')
===
<snipped>

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

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the fixes!
LGTM!

-- 
Best regards,
Sergey Kaplun

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

[Sergey Bronnikov via Tarantool-patches]

On 10/3/23 16:37, Maksim Kokryashkin wrote:
> From: Mike Pall <mike>
>
> An unused guarded CONV int.num cannot be omitted in general.
>
> (cherry-picked from commit 881d02d3117838acaf4fb844332c8e33cc95c8c5)
>
> In some cases, an unused `CONV int.num` omission in `DUALNUM` mode

I would add that DUALNUM could be enabled in CMake by "-DLUAJIT_NUMMODE=2"

because it is not obvious and there is no documentation for CMake options.

> may lead to a guard absence, resulting in invalid control flow
> branching and undefined behavior. For a comprehensive example of
> the described situation, please refer to the comment
> in `test/tarantool-tests/mark-conv-non-weak.test.lua`.
<snipped>