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

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

[-- 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 --]