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

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

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 <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/

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?

| 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

<snipped>

> > 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/

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 also https://github.com/LuaJIT/LuaJIT/issue/903.
+-- See also https://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(),
 })
===================================================================

<snipped>

-- 
Best regards,
Sergey Kaplun