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

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

Hi, Maxim!
Thanks for the patch!
Please consider my comment below.

On 12.07.23, 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` omission 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#8825
> ---

<snipped>

> +
> +jit.opt.start('hotloop=1', 'hotexit=1')
> +
> +-- When the last trace is recorded, the traced bytecode
> +-- is the following before the patch:
> +-- ---- TRACE 4 start 2/3 test.lua:6
> +-- 0018  ADDVV    1   1   6
> +-- 0019  ITERC    5   3   3
> +-- 0000  . FUNCC               ; ipairs_aux
> +-- 0020  JITERL   5   1
> +-- 0021  GGET     2   7      ; "assert"
> +-- 0022  ISEQV    1   1
> +-- 0023  JMP      4 => 0026
> +-- 0024  KPRI     4   1
> +-- 0025  JMP      5 => 0027
> +-- 0027  CALL     2   1   2
> +-- 0000  . FUNCC               ; assert
> +--
> +-- And the following after the patch:
> +-- ---- TRACE 4 start 2/2 test.lua:5
> +-- 0016  ISNEV    6   6
> +-- 0017  JMP      7 => 0019
> +-- 0019  ITERC    5   3   3
> +-- 0000  . FUNCC               ; ipairs_aux
> +-- 0020  JITERL   5   1
> +-- 0021  GGET     2   7      ; "assert"
> +-- 0022  ISEQV    1   1
> +-- 0023  JMP      4 => 0026
> +-- 0026  KPRI     4   2
> +-- 0027  CALL     2   1   2
> +-- 0000  . FUNCC               ; assert
> +-- 0028  RET0     0   1
> +--
> +-- The crucial difference here is the abscent
> +-- `ISNEV` in the first case, which produces the
> +-- desired guarded `CONV`, when translated to IR.

There are two important statements:
As you mentioned, the case is observed only in the DUALNUM mode.
As mentioned in the commit, this is due to `CONV int.num` IRs.

The comment for the test itself is misleading the reason of misbehaviour
isn't the wrong bytecode in the recording -- it's the consequence.

Let's use the reproducer, but with hotloop=2 to decrease amount of
traces:

| LUA_PATH="src/?.lua;;" src/luajit -jdump=X -e "
| local data = {0.1, 0, 0.1, 0, 0 / 0}
| local sum = 0
|
| jit.opt.start('hotloop=2', 'hotexit=1')
|
| for _, val in ipairs(data) do
|     if val == val then
|         sum = sum + val
|     end
| end
|
| math.fmod(1,1)
|
| print(sum)
| "

This is output before the patch (see `<-- !!!` marks):

| ---- TRACE 1 exit 0
|  000056449701ff26 0000000041bb9880 00000000409cf228 0000000041bb9e94
|  00007fffa700cfa0 00000000409c2378 00000000409cf228 00000000409c3320
|  0000000000000000 0000000000000000 00007fd4e2d38938 00000000fffffffe
|  00007fffa700d3e8 000056446cea132a 00000000409c3098 0000000041bb9ee0
|  +4.6863058149307e-310 +4.6863058151824e-310 +4.6863058151959e-310 +4.6863058152213e-310
|  +4.6863058152444e-310 +4.6863058152873e-310 +4.6863058152729e-310 +4.6863058152444e-310
|  +4.6863058152213e-310                +0 -0.66666666666667  +0.7999999995324
|   -1.1429096284595                +0                +0                +0
| ---- TRACE 1 exit 3 <-- !!!
|  0000000041bb9848 0000000000000006 0000000041bb9828 0000000000000005
|  00007fffa700cfa0 0000000000000006 00000000409cf228 00000000409c3320
|  0000000000000000 0000000000000029 00007fd4e2d3ab28 00000000fffffffe
|  00007fffa700d3e8 000056446cea132a 00000000409c3098 0000000041bb9ee0
|  +4.6863058149307e-310 +4.6863058151824e-310 +4.6863058151959e-310 +4.6863058152213e-310
|                nan              +0.2               nan               nan <-- !!!
|  +4.6863058152213e-310                +0 -0.66666666666667  +0.7999999995324
|   -1.1429096284595                +0                +0                +0
| nan

And this is after:

| ---- TRACE 1 exit 0
|  0000556168ccff06 0000000041f8a880 0000000040275220 0000000041f8ae94
|  00007ffda4a82900 0000000040268378 0000000040275220 0000000040269320
|  0000000000000000 0000000000000000 00007fdd97c11938 00000000fffffffe
|  00007ffda4a82d48 000055613eb5132a 0000000040269090 0000000041f8aee0
|  +4.6380982089931e-310 +4.6380982092477e-310 +4.6380982092617e-310 +4.6380982092839e-310
|  +4.6380982093151e-310 +4.6380982093497e-310 +4.6380982093353e-310 +4.6380982093068e-310
|  +4.6380982092837e-310                +0 -0.66666666666667  +0.7999999995324
|   -1.1429096284595                +0                +0                +0
| ---- TRACE 1 exit 2 <-- !!!
|  0000000041f8a848 0000000000000006 0000000041f8a828 0000000041f8ae94
|  00007ffda4a82900 0000000000000005 0000000040275220 0000000080000000
|  0000000000000000 0000000000000029 00007fdd97c13b28 00000000fffffffe
|  00007ffda4a82d48 000055613eb5132a 0000000040269090 0000000041f8aee0
|  +4.6380982089931e-310 +4.6380982092477e-310 +4.6380982092617e-310 +4.6380982092839e-310
|                nan              +0.2               nan       -2147483648 <-- !!!
|  +4.6380982092837e-310                +0 -0.66666666666667  +0.7999999995324
|   -1.1429096284595                +0                +0                +0
| 0.2

As you can see different side exits are taken, and **this** leads to
different bytecodes on the recording of the side trace (this is observed
in the dump of the bytecode). So, the second side exit doesn't contain
NaN.

The IRs of the first trace are the following:

| ---- TRACE 1 start (command line):7
| ---- TRACE 1 IR
| ....              SNAP   #0   [ ---- ---- ---- ---- ---- ---- ---- ---- ]
| 0001          u8  XLOAD  [0x41b864c1]  V
| 0002          int BAND   0001  +12
| 0003       >  int EQ     0002  +0
| 0004       >  int SLOAD  #7    T
| ....              SNAP   #1   [ ---- ---- ---- ---- ---- ---- ---- ---- ]
| 0005       >  num SLOAD  #2    T
| 0006 xmm7     num CONV   0004  num.int
| 0007 xmm7   + num ADD    0006  0005
| 0008       >  fun SLOAD  #3    T
| 0009 rdx   >  tab SLOAD  #4    T
| 0010 rbp   >  int SLOAD  #5    T
| 0011       >  fun EQ     0008  ipairs_aux
| 0012 rbp    + int ADD    0010  +1
| 0013 rcx      int FLOAD  0009  tab.asize
| 0014       >  int ABC    0013  0012
| 0015 rax      p32 FLOAD  0009  tab.array
| 0016          p32 AREF   0015  0012
| 0017 xmm6  >+ num ALOAD  0016
| ....              SNAP   #2   [ ---- ---- 0007 ---- ---- 0012 0012 0017 ]
| 0018 ------------ LOOP ------------
| 0019          u8  XLOAD  [0x41b864c1]  V
| 0020          int BAND   0019  +12
| 0021       >  int EQ     0020  +0
| 0022 rdi   >  int CONV   0017  int.num
| ....              SNAP   #3   [ ---- ---- 0007 ---- ---- 0012 0012 0017 ]
| 0023 xmm7   + num ADD    0017  0007
| 0024 rbp    + int ADD    0012  +1
| 0025       >  int ABC    0013  0024
| 0026          p32 AREF   0015  0024
| 0027 xmm6  >+ num ALOAD  0026
| 0028 xmm6     num PHI    0017  0027
| 0029 xmm7     num PHI    0007  0023
| 0030 rbp      int PHI    0012  0024
| 0031 rbx      nil RENAME 0012  #3
| 0032 xmm4     nil RENAME 0017  #2
| 0033 xmm5     nil RENAME 0007  #2
| ---- TRACE 1 stop -> loop

The skipped due to DCE conversion is the following:
| 0022 rdi   >  int CONV   0017 int.num

Also, from gdb we can check, that omitted conversion has `IRCONV_CHECK`
mode (I suppose that only check conversion can't be omitted, but Mike
prefer more coarse fix).

Also, it should be mentioned that we get this conversion due to type
instability in loop carried dependency (see `loop_unrool()` in
<src/lj_opt_loop.c>):
| else if (irt_isnum(irr->t) && irt_isinteger(t))  /* Fix num->int. */
|   ref = tref_ref(emitir(IRTGI(IR_CONV), ref,
|                  IRCONV_INT_NUM|IRCONV_CHECK));

All this confusion is because of the too complex traces in the given
example. We have 4 traces, but I suppose that should be reduced to only
one with the necessary side exit to restore NaN value after missing
check.

I suggest to look forward for all places, where such int.num check
conversion is used:

| src/lj_record.c:491:      tr[i] = emitir(IRTGI(IR_CONV), tr[i], IRCONV_INT_NUM|IRCONV_CHECK);
| src/lj_opt_loop.c:349:                              IRCONV_INT_NUM|IRCONV_CHECK));
| src/lj_ffrecord.c:529:  tr = emitir(IRTGI(IR_CONV), tr, IRCONV_INT_NUM|IRCONV_CHECK);
| src/lj_opt_narrow.c:604:      rc = emitir(IRTGI(IR_CONV), rc, IRCONV_INT_NUM|IRCONV_CHECK);

to create a testcase with one trace.

> +--
> +-- Since there is no guard, NaN is added to the sum,
> +-- despite the test case logic.
> +
> +for _, val in ipairs(data) do
> +    if val == val then
> +        sum = sum + val
> +    end
> +end
> +
> +test:ok(sum == sum, 'NaN check was not omitted')
> +
> +os.exit(test:check() and 0 or 1)
> -- 
> 2.39.2 (Apple Git-143)
> 

-- 
Best regards,
Sergey Kaplun