Hi, Sergey! LGTM, thanks! On 5/20/25 15:40, Sergey Kaplun wrote: > Hi, Sergey! > Thanks for the review! > See, my answers below. > > On 16.05.25, Sergey Bronnikov wrote: >> Hello, Sergey, >> >> thanks for the patch! Please see my comments. >> >> Sergey >> >> On 5/14/25 14:56, Sergey Kaplun wrote: >>> From: Mike Pall >>> >>> 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/ > Fixed, thanks. > >>> 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? > None of them from your mcode dump, since it is for the fixed version. > But here we can see the d0 initialization (by the fmov too). > >> 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); > Yes, this is referenced (already fixed) part of the code . > >> 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. > I've updated the commit message like the following, does it become more > clear? Yes, thanks! > > | ARM64: Fix IR_SLOAD assembly. > | > | Reported by Gate88. > | > | (cherry picked from commit 6c4826f12c4d33b8b978004bc681eb1eef2be977) > | > | The issue is in the case when IR SLOAD is unused on a trace, it persists > | only for typecheck, and has the `num` type. In this case, the `dest` > | register is `RID_NONE`. Hence, the `fmov` instruction is emitted > | 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. > | > | So the emitted assembly for the aforementioned SLOAD looks like the > | following: > | | ldr x27, [x7, #24] > | | cmp x0, x27, lsr #32 > | | bls 0x67d7f4f0 ->0 > | | fmov d0, x27 ; this spoils d0 > | > | Instead of the following: > | | ldr x27, [x7, #24] > | | cmp x0, x27, lsr #32 > | | bls 0x6a91f4e8 ->0 > | > | 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 > >>> 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 > > >>> 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. >>> +-- Seealsohttps://github.com/LuaJIT/LuaJIT/issue/903. >> typo:  s/issue/issues/ > Fixed, thanks! > > =================================================================== > 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.lu > a > index 748b88e2..1a0531f0 100644 > --- 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 > @@ -1,7 +1,7 @@ > 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. > +-- See alsohttps://github.com/LuaJIT/LuaJIT/issues/903. > local test = tap.test('lj-903-arm64-unused-number-sload-typecheck'):skipcond({ > ['Test requires JIT enabled'] = not jit.status(), > }) > =================================================================== > > >