[Tarantool-patches] [PATCH luajit] x86/x64: Fix math.ceil(-0.9) result sign.

[Tarantool-patches] [PATCH luajit] x86/x64: Fix math.ceil(-0.9) result sign.

[Sergey Kaplun via Tarantool-patches]

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

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.
+-- Take any value from the mentioned range. 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')
+
+test:done(true)
-- 
2.42.0

Re: [Tarantool-patches] [PATCH luajit] x86/x64: Fix math.ceil(-0.9) result sign.

[Maxim Kokryashkin via 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 <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,/
> 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
> 

Re: [Tarantool-patches] [PATCH luajit] x86/x64: Fix math.ceil(-0.9) result sign.

[Sergey Kaplun via 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 <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

Re: [Tarantool-patches] [PATCH luajit] x86/x64: Fix math.ceil(-0.9) result sign.

[Maxim Kokryashkin via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 7210 bytes --]


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

[-- Attachment #2: Type: text/html, Size: 9421 bytes --]

Re: [Tarantool-patches] [PATCH luajit] x86/x64: Fix math.ceil(-0.9) result sign.

[Sergey Bronnikov via Tarantool-patches]

Hi, Sergey

thanks for the patch! LGTM

see a minor comment below

On 11/2/23 15:36, Sergey Kaplun wrote:


<snipped>

> +++ 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.

tostirng -> tostring


> +-- Take any value from the mentioned range. 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')
> +
> +test:done(true)

Re: [Tarantool-patches] [PATCH luajit] x86/x64: Fix math.ceil(-0.9) result sign.

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the review!
Fixed your comment inline, see the iterative patch below.
Branch is rebased and 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 831b6480..a67de888 100644
--- a/test/tarantool-tests/lj-859-math-ceil-sign.test.lua
+++ b/test/tarantool-tests/lj-859-math-ceil-sign.test.lua
@@ -11,7 +11,7 @@ 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 the sign of the returned value.
+-- Use `tostring()` 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',
===================================================================

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit] x86/x64: Fix math.ceil(-0.9) result sign.

[Sergey Bronnikov via Tarantool-patches]

On 11/9/23 17:32, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the review!
> Fixed your comment inline, see the iterative patch below.
> Branch is rebased and force-pushed.
Thanks! LGTM
>

Re: [Tarantool-patches] [PATCH luajit] x86/x64: Fix math.ceil(-0.9) result sign.

[Igor Munkin via Tarantool-patches]

Sergey,

I've checked the patchset into all long-term branches in
tarantool/luajit and bumped a new version in master, release/2.11 and
release/2.10.

On 02.11.23, Sergey Kaplun via Tarantool-patches 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
> 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.
> 
> 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
> 

<snipped>

> -- 
> 2.42.0
> 

-- 
Best regards,
IM