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 BF8CD6D1544; Tue, 7 Nov 2023 13:26:16 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org BF8CD6D1544 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1699352776; bh=LHotB5++oNHFwxG9pFQ4Ad+onboBb4s2f7JkBnvocBI=; 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=EoRf7BXbw2UYe/IQonu00wF6fNuVWjENqsU535k4s7Q32Yezp+WO9R2Q6UO5eozwX oHvyn7RpSxTPL8NpbNd3B0q/pjN5Ru1dJxsh4cFQKfsk8tcZTMBYy7Z8VpV4+vTPHu eV6GR3zRc86J6gGB0jRCUcQtDNwJY/4CmHu08/fQ= Received: from smtpng1.i.mail.ru (smtpng1.i.mail.ru [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 053E76D1544 for ; Tue, 7 Nov 2023 13:26:16 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 053E76D1544 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1r0JHj-0007OD-3B; Tue, 07 Nov 2023 13:26:15 +0300 Date: Tue, 7 Nov 2023 13:21:42 +0300 To: Maxim Kokryashkin 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: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD9CDA3EE4AC542069D57243F17B4512B05A2098E048DA3CAD7182A05F5380850404C228DA9ACA6FE2743D525BD93C6DAC53BA970524DBCB2EFA7578E7289ABD0383A2E587401C66A3F X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7E50C24D7D7C3118AC2099A533E45F2D0395957E7521B51C2CFCAF695D4D8E9FCEA1F7E6F0F101C6778DA827A17800CE797F4D2EDC29AFAF7EA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38B73AB1701401CD8716BC1B225093007B2E16009315E1AF190CC7F00164DA146DAFE8445B8C89999728AA50765F79006370BDB19F53EE528DD389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC80839144E5BB460BAF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA7E827F84554CEF5019E625A9149C048EE33AC447995A7AD183FC91FA280E0CE3D3A03B725D353964B0B7D0EA88DDEDAC722CA9DD8327EE4930A3850AC1BE2E73564E4DC1543031AB7C4224003CC83647689D4C264860C145E X-C1DE0DAB: 0D63561A33F958A5B4CBC2A08A0834A31F0CD5C7E07B22E41AB777EC901EB9E0F87CCE6106E1FC07E67D4AC08A07B9B0735DFC8FA7AC1207CB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF85ED4E12FDF70CC23F7667D7C0F7C77B807AE19F8318DBFC66E2B6FE4B26E4AB0FDE0425728830092B33B16BC67043E09FFD2B42ADC85A61686FE7B743446AADE48CAC7CA610320002C26D483E81D6BE5EF9655DD6DEA7D65774BB76CC95456EEC5B5AD62611EEC62B5AFB4261A09AF0 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojoiosMWyUVRfsE4svvxcX/w== X-DA7885C5: 4619685515DF51A7E6386CB7E8C352599E0C6EE882268C3368372293123DACAC262E2D401490A4A0DB037EFA58388B346E8BC1A9835FDE71 X-Mailru-Sender: 689FA8AB762F7393590D8C940224AE3331554B22DFE5FD0B642958CB531A9CA30FBE9A32752B8C9C2AA642CC12EC09F1FB559BB5D741EB962F61BD320559CF1EFD657A8799238ED55FEEDEB644C299C0ED14614B50AE0675 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: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Maxim! Thanks for the review! Fixed typos and force-pushed the branch. On 07.11.23, Maxim Kokryashkin wrote: > 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,/ Fixed, thanks! > > 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,/ Fixed! > > > > 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/ Fixed. > > +-- Take any value from the mentioned range. Chosen one is > Typo: s/Chosen/The chosen/ Fixed. > > +-- 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. I preferred the following way to accent in one place when the test is skipped (in the test itself). Ignoring for now. > > + 'correct zero sign') > > + > > +test:done(true) > > -- > > 2.42.0 > > See the iterative patch below, branch is force-pushed. =================================================================== diff --git a/test/tarantool-tests/lj-859-math-ceil-sign.test.lua b/test/tarantool-tests/lj-859-math-ceil-sign.test.lua index df0ca329..831b6480 100644 --- a/test/tarantool-tests/lj-859-math-ceil-sign.test.lua +++ b/test/tarantool-tests/lj-859-math-ceil-sign.test.lua @@ -11,8 +11,8 @@ 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. --- Take any value from the mentioned range. Chosen one is +-- Use `tostirng()` to compare the sign of the returned value. +-- Take any value from the mentioned range. The chosen one is -- mentioned in the commit message. test:ok((IS_DUALNUM and IS_X86_64) or tostring(math.ceil(-0.9)) == '-0', 'correct zero sign') =================================================================== -- Best regards, Sergey Kaplun