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

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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
>