<HTML><BODY><div>Hi, Sergey!</div><div>Thanks for the patch!</div><div>LGTM</div><div data-signature-widget="container"><div data-signature-widget="content"><div>--<br>Best regards,</div><div>Maxim Kokryashkin</div></div></div><div> </div><div> </div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">Вторник, 24 октября 2023, 13:18 +03:00 от Sergey Kaplun <skaplun@tarantool.org>:<br> <div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_16981427311540721826_BODY">From: Mike Pall <mike><br><br>Thanks to Peter Cawley.<br><br>(cherry-picked from commit 7a2b83a0c5d980bf3db0aeda33c79e7bb4b3da01)<br><br>Even after the commit c05d103305da0626e025dbf81370ca9f4f788c83 ("Fix<br>math.min()/math.max() inconsistencies."), some of the corner cases (see<br>tests in the commit for details) for `math.min()`/`math.max()` still<br>inconsistent in the JIT and the VM. This happens because `IR_MIN` and<br>`IR_MAX` are marked as commutative, so their operands were swapped by<br>`asm_swapops()`. As a result, because `minsd`[1]/`maxsd`[2] instructions<br>don't change the sign byte of the destination register, its sign is<br>preserved if we compare 0 with -0. When we compare something with NaN,<br>the second source operand (either a NaN or a valid floating-point value)<br>is written to the result. Hence, swapping the operands changed the<br>resulting value.<br><br>This patch removes the commutative flag from the aforementioned IRs to<br>prevent swapping of their operands.<br><br>[1]: <a href="https://c9x.me/x86/html/file_module_x86_id_173.html" target="_blank">https://c9x.me/x86/html/file_module_x86_id_173.html</a><br>[2]: <a href="https://c9x.me/x86/html/file_module_x86_id_168.html" target="_blank">https://c9x.me/x86/html/file_module_x86_id_168.html</a><br><br>Sergey Kaplun:<br>* added the description and the test for the problem<br><br>Part of tarantool/tarantool#9145<br>---<br><br>Branch: <a href="https://github.com/tarantool/luajit/tree/skaplun/lj-1082-min-max-0-commutative" target="_blank">https://github.com/tarantool/luajit/tree/skaplun/lj-1082-min-max-0-commutative</a><br>Tarantool PR: <a href="https://github.com/tarantool/tarantool/pull/9294" target="_blank">https://github.com/tarantool/tarantool/pull/9294</a><br>Related Issues:<br>* <a href="https://github.com/tarantool/tarantool/issues/9145" target="_blank">https://github.com/tarantool/tarantool/issues/9145</a><br>* <a href="https://github.com/LuaJIT/LuaJIT/issues/1082" target="_blank">https://github.com/LuaJIT/LuaJIT/issues/1082</a><br>* <a href="https://github.com/LuaJIT/LuaJIT/issues/957" target="_blank">https://github.com/LuaJIT/LuaJIT/issues/957</a><br><br> src/lj_ir.h | 4 +-<br> test/tarantool-tests/gh-6163-min-max.test.lua | 23 ++++-------<br> .../lj-1082-min-max-0-commutative.test.lua | 41 +++++++++++++++++++<br> 3 files changed, 51 insertions(+), 17 deletions(-)<br> create mode 100644 test/tarantool-tests/lj-1082-min-max-0-commutative.test.lua<br><br>diff --git a/src/lj_ir.h b/src/lj_ir.h<br>index 46af54e4..53e9dfd6 100644<br>--- a/src/lj_ir.h<br>+++ b/src/lj_ir.h<br>@@ -76,8 +76,8 @@<br> \<br> _(ABS, N , ref, ref) \<br> _(LDEXP, N , ref, ref) \<br>- _(MIN, C , ref, ref) \<br>- _(MAX, C , ref, ref) \<br>+ _(MIN, N , ref, ref) \<br>+ _(MAX, N , ref, ref) \<br> _(FPMATH, N , ref, lit) \<br> \<br> /* Overflow-checking arithmetic ops. */ \<br>diff --git a/test/tarantool-tests/gh-6163-min-max.test.lua b/test/tarantool-tests/gh-6163-min-max.test.lua<br>index 4bc6155c..f52cb5ca 100644<br>--- a/test/tarantool-tests/gh-6163-min-max.test.lua<br>+++ b/test/tarantool-tests/gh-6163-min-max.test.lua<br>@@ -3,11 +3,6 @@ local test = tap.test('gh-6163-jit-min-max'):skipcond({<br> ['Test requires JIT enabled'] = not jit.status(),<br> })<br> <br>-local x86_64 = jit.arch == 'x86' or jit.arch == 'x64'<br>--- XXX: table to use for dummy check for some inconsistent results<br>--- on the x86/64 architecture.<br>-local DUMMY_TAB = {}<br>-<br> test:plan(18)<br> --<br> -- gh-6163: math.min/math.max inconsistencies.<br>@@ -110,17 +105,17 @@ result = {}<br> for k = 1, 4 do<br> result[k] = min(x, min(x, nan))<br> end<br>--- FIXME: results are still inconsistent for the x86/64 architecture.<br>+-- See also <a href="https://github.com/LuaJIT/LuaJIT/issues/957" target="_blank">https://github.com/LuaJIT/LuaJIT/issues/957</a>.<br> -- expected: nan nan nan nan<br>-test:samevalues(x86_64 and DUMMY_TAB or result, 'math.min: comm_dup_minmax')<br>+test:samevalues(result, 'math.min: comm_dup_minmax')<br> <br> result = {}<br> for k = 1, 4 do<br> result[k] = max(x, max(x, nan))<br> end<br>--- FIXME: results are still inconsistent for the x86/64 architecture.<br>+-- See also <a href="https://github.com/LuaJIT/LuaJIT/issues/957" target="_blank">https://github.com/LuaJIT/LuaJIT/issues/957</a>.<br> -- expected: nan nan nan nan<br>-test:samevalues(x86_64 and DUMMY_TAB or result, 'math.max: comm_dup_minmax')<br>+test:samevalues(result, 'math.max: comm_dup_minmax')<br> <br> -- The following optimization should be disabled:<br> -- (x o k1) o k2 ==> x o (k1 o k2)<br>@@ -179,10 +174,9 @@ result = {}<br> for k = 1, 4 do<br> result[k] = min(1, max(1, nan))<br> end<br>--- FIXME: results are still inconsistent for the x86/64 architecture.<br>+-- See also <a href="https://github.com/LuaJIT/LuaJIT/issues/957" target="_blank">https://github.com/LuaJIT/LuaJIT/issues/957</a>.<br> -- expected: nan nan nan nan<br>-test:samevalues(x86_64 and DUMMY_TAB or result,<br>- 'min-max-case2: reassoc_minmax_right')<br>+test:samevalues(result, 'min-max-case2: reassoc_minmax_right')<br> <br> result = {}<br> for k = 1, 4 do<br>@@ -195,10 +189,9 @@ result = {}<br> for k = 1, 4 do<br> result[k] = max(1, min(1, nan))<br> end<br>--- FIXME: results are still inconsistent for the x86/64 architecture.<br>+-- See also <a href="https://github.com/LuaJIT/LuaJIT/issues/957" target="_blank">https://github.com/LuaJIT/LuaJIT/issues/957</a>.<br> -- expected: nan nan nan nan<br>-test:samevalues(x86_64 and DUMMY_TAB or result,<br>- 'max-min-case2: reassoc_minmax_right')<br>+test:samevalues(result, 'max-min-case2: reassoc_minmax_right')<br> <br> -- XXX: If we look into the disassembled code of `lj_vm_foldarith()`<br> -- we can see the following:<br>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<br>new file mode 100644<br>index 00000000..99bfa298<br>--- /dev/null<br>+++ b/test/tarantool-tests/lj-1082-min-max-0-commutative.test.lua<br>@@ -0,0 +1,41 @@<br>+local tap = require('tap')<br>+<br>+-- Test file to demonstrate LuaJIT inconsistent behaviour for<br>+-- `math.min()` and `math.max()` operations when comparing 0 with<br>+-- -0 in the JIT and the VM.<br>+-- See also: <a href="https://github.com/LuaJIT/LuaJIT/issues/1082" target="_blank">https://github.com/LuaJIT/LuaJIT/issues/1082</a>.<br>+<br>+local test = tap.test('lj-1082-min-max-0-commutative'):skipcond({<br>+ ['Test requires JIT enabled'] = not jit.status(),<br>+})<br>+<br>+test:plan(4)<br>+<br>+-- XXX: simplify `jit.dump()` output.<br>+local min, max = math.min, math.max<br>+<br>+-- Use local variables to prevent fold optimization.<br>+local minus_zero = -0<br>+local zero = 0<br>+<br>+local results_min_pm = {}<br>+local results_min_mp = {}<br>+local results_max_mp = {}<br>+local results_max_pm = {}<br>+<br>+jit.opt.start('hotloop=1')<br>+for i = 1, 4 do<br>+ -- The resulting value is the second parameter for comparison.<br>+ -- Use `tostring()` to distinguish 0 and -0.<br>+ results_min_pm[i] = tostring(min(zero, minus_zero))<br>+ results_min_mp[i] = tostring(min(minus_zero, zero))<br>+ results_max_pm[i] = tostring(max(zero, minus_zero))<br>+ results_max_mp[i] = tostring(max(minus_zero, zero))<br>+end<br>+<br>+test:samevalues(results_min_pm, 'min(0, -0)')<br>+test:samevalues(results_min_mp, 'min(-0, 0)')<br>+test:samevalues(results_max_pm, 'max(0, -0)')<br>+test:samevalues(results_max_mp, 'max(-0, 0)')<br>+<br>+test:done(true)<br>--<br>2.42.0</div></div></div></div></blockquote><div> </div></BODY></HTML>