From: Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Maxim Kokryashkin <m.kokryashkin@tarantool.org>, Sergey Bronnikov <sergeyb@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: [Tarantool-patches] [PATCH luajit] IR_MIN/IR_MAX is non-commutative due to underlying FPU ops. Date: Tue, 24 Oct 2023 13:14:20 +0300 [thread overview] Message-ID: <20231024101420.12398-1-skaplun@tarantool.org> (raw) From: Mike Pall <mike> Thanks to Peter Cawley. (cherry-picked from commit 7a2b83a0c5d980bf3db0aeda33c79e7bb4b3da01) Even after the commit c05d103305da0626e025dbf81370ca9f4f788c83 ("Fix math.min()/math.max() inconsistencies."), some of the corner cases (see tests in the commit for details) for `math.min()`/`math.max()` still inconsistent in the JIT and the VM. This happens because `IR_MIN` and `IR_MAX` are marked as commutative, so their operands were swapped by `asm_swapops()`. As a result, because `minsd`[1]/`maxsd`[2] instructions don't change the sign byte of the destination register, its sign is preserved if we compare 0 with -0. When we compare something with NaN, the second source operand (either a NaN or a valid floating-point value) is written to the result. Hence, swapping the operands changed the resulting value. This patch removes the commutative flag from the aforementioned IRs to prevent swapping of their operands. [1]: https://c9x.me/x86/html/file_module_x86_id_173.html [2]: https://c9x.me/x86/html/file_module_x86_id_168.html 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-1082-min-max-0-commutative Tarantool PR: https://github.com/tarantool/tarantool/pull/9294 Related Issues: * https://github.com/tarantool/tarantool/issues/9145 * https://github.com/LuaJIT/LuaJIT/issues/1082 * https://github.com/LuaJIT/LuaJIT/issues/957 src/lj_ir.h | 4 +- test/tarantool-tests/gh-6163-min-max.test.lua | 23 ++++------- .../lj-1082-min-max-0-commutative.test.lua | 41 +++++++++++++++++++ 3 files changed, 51 insertions(+), 17 deletions(-) create mode 100644 test/tarantool-tests/lj-1082-min-max-0-commutative.test.lua diff --git a/src/lj_ir.h b/src/lj_ir.h index 46af54e4..53e9dfd6 100644 --- a/src/lj_ir.h +++ b/src/lj_ir.h @@ -76,8 +76,8 @@ \ _(ABS, N , ref, ref) \ _(LDEXP, N , ref, ref) \ - _(MIN, C , ref, ref) \ - _(MAX, C , ref, ref) \ + _(MIN, N , ref, ref) \ + _(MAX, N , ref, ref) \ _(FPMATH, N , ref, lit) \ \ /* Overflow-checking arithmetic ops. */ \ diff --git a/test/tarantool-tests/gh-6163-min-max.test.lua b/test/tarantool-tests/gh-6163-min-max.test.lua index 4bc6155c..f52cb5ca 100644 --- a/test/tarantool-tests/gh-6163-min-max.test.lua +++ b/test/tarantool-tests/gh-6163-min-max.test.lua @@ -3,11 +3,6 @@ local test = tap.test('gh-6163-jit-min-max'):skipcond({ ['Test requires JIT enabled'] = not jit.status(), }) -local x86_64 = jit.arch == 'x86' or jit.arch == 'x64' --- XXX: table to use for dummy check for some inconsistent results --- on the x86/64 architecture. -local DUMMY_TAB = {} - test:plan(18) -- -- gh-6163: math.min/math.max inconsistencies. @@ -110,17 +105,17 @@ result = {} for k = 1, 4 do result[k] = min(x, min(x, nan)) end --- FIXME: results are still inconsistent for the x86/64 architecture. +-- See also https://github.com/LuaJIT/LuaJIT/issues/957. -- expected: nan nan nan nan -test:samevalues(x86_64 and DUMMY_TAB or result, 'math.min: comm_dup_minmax') +test:samevalues(result, 'math.min: comm_dup_minmax') result = {} for k = 1, 4 do result[k] = max(x, max(x, nan)) end --- FIXME: results are still inconsistent for the x86/64 architecture. +-- See also https://github.com/LuaJIT/LuaJIT/issues/957. -- expected: nan nan nan nan -test:samevalues(x86_64 and DUMMY_TAB or result, 'math.max: comm_dup_minmax') +test:samevalues(result, 'math.max: comm_dup_minmax') -- The following optimization should be disabled: -- (x o k1) o k2 ==> x o (k1 o k2) @@ -179,10 +174,9 @@ result = {} for k = 1, 4 do result[k] = min(1, max(1, nan)) end --- FIXME: results are still inconsistent for the x86/64 architecture. +-- See also https://github.com/LuaJIT/LuaJIT/issues/957. -- expected: nan nan nan nan -test:samevalues(x86_64 and DUMMY_TAB or result, - 'min-max-case2: reassoc_minmax_right') +test:samevalues(result, 'min-max-case2: reassoc_minmax_right') result = {} for k = 1, 4 do @@ -195,10 +189,9 @@ result = {} for k = 1, 4 do result[k] = max(1, min(1, nan)) end --- FIXME: results are still inconsistent for the x86/64 architecture. +-- See also https://github.com/LuaJIT/LuaJIT/issues/957. -- expected: nan nan nan nan -test:samevalues(x86_64 and DUMMY_TAB or result, - 'max-min-case2: reassoc_minmax_right') +test:samevalues(result, 'max-min-case2: reassoc_minmax_right') -- XXX: If we look into the disassembled code of `lj_vm_foldarith()` -- we can see the following: diff --git a/test/tarantool-tests/lj-1082-min-max-0-commutative.test.lua b/test/tarantool-tests/lj-1082-min-max-0-commutative.test.lua new file mode 100644 index 00000000..99bfa298 --- /dev/null +++ b/test/tarantool-tests/lj-1082-min-max-0-commutative.test.lua @@ -0,0 +1,41 @@ +local tap = require('tap') + +-- Test file to demonstrate LuaJIT inconsistent behaviour for +-- `math.min()` and `math.max()` operations when comparing 0 with +-- -0 in the JIT and the VM. +-- See also: https://github.com/LuaJIT/LuaJIT/issues/1082. + +local test = tap.test('lj-1082-min-max-0-commutative'):skipcond({ + ['Test requires JIT enabled'] = not jit.status(), +}) + +test:plan(4) + +-- XXX: simplify `jit.dump()` output. +local min, max = math.min, math.max + +-- Use local variables to prevent fold optimization. +local minus_zero = -0 +local zero = 0 + +local results_min_pm = {} +local results_min_mp = {} +local results_max_mp = {} +local results_max_pm = {} + +jit.opt.start('hotloop=1') +for i = 1, 4 do + -- The resulting value is the second parameter for comparison. + -- Use `tostring()` to distinguish 0 and -0. + results_min_pm[i] = tostring(min(zero, minus_zero)) + results_min_mp[i] = tostring(min(minus_zero, zero)) + results_max_pm[i] = tostring(max(zero, minus_zero)) + results_max_mp[i] = tostring(max(minus_zero, zero)) +end + +test:samevalues(results_min_pm, 'min(0, -0)') +test:samevalues(results_min_mp, 'min(-0, 0)') +test:samevalues(results_max_pm, 'max(0, -0)') +test:samevalues(results_max_mp, 'max(-0, 0)') + +test:done(true) -- 2.42.0
next reply other threads:[~2023-10-24 10:18 UTC|newest] Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-10-24 10:14 Sergey Kaplun via Tarantool-patches [this message] 2023-10-24 11:35 ` Maxim Kokryashkin via Tarantool-patches 2023-11-10 11:18 ` Sergey Bronnikov via Tarantool-patches 2023-11-23 6:30 ` Igor Munkin via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20231024101420.12398-1-skaplun@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=m.kokryashkin@tarantool.org \ --cc=sergeyb@tarantool.org \ --cc=skaplun@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH luajit] IR_MIN/IR_MAX is non-commutative due to underlying FPU ops.' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox