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 <mike>
> >
> > 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