From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id A8B766DDCD5; Fri, 10 Nov 2023 14:18:15 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A8B766DDCD5 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1699615095; bh=SL/mZHPQUQw18N++VS2a4HzEW1ZgbGR3TWbaUWTMYHI=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=ZFol8lykEV5Itlin7upPXnImko/5s4Dxhr53CKRu4D+b/f08AqL0iVWOdjavyN7Af cKT7ICEZR6yxvw+e0AETInLPSm7Z55BXQNONvX9hsQWTAPlNV3qHq8py1zcYotQ6Lq P8TQHGVIXdQRyBpIxWZTUeDofqeSoP1dzyFIHEf8= Received: from smtp49.i.mail.ru (smtp49.i.mail.ru [95.163.41.91]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 711766C46BA for ; Fri, 10 Nov 2023 14:18:13 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 711766C46BA Received: by smtp49.i.mail.ru with esmtpa (envelope-from ) id 1r1PWe-00Deeg-14; Fri, 10 Nov 2023 14:18:12 +0300 Message-ID: Date: Fri, 10 Nov 2023 14:18:12 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Sergey Kaplun , Maxim Kokryashkin Cc: tarantool-patches@dev.tarantool.org References: <20231024101420.12398-1-skaplun@tarantool.org> Content-Language: en-US In-Reply-To: <20231024101420.12398-1-skaplun@tarantool.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9C2A6B03AB739174C8B8ADBDCDAD54AB847BB7A66D7B1BE8200894C459B0CD1B95FFDDEA63F18A314F9B3D19F659375CD8B0A5B83515100FC0F3046A6F430FAC3 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7627B7646117F0BF3EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637A975286280E7A0BD8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D809B99343CDF51E43BC55FE366221D3CF117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC8C7ADC89C2F0B2A5A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18F04B652EEC242312D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE26055571C92BF10FB341D7040ADD27A2D8FC6C240DEA76429C9F4D5AE37F343AA9539A8B242431040A6AB1C7CE11FEE32D01283D1ACF37BA2D242C3BD2E3F4C6C4224003CC836476E2F48590F00D11D6E2021AF6380DFAD1A18204E546F3947C062BEEFFB5F8EA3E2E808ACE2090B5E1725E5C173C3A84C3ED8438A78DFE0A9E089D37D7C0E48F6C8AA50765F79006372C56F9BC9525AE6AEFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-C1DE0DAB: 0D63561A33F958A5041FBF6F2FAE93B67A1D972CCA370FE1C6EC20BF4CFA3F44F87CCE6106E1FC07E67D4AC08A07B9B0E753FA5741D1AD02CB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF0EA377ECD63B05A9747708F452BF8ACB572A14171CF64C683287DD50B5842BFFDDCE95BB4BF576A6D0DB820CB2E38D8B77841C2C22A780C06458B5E6F2B4707AE48CAC7CA610320002C26D483E81D6BE0DBAE6F56676BC7117BB6831D7356A2DEC5B5AD62611EEC62B5AFB4261A09AF0 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj1d5ULkquG7x6rYwwEuyDfg== X-Mailru-Sender: 11C2EC085EDE56FAC07928AF2646A769B37246C3AEC779782450A233144E8905816609AA8FC7A761EBA65886582A37BD66FEC6BF5C9C28D98A98C1125256619760D574B6FC815AB872D6B4FCE48DF648AE208404248635DF X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] IR_MIN/IR_MAX is non-commutative due to underlying FPU ops. X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Sergey Thanks for the patch! LGTM On 10/24/23 13:14, Sergey Kaplun wrote: > From: Mike Pall > > 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)