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: 6381 bytes --]

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 <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?
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
> <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.
>>> +-- 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(),
>   })
> ===================================================================
>
> <snipped>
>

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