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 4255D6D1544; Tue, 7 Nov 2023 12:44:14 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 4255D6D1544 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1699350254; bh=pbjQIGiLQaSH2Tdjhg2K5XymTnGeyvg++RSjKY8msnQ=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=s2Di3Wp2LtyuL3qkiENZDXOJ8aAY5ZpD43juMVirDHAy+klN+Kh+xRERI8De+nlIX H8nfGmyBSAgPpirA5lPpT+EvsoqkH0vDwLK3XJJKfH6EPbFUgDOmVO6G2HVBWchQ1v v/xx/9txkFxvz2fB5wjDxzmHvjCAEZK6X3nuGtno= Received: from smtp30.i.mail.ru (smtp30.i.mail.ru [95.163.41.71]) (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 2A85169961B for ; Tue, 7 Nov 2023 12:44:13 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 2A85169961B Received: by smtp30.i.mail.ru with esmtpa (envelope-from ) id 1r0Id1-00GEWR-1s; Tue, 07 Nov 2023 12:44:12 +0300 Date: Tue, 7 Nov 2023 12:44:11 +0300 To: Sergey Kaplun Message-ID: References: <20231102123616.10527-1-skaplun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20231102123616.10527-1-skaplun@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9DD83A8ACFE75317516FDD8D1EA696B137287CA10F5C4870E00894C459B0CD1B905CCA356A48245DC42731F0A919FAD4B6CF10209CFF08801A3221720B8975CE9 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7AC43A7B4C36B769DEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063737452AF4BFD067BF8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D87DD214D50F3A980A408528DCFE35131F117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC3733B5EC72352B9FA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F44604297287769387670735201E561CDFBCA1751F2CC0D3CB04F14752D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EEB28585415E75ADA9040F9FF01DFDA4A84AD6D5ED66289B523666184CF4C3C14F6136E347CC761E07725E5C173C3A84C30942DC5495D0595E76E601842F6C81A1F004C906525384303E02D724532EE2C3F43C7A68FF6260569E8FC8737B5C2249957A4DEDD2346B42E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B613439FA09F3DCB32089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A5C830FC33EB3AAF8430801248BBD0B241F03BE6AF0DAC99A5F87CCE6106E1FC07E67D4AC08A07B9B0CE135D2742255B35CB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CFF4E876CE1FE2BB595F7CCC3C7443EC0D42F29F8D4275A305F87DE84224A59447508B3DC588E9D1C72B33B16BC67043E0F629FE623C09AE0EDB2546E6A87E32C0E48CAC7CA610320002C26D483E81D6BE64ACE4A408B72B61B0CA6F94E606A667A52EF62A646584F811BD90D3D42C882D43082AE146A756F3 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojoiosMWyUVRdlbpca0yvfOQ== X-Mailru-Sender: 7940E2A4EB16C9974D1995FACA6D044E05CCA356A48245DC42731F0A919FAD4BE2527C969975515CFF9FCECFB8D89CB6C77752E0C033A69E235A20A81F3B0E39AB3C5F247CB2F7F93A5DB60FBEB33A8A0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] x86/x64: Fix math.ceil(-0.9) result sign. 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: Maxim Kokryashkin via Tarantool-patches Reply-To: Maxim Kokryashkin Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Sergey! Thanks for the patch! LGTM, except for a few nits below. On Thu, Nov 02, 2023 at 03:36:16PM +0300, Sergey Kaplun wrote: > From: Mike Pall > > Reported by minoki. > > (cherry-picked from commit 674afcd4e21d0cf64de3219d347557a0aed8ecc7) > > The `ceil` (`floor`) math function implementation calculates (|x| + > 2^52) - 2^52 for its argument to determine the fractional part of x , so Typo: s/x ,/x,/ > it will be rounded to the nearest integer and its sign is restored. > After that, if the original value is < (>) than the result, the -1 (1) > is subtracted from it. Take a look at the `ceil()` case. The result of > the operation `-1 - (-1)` is +0 for FP arithmetic, against -0 expected > as a result. > > This patch changes the `- (-1)` operation to `+ 1` and restores sign > after it again. > > NB: Since in DUALNUM mode on x86/x64 all results are tried to be > converted to integers the sign of 0 is neglected. Typo: s/integers/integers,/ > > Sergey Kaplun: > * added the description and the test for the problem > > Part of tarantool/tarantool#9145 > --- > > Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-859-math-ceil-sign > Tarantool PR: https://github.com/tarantool/tarantool/pull/9326 > Related issues: > * https://github.com/LuaJIT/LuaJIT/issues/859 > * https://github.com/tarantool/tarantool/issues/9145 > > src/vm_x64.dasc | 13 ++++++------ > src/vm_x86.dasc | 13 ++++++------ > .../lj-859-math-ceil-sign.test.lua | 20 +++++++++++++++++++ > 3 files changed, 32 insertions(+), 14 deletions(-) > create mode 100644 test/tarantool-tests/lj-859-math-ceil-sign.test.lua > > diff --git a/src/vm_x64.dasc b/src/vm_x64.dasc > index 399dfcbf..fc4c6259 100644 > --- a/src/vm_x64.dasc > +++ b/src/vm_x64.dasc > @@ -400,9 +400,6 @@ > |.macro sseconst_1, reg, tmp // Synthesize 1.0. > | sseconst_hi reg, tmp, 3ff00000 > |.endmacro > -|.macro sseconst_m1, reg, tmp // Synthesize -1.0. > -| sseconst_hi reg, tmp, bff00000 > -|.endmacro > |.macro sseconst_2p52, reg, tmp // Synthesize 2^52. > | sseconst_hi reg, tmp, 43300000 > |.endmacro > @@ -2598,15 +2595,17 @@ static void build_subroutines(BuildCtx *ctx) > | addsd xmm1, xmm3 // (|x| + 2^52) - 2^52 > | subsd xmm1, xmm3 > | orpd xmm1, xmm2 // Merge sign bit back in. > + | sseconst_1 xmm3, RD > | .if mode == 1 // ceil(x)? > - | sseconst_m1 xmm2, RD // Must subtract -1 to preserve -0. > | cmpsd xmm0, xmm1, 6 // x > result? > + | andpd xmm0, xmm3 > + | addsd xmm1, xmm0 // If yes, add 1. > + | orpd xmm1, xmm2 // Merge sign bit back in (again). > | .else // floor(x)? > - | sseconst_1 xmm2, RD > | cmpsd xmm0, xmm1, 1 // x < result? > + | andpd xmm0, xmm3 > + | subsd xmm1, xmm0 // If yes, subtract 1. > | .endif > - | andpd xmm0, xmm2 > - | subsd xmm1, xmm0 // If yes, subtract +-1. > |.endif > | movaps xmm0, xmm1 > |1: > diff --git a/src/vm_x86.dasc b/src/vm_x86.dasc > index 9fa9a3f7..05a8267b 100644 > --- a/src/vm_x86.dasc > +++ b/src/vm_x86.dasc > @@ -533,9 +533,6 @@ > |.macro sseconst_1, reg, tmp // Synthesize 1.0. > | sseconst_hi reg, tmp, 3ff00000 > |.endmacro > -|.macro sseconst_m1, reg, tmp // Synthesize -1.0. > -| sseconst_hi reg, tmp, bff00000 > -|.endmacro > |.macro sseconst_2p52, reg, tmp // Synthesize 2^52. > | sseconst_hi reg, tmp, 43300000 > |.endmacro > @@ -3089,15 +3086,17 @@ static void build_subroutines(BuildCtx *ctx) > | addsd xmm1, xmm3 // (|x| + 2^52) - 2^52 > | subsd xmm1, xmm3 > | orpd xmm1, xmm2 // Merge sign bit back in. > + | sseconst_1 xmm3, RDa > | .if mode == 1 // ceil(x)? > - | sseconst_m1 xmm2, RDa // Must subtract -1 to preserve -0. > | cmpsd xmm0, xmm1, 6 // x > result? > + | andpd xmm0, xmm3 > + | addsd xmm1, xmm0 // If yes, add 1. > + | orpd xmm1, xmm2 // Merge sign bit back in (again). > | .else // floor(x)? > - | sseconst_1 xmm2, RDa > | cmpsd xmm0, xmm1, 1 // x < result? > + | andpd xmm0, xmm3 > + | subsd xmm1, xmm0 // If yes, subtract 1. > | .endif > - | andpd xmm0, xmm2 > - | subsd xmm1, xmm0 // If yes, subtract +-1. > |.endif > | movaps xmm0, xmm1 > |1: > diff --git a/test/tarantool-tests/lj-859-math-ceil-sign.test.lua b/test/tarantool-tests/lj-859-math-ceil-sign.test.lua > new file mode 100644 > index 00000000..df0ca329 > --- /dev/null > +++ b/test/tarantool-tests/lj-859-math-ceil-sign.test.lua > @@ -0,0 +1,20 @@ > +local tap = require('tap') > + > +-- Test file to demonstrate the incorrect LuaJIT's behaviour > +-- for `math.ceil(x)` when argument `x`: -1 < x < -0.5. > +-- See also https://github.com/LuaJIT/LuaJIT/issues/859. > + > +local test = tap.test('lj-859-math-ceil-sign') > + > +test:plan(1) > + > +local IS_DUALNUM = tostring(tonumber('-0')) ~= tostring(-0) > +local IS_X86_64 = jit.arch == 'x86' or jit.arch == 'x64' > + > +-- Use `tostirng()` to compare sign of the returned value. Typo: s/sign/the sign/ > +-- Take any value from the mentioned range. Chosen one is Typo: s/Chosen/The chosen/ > +-- mentioned in the commit message. > +test:ok((IS_DUALNUM and IS_X86_64) or tostring(math.ceil(-0.9)) == '-0', Nit: maybe we should precalculate the predicate before the `test:ok` call? Or, at least the `IS_DUALNUM and IS_X86_64`, as it is essentially a skipcond. > + 'correct zero sign') > + > +test:done(true) > -- > 2.42.0 >