Re: [Tarantool-patches] [PATCH luajit] ARM64: Fix IR_SLOAD assembly.

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

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

Hello, Sergey,

thanks for the patch! Please see my comments.

Sergey

On 5/14/25 14:56, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Reported by Gate88.
>
> (cherry picked from commit 6c4826f12c4d33b8b978004bc681eb1eef2be977)
>
> The issue is in the case when IR SLOAD is unused on a trace, persists
typo: s/, /, it/
> only for typecheck, and has the `num` type. In this case, the `dest`
> register is `RID_NONE`. Hence, the `fmov` instruction is emitted

there are two instructions `fmov` in the generated assembler output, 
which one do you mean?

55690afedc  fmov  d0, x30
55690afee0  ldr   w28, [x19, #8]
55690afee4  ldur  x3, [x19, #-16]
55690afee8  and   x3, x3, #0x7fffffffffff
55690afeec  ldr   x27, [x3, #48]
55690afef0  ldr   x8, [x27, #32]
55690afef4  sub   x9, x8, x19
55690afef8  cmp   x9, #56
55690afefc  bls   0x690affe8    ->0
55690aff00  ldr   x6, [x8]
55690aff04  asr   x27, x6, #47
55690aff08  cmn   x27, #12
55690aff0c  bne   0x690affe8    ->0
55690aff10  and   x6, x6, #0x7fffffffffff
55690aff14  ldr   w7, [x6, #52]
55690aff18  cmp   w7, #1
55690aff1c  bne   0x690affe8    ->0
55690aff20  ldr   x5, [x6, #40]
55690aff24  ldr   x27, [x5, #32]
55690aff28  cmp   x27, x1
55690aff2c  bne   0x690affe8    ->0
55690aff30  ldr   x27, [x5, #24]
55690aff34  cmp   x0, x27, lsr #32
55690aff38  bls   0x690affe8    ->0
55690aff3c  fmov  d2, x27

or (probably) you just refer to a C code of `asm_sload`, that emits `fmov`:

   1180     if (ra_hasreg(dest) && irt_isnum(t) && !(ir->op2 & 
IRSLOAD_CONVERT))
   1181       emit_dn(as, A64I_FMOV_D_R, (dest & 31), tmp);

Sorry, it is not easy for me to match IR and ASM, so I believe more 
details is required in the description.

I just want to understand a difference for emitted assembler before and 
after the patch.

> unconditionally, where the destination register is `d0` (`RID_NONE &
> 31`). So, the value of this register is spoiled. If it holds any value
> evaluated before and used after this SLOAD, it leads to incorrect
> behaviour.
>
> This patch adds the check that the register is in use before emitting
> the instruction.
>
> 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-903-arm64-unused-number-sload-typecheck
> Related issues:
> *https://github.com/LuaJIT/LuaJIT/issues/903
> *https://github.com/tarantool/tarantool/issues/11278
>
>   src/lj_asm_arm64.h                            |  2 +-
>   ...m64-unused-number-sload-typecheck.test.lua | 45 +++++++++++++++++++
>   2 files changed, 46 insertions(+), 1 deletion(-)
>   create mode 100644 test/tarantool-tests/lj-903-arm64-unused-number-sload-typecheck.test.lua
>
> diff --git a/src/lj_asm_arm64.h b/src/lj_asm_arm64.h
> index 554bb60a..9b27473c 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 (irt_isnum(t) && !(ir->op2 & IRSLOAD_CONVERT))
> +    if (ra_hasreg(dest) && irt_isnum(t) && !(ir->op2 & IRSLOAD_CONVERT))
>         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-903-arm64-unused-number-sload-typecheck.test.lua b/test/tarantool-tests/lj-903-arm64-unused-number-sload-typecheck.test.lua
> new file mode 100644
> index 00000000..748b88e2
> --- /dev/null
> +++ b/test/tarantool-tests/lj-903-arm64-unused-number-sload-typecheck.test.lua
> @@ -0,0 +1,45 @@
> +local tap = require('tap')
> +-- Test file to demonstrate the incorrect JIT assembling of unused
> +-- `IR_SLOAD` with number type on arm64.
> +-- See alsohttps://github.com/LuaJIT/LuaJIT/issue/903.
typo:  s/issue/issues/
> +local test = tap.test('lj-903-arm64-unused-number-sload-typecheck'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +test:plan(1)
> +
> +-- Just use any different numbers (but not integers to avoid
> +-- integer IR type).
> +local SLOT = 0.1
> +local MARKER_VALUE = 4.2
> +-- XXX: Special mapping to avoid folding and removing always true
> +-- comparison.
> +local anchor = {marker = MARKER_VALUE}
> +
> +-- Special function to inline on trace to generate SLOAD
> +-- typecheck.
> +local function sload_unused(x)
> +  return x
> +end
> +
> +-- The additional wrapper to use stackslots in the function.
> +local function test_sload()
> +  local sload = SLOT
> +  for _ = 1, 4 do
> +    -- This line should use the `d0` register.
> +    local marker = anchor.marker - MARKER_VALUE
> +    -- This generates unused IR_SLOAD with typecheck (number).
> +    -- Before the patch, it occasionally overwrites the `d0`
> +    -- register and causes the execution of the branch.
> +    sload_unused(sload)
> +    if marker ~= 0 then
> +      return false
> +    end
> +  end
> +  return true
> +end
> +
> +jit.opt.start('hotloop=1')
> +test:ok(test_sload(), 'correct SLOAD assembling')
> +
> +test:done(true)

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