Hi! Thanks for the fixes! LGTM now.     -- Best regards, Maxim Kokryashkin     >Вторник, 7 ноября 2023, 13:26 +03:00 от Sergey Kaplun : >  >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