[Tarantool-patches] [PATCH luajit] ARM64: Fix code generation for IR_SLOAD with typecheck + conversion.

[Tarantool-patches] [PATCH luajit] ARM64: Fix code generation for IR_SLOAD with typecheck + conversion.

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

Reported by memcorrupt.

(cherry picked from commit 564147f518af5a5d8985d9e09fc3a768231f4e75)

The assembling of the SLOAD with typecheck and conversion from number to
int misses the corresponding move for emitting conversion to the FPR
during assembling.

Consider the following SLOAD:
| 0006 x28   >  int SLOAD  #4    TCI

Which results in the following mcode before the patch:
|  ldr   x28, [x3, #16]
|  cmp   x2, x28, lsr #32
|  bls   0x62d2fda0        ->0
|                              ; here missing the move to d31
|  fcvtzs w28, d31
|  scvtf d30, w28
|  fcmp  d30, d31
|  bne   0x62d2fda0        ->0

Instead of the expected:
|  ldr   x28, [x3, #16]
|  cmp   x2, x28, lsr #32
|  bls   0x7bacfda0        ->0
|  fmov  d31, x28
|  fcvtzs w28, d31
|  scvtf d30, w28
|  fcmp  d30, d31
|  bne   0x7bacfda0        ->0

Due to the incorrect check of the condition inside the `asm_sload()`,
which excluded the `IRSLOAD_CONVERT` flag. It may lead to inconsistent
behaviour on the trace. This patch fixes the check by comparing the
source and destination registers instead.

Sergey Kaplun:
* added the description and the test for the problem

Part of tarantool/tarantool#11278
---

Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-917-arm64-sload-typecheck-conversion
Related issues:
* https://github.com/LuaJIT/LuaJIT/issues/917
* https://github.com/tarantool/tarantool/issues/11278

 src/lj_asm_arm64.h                            |  2 +-
 ...-arm64-sload-typecheck-conversion.test.lua | 58 +++++++++++++++++++
 2 files changed, 59 insertions(+), 1 deletion(-)
 create mode 100644 test/tarantool-tests/lj-917-arm64-sload-typecheck-conversion.test.lua

diff --git a/src/lj_asm_arm64.h b/src/lj_asm_arm64.h
index 9b27473c..6c7b011f 100644
--- a/src/lj_asm_arm64.h
+++ b/src/lj_asm_arm64.h
@@ -1177,7 +1177,7 @@ dotypecheck:
       tmp = ra_scratch(as, allow);
       rset_clear(allow, tmp);
     }
-    if (ra_hasreg(dest) && irt_isnum(t) && !(ir->op2 & IRSLOAD_CONVERT))
+    if (ra_hasreg(dest) && tmp != dest)
       emit_dn(as, A64I_FMOV_D_R, (dest & 31), tmp);
     /* Need type check, even if the load result is unused. */
     asm_guardcc(as, irt_isnum(t) ? CC_LS : CC_NE);
diff --git a/test/tarantool-tests/lj-917-arm64-sload-typecheck-conversion.test.lua b/test/tarantool-tests/lj-917-arm64-sload-typecheck-conversion.test.lua
new file mode 100644
index 00000000..9cf5cda0
--- /dev/null
+++ b/test/tarantool-tests/lj-917-arm64-sload-typecheck-conversion.test.lua
@@ -0,0 +1,58 @@
+local tap = require('tap')
+-- Test file to demonstrate the incorrect JIT assembling of
+-- `IR_SLOAD` with typecheck and conversion to integer from
+-- number.
+-- See also https://github.com/LuaJIT/LuaJIT/issues/917.
+local test = tap.test('lj-917-arm64-sload-typecheck-conversion'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+test:plan(1)
+
+jit.opt.start('hotloop=1')
+
+local results = {}
+
+-- Use the following mathematics on a huge number not fitting into
+-- an int to be sure that all 3 control numbers (start, stop,
+-- step) of the loop should be non-integers to avoid fallback to
+-- the `lj_vmeta_for()` and narrowing in the `lj_meta_for()`
+-- (see <src/vm_arm64.dasm> for details).
+local NOT_INT = 2 ^ 32
+
+-- The interesting for us SLOAD is the loading of the start index:
+-- | 0006 x28   >  int SLOAD  #4    TCI
+--
+-- Which results in the following mcode before the patch:
+-- |  ldr   x28, [x3, #16]
+-- |  cmp   x2, x28, lsr #32
+-- |  bls   0x62d2fda0        ->0
+-- |                              ; here missing the move to d31
+-- |  fcvtzs w28, d31
+-- |  scvtf d30, w28
+-- |  fcmp  d30, d31
+-- |  bne   0x62d2fda0        ->0
+--
+-- Instead of the expected:
+-- |  ldr   x28, [x3, #16]
+-- |  cmp   x2, x28, lsr #32
+-- |  bls   0x7bacfda0        ->0
+-- |  fmov  d31, x28
+-- |  fcvtzs w28, d31
+-- |  scvtf d30, w28
+-- |  fcmp  d30, d31
+-- |  bne   0x7bacfda0        ->0
+
+-- At this moment d31 contains the value of the `step`, so `step`
+-- should be >= `stop` to obtain inconsistency (the too early loop
+-- end with the last `i` value equals to `step`).
+-- The resulting loop is:
+-- | for i = -4, -1, 1 do
+for i = -4 + NOT_INT * 0, -1 + NOT_INT * 0, 1 + NOT_INT * 0 do
+  results[-i] = true
+end
+
+-- Expected {true, true, true, true}, since -4 is a start.
+test:samevalues(results, 'correct SLOAD TC assembling')
+
+test:done(true)
-- 
2.49.0

Re: [Tarantool-patches] [PATCH luajit] ARM64: Fix code generation for IR_SLOAD with typecheck + conversion.

[Sergey Bronnikov via Tarantool-patches]

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

Sergey,

thanks for the patch! LGTM

On 6/3/25 21:53, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Reported by memcorrupt.
>
> (cherry picked from commit 564147f518af5a5d8985d9e09fc3a768231f4e75)
>
> The assembling of the SLOAD with typecheck and conversion from number to
> int misses the corresponding move for emitting conversion to the FPR
> during assembling.
>
> Consider the following SLOAD:
> | 0006 x28   >  int SLOAD  #4    TCI
>
> Which results in the following mcode before the patch:
> |  ldr   x28, [x3, #16]
> |  cmp   x2, x28, lsr #32
> |  bls   0x62d2fda0        ->0
> |                              ; here missing the move to d31
> |  fcvtzs w28, d31
> |  scvtf d30, w28
> |  fcmp  d30, d31
> |  bne   0x62d2fda0        ->0
>
> Instead of the expected:
> |  ldr   x28, [x3, #16]
> |  cmp   x2, x28, lsr #32
> |  bls   0x7bacfda0        ->0
> |  fmov  d31, x28
> |  fcvtzs w28, d31
> |  scvtf d30, w28
> |  fcmp  d30, d31
> |  bne   0x7bacfda0        ->0
>
> Due to the incorrect check of the condition inside the `asm_sload()`,
> which excluded the `IRSLOAD_CONVERT` flag. It may lead to inconsistent
> behaviour on the trace. This patch fixes the check by comparing the
> source and destination registers instead.
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#11278
> ---
>
> Branch:https://github.com/tarantool/luajit/tree/skaplun/lj-917-arm64-sload-typecheck-conversion
> Related issues:
> *https://github.com/LuaJIT/LuaJIT/issues/917
> *https://github.com/tarantool/tarantool/issues/11278
>
>   src/lj_asm_arm64.h                            |  2 +-
>   ...-arm64-sload-typecheck-conversion.test.lua | 58 +++++++++++++++++++
>   2 files changed, 59 insertions(+), 1 deletion(-)
>   create mode 100644 test/tarantool-tests/lj-917-arm64-sload-typecheck-conversion.test.lua
>
> diff --git a/src/lj_asm_arm64.h b/src/lj_asm_arm64.h
> index 9b27473c..6c7b011f 100644
> --- a/src/lj_asm_arm64.h
> +++ b/src/lj_asm_arm64.h
> @@ -1177,7 +1177,7 @@ dotypecheck:
>         tmp = ra_scratch(as, allow);
>         rset_clear(allow, tmp);
>       }
> -    if (ra_hasreg(dest) && irt_isnum(t) && !(ir->op2 & IRSLOAD_CONVERT))
> +    if (ra_hasreg(dest) && tmp != dest)
>         emit_dn(as, A64I_FMOV_D_R, (dest & 31), tmp);
>       /* Need type check, even if the load result is unused. */
>       asm_guardcc(as, irt_isnum(t) ? CC_LS : CC_NE);
> diff --git a/test/tarantool-tests/lj-917-arm64-sload-typecheck-conversion.test.lua b/test/tarantool-tests/lj-917-arm64-sload-typecheck-conversion.test.lua
> new file mode 100644
> index 00000000..9cf5cda0
> --- /dev/null
> +++ b/test/tarantool-tests/lj-917-arm64-sload-typecheck-conversion.test.lua
> @@ -0,0 +1,58 @@
> +local tap = require('tap')
> +-- Test file to demonstrate the incorrect JIT assembling of
> +-- `IR_SLOAD` with typecheck and conversion to integer from
> +-- number.
> +-- See alsohttps://github.com/LuaJIT/LuaJIT/issues/917.
> +local test = tap.test('lj-917-arm64-sload-typecheck-conversion'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +test:plan(1)
> +
> +jit.opt.start('hotloop=1')
> +
> +local results = {}
> +
> +-- Use the following mathematics on a huge number not fitting into
> +-- an int to be sure that all 3 control numbers (start, stop,
> +-- step) of the loop should be non-integers to avoid fallback to
> +-- the `lj_vmeta_for()` and narrowing in the `lj_meta_for()`
> +-- (see <src/vm_arm64.dasm> for details).
> +local NOT_INT = 2 ^ 32
> +
> +-- The interesting for us SLOAD is the loading of the start index:
> +-- | 0006 x28   >  int SLOAD  #4    TCI
> +--
> +-- Which results in the following mcode before the patch:
> +-- |  ldr   x28, [x3, #16]
> +-- |  cmp   x2, x28, lsr #32
> +-- |  bls   0x62d2fda0        ->0
> +-- |                              ; here missing the move to d31
> +-- |  fcvtzs w28, d31
> +-- |  scvtf d30, w28
> +-- |  fcmp  d30, d31
> +-- |  bne   0x62d2fda0        ->0
> +--
> +-- Instead of the expected:
> +-- |  ldr   x28, [x3, #16]
> +-- |  cmp   x2, x28, lsr #32
> +-- |  bls   0x7bacfda0        ->0
> +-- |  fmov  d31, x28
> +-- |  fcvtzs w28, d31
> +-- |  scvtf d30, w28
> +-- |  fcmp  d30, d31
> +-- |  bne   0x7bacfda0        ->0
> +
> +-- At this moment d31 contains the value of the `step`, so `step`
> +-- should be >= `stop` to obtain inconsistency (the too early loop
> +-- end with the last `i` value equals to `step`).
> +-- The resulting loop is:
> +-- | for i = -4, -1, 1 do
> +for i = -4 + NOT_INT * 0, -1 + NOT_INT * 0, 1 + NOT_INT * 0 do
> +  results[-i] = true
> +end
> +
> +-- Expected {true, true, true, true}, since -4 is a start.
> +test:samevalues(results, 'correct SLOAD TC assembling')
> +
> +test:done(true)

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