Re: [Tarantool-patches] [PATCH luajit 3/3] DUALNUM: Improve/fix edge cases of unary minus.

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

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

Hi, Sergey,

thanks for the patch! LGTM

Sergey

On 3/2/26 10:52, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Thanks to Sergey Kaplun.
>
> (cherry picked from commit 707c12bf00dafdfd3899b1a6c36435dbbf6c7022)
>
> In a single-number build (default for x86/x64), `-(0)` is always `-0`
> due to its number type. However, in DUALNUM mode, the `0` may have the
> integer type, and for this integer, `BC_UNM` has no effect. `0` may be
> represented via number or integer type, where the resulting `0` is
> constructed during initial parsing (`lj_strscan_scan()`) or arithmetic
> operations (`BC_MODVV`). This inconsistency may break the JIT semantics
> and lead to the assertion failure in the `lj_check_slots()`.
>
> This patch fixes the incorrect `BC_UNM` for integer operand 0. Also, the
> narrowing optimization for the unary minus should be more strict to
> avoid incorrect narrowing for integer slots and make JIT intact with the
> VM semantics.
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#12134
> ---
>   src/lj_opt_narrow.c                           |  5 +-
>   src/vm_arm.dasc                               |  5 +-
>   src/vm_arm64.dasc                             |  2 +
>   src/vm_mips.dasc                              |  6 ++-
>   src/vm_mips64.dasc                            |  6 ++-
>   src/vm_ppc.dasc                               |  7 ++-
>   src/vm_x64.dasc                               |  4 ++
>   src/vm_x86.dasc                               |  5 ++
>   .../lj-1422-incorrect-unm-for-mzero.test.lua  | 54 +++++++++++++++++++
>   .../tarantool-tests/lj-1422-unm-zero.test.lua | 12 +++++
>   10 files changed, 99 insertions(+), 7 deletions(-)
>   create mode 100644 test/tarantool-tests/lj-1422-incorrect-unm-for-mzero.test.lua
>   create mode 100644 test/tarantool-tests/lj-1422-unm-zero.test.lua
>
> diff --git a/src/lj_opt_narrow.c b/src/lj_opt_narrow.c
> index 6e3e9533..3bf7320d 100644
> --- a/src/lj_opt_narrow.c
> +++ b/src/lj_opt_narrow.c
> @@ -553,10 +553,9 @@ TRef lj_opt_narrow_unm(jit_State *J, TRef rc, TValue *vc)
>     rc = conv_str_tonum(J, rc, vc);
>     if (tref_isinteger(rc)) {
>       uint32_t k = (uint32_t)numberVint(vc);
> -    if ((tvisint(vc) || k != 0) && k != 0x80000000u) {
> +    if (k != 0 && k != 0x80000000u) {
>         TRef zero = lj_ir_kint(J, 0);
> -      if (!tvisint(vc))
> -	emitir(IRTGI(IR_NE), rc, zero);
> +      emitir(IRTGI(IR_NE), rc, zero);
>         return emitir(IRTGI(IR_SUBOV), zero, rc);
>       }
>       rc = emitir(IRTN(IR_CONV), rc, IRCONV_NUM_INT);
> diff --git a/src/vm_arm.dasc b/src/vm_arm.dasc
> index 7095e660..785d13ac 100644
> --- a/src/vm_arm.dasc
> +++ b/src/vm_arm.dasc
> @@ -2930,13 +2930,16 @@ static void build_ins(BuildCtx *ctx, BCOp op, int defop)
>       |  bhi ->vmeta_unm
>       |  eorne CARG2, CARG2, #0x80000000
>       |  bne >5
> -    |  rsbseq CARG1, CARG1, #0
> +    |  rsbs CARG1, CARG1, #0
> +    |  ldrdeq CARG12, >8
>       |  ldrdvs CARG12, >9
>       |5:
>       |  strd CARG12, [BASE, RA]
>       |   ins_next3
>       |
>       |.align 8
> +    |8:
> +    |  .long 0x00000000, 0x80000000	// -0.
>       |9:
>       |  .long 0x00000000, 0x41e00000	// 2^31.
>       break;
> diff --git a/src/vm_arm64.dasc b/src/vm_arm64.dasc
> index 6600e226..c64b736b 100644
> --- a/src/vm_arm64.dasc
> +++ b/src/vm_arm64.dasc
> @@ -2481,6 +2481,8 @@ static void build_ins(BuildCtx *ctx, BCOp op, int defop)
>       |   movz CARG3, #0x41e0, lsl #48	// 2^31.
>       |   add TMP0, TMP0, TISNUM
>       |  csel TMP0, TMP0, CARG3, vc
> +    |   movz CARG3, #0x8000, lsl #48	// -0.
> +    |  csel TMP0, TMP0, CARG3, ne
>       |5:
>       |  str TMP0, [BASE, RA, lsl #3]
>       |  ins_next
> diff --git a/src/vm_mips.dasc b/src/vm_mips.dasc
> index 32caabf7..dcb04ed8 100644
> --- a/src/vm_mips.dasc
> +++ b/src/vm_mips.dasc
> @@ -3319,7 +3319,8 @@ static void build_ins(BuildCtx *ctx, BCOp op, int defop)
>       |   addu RA, BASE, RA
>       |  bne SFARG1HI, TISNUM, >2
>       |.  lw SFARG1LO, LO(RB)
> -    |  lui TMP1, 0x8000
> +    |  beqz SFARG1LO, >3
> +    |.  lui TMP1, 0x8000
>       |  beq SFARG1LO, TMP1, ->vmeta_unm	// Meta handler deals with -2^31.
>       |.  negu SFARG1LO, SFARG1LO
>       |1:
> @@ -3333,6 +3334,9 @@ static void build_ins(BuildCtx *ctx, BCOp op, int defop)
>       |.  lui TMP1, 0x8000
>       |  b <1
>       |.  xor SFARG1HI, SFARG1HI, TMP1
> +    |3:
> +    |  b <1
> +    |.  lui SFARG1HI, 0x8000	// -0.
>       break;
>     case BC_LEN:
>       |  // RA = dst*8, RD = src*8
> diff --git a/src/vm_mips64.dasc b/src/vm_mips64.dasc
> index 44fba36c..34da6473 100644
> --- a/src/vm_mips64.dasc
> +++ b/src/vm_mips64.dasc
> @@ -3563,7 +3563,8 @@ static void build_ins(BuildCtx *ctx, BCOp op, int defop)
>       |  sextw CARG1, CARG1
>       |  beq CARG1, TMP1, ->vmeta_unm	// Meta handler deals with -2^31.
>       |.  negu CARG1, CARG1
> -    |  zextw CARG1, CARG1
> +    |  beqz CARG1, >3
> +    |.  zextw CARG1, CARG1
>       |  settp CARG1, TISNUM
>       |1:
>       |  ins_next1
> @@ -3575,6 +3576,9 @@ static void build_ins(BuildCtx *ctx, BCOp op, int defop)
>       |.  dsll TMP1, TMP1, 32
>       |  b <1
>       |.  xor CARG1, CARG1, TMP1
> +    |3:
> +    |  b <1
> +    |.  dsll CARG1, TMP1, 32
>       break;
>     case BC_LEN:
>       |  // RA = dst*8, RD = src*8
> diff --git a/src/vm_ppc.dasc b/src/vm_ppc.dasc
> index 980ad897..88fa7b67 100644
> --- a/src/vm_ppc.dasc
> +++ b/src/vm_ppc.dasc
> @@ -3803,11 +3803,13 @@ static void build_ins(BuildCtx *ctx, BCOp op, int defop)
>       |  bne >5
>       |.if GPR64
>       |  lus TMP2, 0x8000
> -    |  neg TMP0, TMP0
> +    |  neg. TMP0, TMP0
> +    |  beq >8
>       |  cmplw TMP0, TMP2
>       |  beq >4
>       |.else
>       |  nego. TMP0, TMP0
> +    |  beq >8
>       |  bso >4
>       |1:
>       |.endif
> @@ -3834,6 +3836,9 @@ static void build_ins(BuildCtx *ctx, BCOp op, int defop)
>       |   stw TMP0, 4(RA)
>       |.if DUALNUM
>       |  b <3
> +    |8:
> +    |  lus TMP1, 0x8000			// -0.
> +    |  b <7
>       |.else
>       |  ins_next2
>       |.endif
> diff --git a/src/vm_x64.dasc b/src/vm_x64.dasc
> index 8b6781a6..ee669887 100644
> --- a/src/vm_x64.dasc
> +++ b/src/vm_x64.dasc
> @@ -3179,11 +3179,15 @@ static void build_ins(BuildCtx *ctx, BCOp op, int defop)
>       |.if DUALNUM
>       |  checkint RB, >5
>       |  neg RBd
> +    |  jz >3
>       |  jo >4
>       |  setint RB
>       |9:
>       |  mov [BASE+RA*8], RB
>       |  ins_next
> +    |3:
> +    |  mov64 RB, U64x(80000000,00000000)  // -0.
> +    |  jmp <9
>       |4:
>       |  mov64 RB, U64x(41e00000,00000000)  // 2^31.
>       |  jmp <9
> diff --git a/src/vm_x86.dasc b/src/vm_x86.dasc
> index 7c11c78e..027bdcd6 100644
> --- a/src/vm_x86.dasc
> +++ b/src/vm_x86.dasc
> @@ -3780,11 +3780,16 @@ static void build_ins(BuildCtx *ctx, BCOp op, int defop)
>       |  checkint RD, >5
>       |  mov RB, [BASE+RD*8]
>       |  neg RB
> +    |  jz >3
>       |  jo >4
>       |  mov dword [BASE+RA*8+4], LJ_TISNUM
> +    |8:
>       |  mov dword [BASE+RA*8], RB
>       |9:
>       |  ins_next
> +    |3:
> +    |  mov dword [BASE+RA*8+4], 0x80000000  // -0.
> +    |  jmp <8
>       |4:
>       |  mov dword [BASE+RA*8+4], 0x41e00000  // 2^31.
>       |  mov dword [BASE+RA*8], 0
> diff --git a/test/tarantool-tests/lj-1422-incorrect-unm-for-mzero.test.lua b/test/tarantool-tests/lj-1422-incorrect-unm-for-mzero.test.lua
> new file mode 100644
> index 00000000..7046784c
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1422-incorrect-unm-for-mzero.test.lua
> @@ -0,0 +1,54 @@
> +local tap = require('tap')
> +
> +-- This test demonstrates LuaJIT's inconsistencies in the VM and
> +-- the JIT engine in the DUALNUM mode for 0.
> +-- See alsohttps://github.com/LuaJIT/LuaJIT/issues/1422.
> +
> +local test = tap.test('lj-1422-incorrect-unm-for-mzero'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +test:plan(3)
> +
> +local tonumber = tonumber
> +local tostring = tostring
> +
> +local function always_number(val)
> +  return tonumber(val) or 1
> +end
> +
> +-- Yielded int type for non-x86 arches.
> +local function modvn(v1)
> +  return always_number(v1) % 1
> +end
> +
> +local function unm(v)
> +  return -v
> +end
> +
> +jit.opt.start('hotloop=1', 'hotexit=1')
> +
> +always_number(nil) -- Root trace.
> +always_number(nil)
> +
> +local stack_slot = nil
> +for _ = 1, 5 do
> +  always_number(0) -- Compile side trace.
> +  -- The side trace crashes in the `rec_check_slots()` for non-x86
> +  -- arches before the patch.
> +  stack_slot = tostring(unm(modvn(stack_slot)))
> +end
> +
> +test:is(stack_slot, '-0', 'correct result of the trace execution')
> +
> +-- `tonumber()` recording and conversion to number.
> +local results = {nil, nil, nil, nil}
> +for i = 1, 4 do
> +  local slot = -tonumber('0')
> +  results[i] = tostring(slot)
> +end
> +
> +test:is(results[1], '-0', 'correct result of the expression')
> +test:samevalues(results, 'correct result of the tonumber recording')
> +
> +test:done(true)
> diff --git a/test/tarantool-tests/lj-1422-unm-zero.test.lua b/test/tarantool-tests/lj-1422-unm-zero.test.lua
> new file mode 100644
> index 00000000..77062418
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1422-unm-zero.test.lua
> @@ -0,0 +1,12 @@
> +local tap = require('tap')
> +
> +-- The test file to demonstrate LuaJIT's incorrect BC_UNM for the
> +-- 0 operand for the DUALNUM mode.
> +-- See alsohttps://github.com/LuaJIT/LuaJIT/issues/1422.
> +
> +local test = tap.test('lj-1422-unm-zero')
> +test:plan(1)
> +
> +test:ok(tostring(-0) == '-0', 'correct unary minus for 0')
> +
> +test:done(true)

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