<HTML><BODY><div>Hi!</div><div>Thanks for the fixes!</div><div>LGTM now.</div><div> </div><div> </div><div data-signature-widget="container"><div data-signature-widget="content"><div>--<br>Best regards,</div><div>Maxim Kokryashkin</div></div></div><div> </div><div> </div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">Вторник, 7 ноября 2023, 13:26 +03:00 от Sergey Kaplun <skaplun@tarantool.org>:<br> <div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_16993527751344518902_BODY">Hi, Maxim!<br>Thanks for the review!<br>Fixed typos and force-pushed the branch.<br><br>On 07.11.23, Maxim Kokryashkin wrote:<br>> Hi, Sergey!<br>> Thanks for the patch!<br>> LGTM, except for a few nits below.<br>><br>> On Thu, Nov 02, 2023 at 03:36:16PM +0300, Sergey Kaplun wrote:<br>> > From: Mike Pall <mike><br>> ><br>> > Reported by minoki.<br>> ><br>> > (cherry-picked from commit 674afcd4e21d0cf64de3219d347557a0aed8ecc7)<br>> ><br>> > The `ceil` (`floor`) math function implementation calculates (|x| +<br>> > 2^52) - 2^52 for its argument to determine the fractional part of x , so<br>> Typo: s/x ,/x,/<br><br>Fixed, thanks!<br><br>> > it will be rounded to the nearest integer and its sign is restored.<br>> > After that, if the original value is < (>) than the result, the -1 (1)<br>> > is subtracted from it. Take a look at the `ceil()` case. The result of<br>> > the operation `-1 - (-1)` is +0 for FP arithmetic, against -0 expected<br>> > as a result.<br>> ><br>> > This patch changes the `- (-1)` operation to `+ 1` and restores sign<br>> > after it again.<br>> ><br>> > NB: Since in DUALNUM mode on x86/x64 all results are tried to be<br>> > converted to integers the sign of 0 is neglected.<br>> Typo: s/integers/integers,/<br><br>Fixed!<br><br>> ><br>> > Sergey Kaplun:<br>> > * added the description and the test for the problem<br>> ><br>> > Part of tarantool/tarantool#9145<br>> > ---<br>> ><br>> > Branch: <a href="https://github.com/tarantool/luajit/tree/skaplun/lj-859-math-ceil-sign" target="_blank">https://github.com/tarantool/luajit/tree/skaplun/lj-859-math-ceil-sign</a><br>> > Tarantool PR: <a href="https://github.com/tarantool/tarantool/pull/9326" target="_blank">https://github.com/tarantool/tarantool/pull/9326</a><br>> > Related issues:<br>> > * <a href="https://github.com/LuaJIT/LuaJIT/issues/859" target="_blank">https://github.com/LuaJIT/LuaJIT/issues/859</a><br>> > * <a href="https://github.com/tarantool/tarantool/issues/9145" target="_blank">https://github.com/tarantool/tarantool/issues/9145</a><br>> ><br>> > src/vm_x64.dasc | 13 ++++++------<br>> > src/vm_x86.dasc | 13 ++++++------<br>> > .../lj-859-math-ceil-sign.test.lua | 20 +++++++++++++++++++<br>> > 3 files changed, 32 insertions(+), 14 deletions(-)<br>> > create mode 100644 test/tarantool-tests/lj-859-math-ceil-sign.test.lua<br>> ><br>> > diff --git a/src/vm_x64.dasc b/src/vm_x64.dasc<br>> > index 399dfcbf..fc4c6259 100644<br>> > --- a/src/vm_x64.dasc<br>> > +++ b/src/vm_x64.dasc<br>> > @@ -400,9 +400,6 @@<br>> > |.macro sseconst_1, reg, tmp // Synthesize 1.0.<br>> > | sseconst_hi reg, tmp, 3ff00000<br>> > |.endmacro<br>> > -|.macro sseconst_m1, reg, tmp // Synthesize -1.0.<br>> > -| sseconst_hi reg, tmp, bff00000<br>> > -|.endmacro<br>> > |.macro sseconst_2p52, reg, tmp // Synthesize 2^52.<br>> > | sseconst_hi reg, tmp, 43300000<br>> > |.endmacro<br>> > @@ -2598,15 +2595,17 @@ static void build_subroutines(BuildCtx *ctx)<br>> > | addsd xmm1, xmm3 // (|x| + 2^52) - 2^52<br>> > | subsd xmm1, xmm3<br>> > | orpd xmm1, xmm2 // Merge sign bit back in.<br>> > + | sseconst_1 xmm3, RD<br>> > | .if mode == 1 // ceil(x)?<br>> > - | sseconst_m1 xmm2, RD // Must subtract -1 to preserve -0.<br>> > | cmpsd xmm0, xmm1, 6 // x > result?<br>> > + | andpd xmm0, xmm3<br>> > + | addsd xmm1, xmm0 // If yes, add 1.<br>> > + | orpd xmm1, xmm2 // Merge sign bit back in (again).<br>> > | .else // floor(x)?<br>> > - | sseconst_1 xmm2, RD<br>> > | cmpsd xmm0, xmm1, 1 // x < result?<br>> > + | andpd xmm0, xmm3<br>> > + | subsd xmm1, xmm0 // If yes, subtract 1.<br>> > | .endif<br>> > - | andpd xmm0, xmm2<br>> > - | subsd xmm1, xmm0 // If yes, subtract +-1.<br>> > |.endif<br>> > | movaps xmm0, xmm1<br>> > |1:<br>> > diff --git a/src/vm_x86.dasc b/src/vm_x86.dasc<br>> > index 9fa9a3f7..05a8267b 100644<br>> > --- a/src/vm_x86.dasc<br>> > +++ b/src/vm_x86.dasc<br>> > @@ -533,9 +533,6 @@<br>> > |.macro sseconst_1, reg, tmp // Synthesize 1.0.<br>> > | sseconst_hi reg, tmp, 3ff00000<br>> > |.endmacro<br>> > -|.macro sseconst_m1, reg, tmp // Synthesize -1.0.<br>> > -| sseconst_hi reg, tmp, bff00000<br>> > -|.endmacro<br>> > |.macro sseconst_2p52, reg, tmp // Synthesize 2^52.<br>> > | sseconst_hi reg, tmp, 43300000<br>> > |.endmacro<br>> > @@ -3089,15 +3086,17 @@ static void build_subroutines(BuildCtx *ctx)<br>> > | addsd xmm1, xmm3 // (|x| + 2^52) - 2^52<br>> > | subsd xmm1, xmm3<br>> > | orpd xmm1, xmm2 // Merge sign bit back in.<br>> > + | sseconst_1 xmm3, RDa<br>> > | .if mode == 1 // ceil(x)?<br>> > - | sseconst_m1 xmm2, RDa // Must subtract -1 to preserve -0.<br>> > | cmpsd xmm0, xmm1, 6 // x > result?<br>> > + | andpd xmm0, xmm3<br>> > + | addsd xmm1, xmm0 // If yes, add 1.<br>> > + | orpd xmm1, xmm2 // Merge sign bit back in (again).<br>> > | .else // floor(x)?<br>> > - | sseconst_1 xmm2, RDa<br>> > | cmpsd xmm0, xmm1, 1 // x < result?<br>> > + | andpd xmm0, xmm3<br>> > + | subsd xmm1, xmm0 // If yes, subtract 1.<br>> > | .endif<br>> > - | andpd xmm0, xmm2<br>> > - | subsd xmm1, xmm0 // If yes, subtract +-1.<br>> > |.endif<br>> > | movaps xmm0, xmm1<br>> > |1:<br>> > diff --git a/test/tarantool-tests/lj-859-math-ceil-sign.test.lua b/test/tarantool-tests/lj-859-math-ceil-sign.test.lua<br>> > new file mode 100644<br>> > index 00000000..df0ca329<br>> > --- /dev/null<br>> > +++ b/test/tarantool-tests/lj-859-math-ceil-sign.test.lua<br>> > @@ -0,0 +1,20 @@<br>> > +local tap = require('tap')<br>> > +<br>> > +-- Test file to demonstrate the incorrect LuaJIT's behaviour<br>> > +-- for `math.ceil(x)` when argument `x`: -1 < x < -0.5.<br>> > +-- See also <a href="https://github.com/LuaJIT/LuaJIT/issues/859" target="_blank">https://github.com/LuaJIT/LuaJIT/issues/859</a>.<br>> > +<br>> > +local test = tap.test('lj-859-math-ceil-sign')<br>> > +<br>> > +test:plan(1)<br>> > +<br>> > +local IS_DUALNUM = tostring(tonumber('-0')) ~= tostring(-0)<br>> > +local IS_X86_64 = jit.arch == 'x86' or jit.arch == 'x64'<br>> > +<br>> > +-- Use `tostirng()` to compare sign of the returned value.<br>> Typo: s/sign/the sign/<br><br>Fixed.<br><br>> > +-- Take any value from the mentioned range. Chosen one is<br>> Typo: s/Chosen/The chosen/<br><br>Fixed.<br><br>> > +-- mentioned in the commit message.<br>> > +test:ok((IS_DUALNUM and IS_X86_64) or tostring(math.ceil(-0.9)) == '-0',<br>> Nit: maybe we should precalculate the predicate before the `test:ok` call?<br>> Or, at least the `IS_DUALNUM and IS_X86_64`, as it is essentially a skipcond.<br><br>I preferred the following way to accent in one place when the test is<br>skipped (in the test itself).<br><br>Ignoring for now.<br><br>> > + 'correct zero sign')<br>> > +<br>> > +test:done(true)<br>> > --<br>> > 2.42.0<br>> ><br><br>See the iterative patch below, branch is force-pushed.<br><br>===================================================================<br>diff --git a/test/tarantool-tests/lj-859-math-ceil-sign.test.lua b/test/tarantool-tests/lj-859-math-ceil-sign.test.lua<br>index df0ca329..831b6480 100644<br>--- a/test/tarantool-tests/lj-859-math-ceil-sign.test.lua<br>+++ b/test/tarantool-tests/lj-859-math-ceil-sign.test.lua<br>@@ -11,8 +11,8 @@ test:plan(1)<br> local IS_DUALNUM = tostring(tonumber('-0')) ~= tostring(-0)<br> local IS_X86_64 = jit.arch == 'x86' or jit.arch == 'x64'<br> <br>--- Use `tostirng()` to compare sign of the returned value.<br>--- Take any value from the mentioned range. Chosen one is<br>+-- Use `tostirng()` to compare the sign of the returned value.<br>+-- Take any value from the mentioned range. The chosen one is<br> -- mentioned in the commit message.<br> test:ok((IS_DUALNUM and IS_X86_64) or tostring(math.ceil(-0.9)) == '-0',<br> 'correct zero sign')<br>===================================================================<br><br>--<br>Best regards,<br>Sergey Kaplun</div></div></div></div></blockquote><div> </div></BODY></HTML>