From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 1C3699E985B; Tue, 20 May 2025 17:01:39 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 1C3699E985B DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1747749699; bh=OolMVsQ09kEH8abyc2YgziHKW7BKaQUwN5Slha9zZqs=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=VLjilOOfjPyEUF8cBHSm2dGx43gO0qPmnQy0WS0zY2ykxBcuZEHpZoLusbQIjdpwK FljVU//O/ZkG7qfsUfPImmnCh2RQ+LPrahMsNZUwY2iGDDUjrukQodde0E0EhqYIu6 59N/rjtCA/LHRg1Yv4KQCy4PMzphWs20iZvQQAWI= Received: from send105.i.mail.ru (send105.i.mail.ru [89.221.237.200]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 2207C1411617 for ; Tue, 20 May 2025 17:01:38 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 2207C1411617 Received: by exim-smtp-7b66877447-tfltx with esmtpa (envelope-from ) id 1uHNXF-0000000072p-0kVj; Tue, 20 May 2025 17:01:37 +0300 Content-Type: multipart/alternative; boundary="------------00LKhwuuna0I2rOwLGuXT0kj" Message-ID: <60ea06b7-d538-45f8-8f37-6d34b73ba907@tarantool.org> Date: Tue, 20 May 2025 17:01:35 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org References: <20250514115656.13243-1-skaplun@tarantool.org> <91a4423c-9968-4176-b4dd-4f5045dc7ee4@tarantool.org> Content-Language: en-US In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD93761F2630DFFAF41DADE37F0BB826AF728399E8040BAFFF2182A05F5380850404C228DA9ACA6FE27B23B7747FFBF196D3DE06ABAFEAF67054B395EDC37B34416C0FE0E5B1233B1EED5A0AF8C34EAAAE4 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7E5D7EAC6EBA58433EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637AC83A81C8FD4AD23D82A6BABE6F325AC2E85FA5F3EDFCBAA7353EFBB553375661B62E235E840615C7FFA218B63F254545F9C13D56907158054DC921916280D32389733CBF5DBD5E913377AFFFEAFD269176DF2183F8FC7C07E7E81EEA8A9722B8941B15DA834481FCF19DD082D7633A0EF3E4896CB9E6436389733CBF5DBD5E9D5E8D9A59859A8B652D31B9D28593E51CC7F00164DA146DA6F5DAA56C3B73B237318B6A418E8EAB8D32BA5DBAC0009BE9E8FC8737B5C2249CB3CB8E9EF962DC476E601842F6C81A12EF20D2F80756B5FB606B96278B59C4276E601842F6C81A127C277FBC8AE2E8BE270C32E94023BD73AA81AA40904B5D99C9F4D5AE37F343AD1F44FA8B9022EA23BBE47FD9DD3FB595F5C1EE8F4F765FC72CEEB2601E22B093A03B725D353964B0B7D0EA88DDEDAC722CA9DD8327EE4930A3850AC1BE2E73579C543ECCDAE434EC4224003CC83647689D4C264860C145E X-C1DE0DAB: 0D63561A33F958A515B82DE3C01E35BC5002B1117B3ED6962B1137598DE7D861108A05421C070DB8823CB91A9FED034534781492E4B8EEADAE4FDBF11360AC9BBDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D3468964757141B82E04FCD61874183EADFFD50D2B598F95AD79E040839F49A82D972B12CCA05D9D2D91D7E09C32AA3244CB600454D4E9521C677DD89D51EBB77429F99E9433555D92FEA455F16B58544A2E30DDF7C44BCB90DA5AE236DF995FB59978A700BF655EAEEED6A17656DB59BCAD427812AF56FC65B X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu53w8ahmwBjZKM/YPHZyZHvz5uv+WouB9+ObcCpyrx6l7KImUglyhkEat/+ysWwi0gdhEs0JGjl6ggRWTy1haxBpVdbIX1nthFXMZebaIdHP2ghjoIc/363UZI6Kf1ptIMVT0GoR8ZYDe70rDQyZ4/73w= X-Mailru-Sender: C4F68CFF4024C8867DFDF7C7F25884585332F4030E4A2356504BCEC120683F15C1A112668DD64576C1B2F903A207F322645D15D82EE4B272BD6E4642A116CA93524AA66B5ACBE6721EF430B9A63E2A504198E0F3ECE9B5443453F38A29522196 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] ARM64: Fix IR_SLOAD assembly. X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" This is a multi-part message in MIME format. --------------00LKhwuuna0I2rOwLGuXT0kj Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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(), > }) > =================================================================== > > > --------------00LKhwuuna0I2rOwLGuXT0kj Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

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.
+-- 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>



    

  
  


--------------00LKhwuuna0I2rOwLGuXT0kj--