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 C37CB58E239; Mon, 21 Aug 2023 12:06:15 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org C37CB58E239 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1692608775; bh=5oZVbtiTtV6Re6SD0lEIjAMybrP00hB6ar1kyY83Ayw=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=P9t9u5v3T8Lc4slDcGEKyFx8d8RTyTuWIjERTB2iZW4MqNClDA89n3b2j0p1+fIAi 9UDfZ4aQy/VFMEtriNalBgQfP05+HYKJlIyjL/jCjQocnUYB9Tgz0SwWoe2bhGeddy B/fwR4YuzJJMzEKMRu5uI9MNobJAxlhtfdTR1P/0= Received: from smtp59.i.mail.ru (smtp59.i.mail.ru [95.163.41.97]) (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 8474C572182 for ; Mon, 21 Aug 2023 12:06:14 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 8474C572182 Received: by smtp59.i.mail.ru with esmtpa (envelope-from ) id 1qY0rV-00FtyW-0k; Mon, 21 Aug 2023 12:06:13 +0300 Date: Mon, 21 Aug 2023 12:06:12 +0300 To: Sergey Kaplun Message-ID: References: <3d4ed7eb3db111ca1571688b281c560bd7a0f674.1692089299.git.skaplun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD90D1D1AB54508998154743820A78DF337F4722C12BCC8CA79182A05F5380850404C228DA9ACA6FE27AB7ACD1E44298F806CDCE5D652B4FCD1247032819F59615F97B7E0E3CB7DE544 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7A2F2DCC785917A3AEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637174E2957C4CE0F938638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D857EE542319169257D8BEA863A5357376117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC07CB022245CAE856389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC8D2DCF9CF1F528DBCF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947C2A336C65186350912D242C3BD2E3F4C64AD6D5ED66289B523666184CF4C3C14F6136E347CC761E07725E5C173C3A84C39A1C9D3BA3303E89BA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CF3D321E7403792E342EB15956EA79C166176DF2183F8FC7C08CCB3ED2A1DE2304725E5C173C3A84C315AF0D0D4FC4FA3D35872C767BF85DA2F004C90652538430E4A6367B16DE6309 X-C1DE0DAB: 0D63561A33F958A5828D7B4F8A54F40894976F265E8E575FDD173280ABC2680BF87CCE6106E1FC07E67D4AC08A07B9B0E753FA5741D1AD02CB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D34C1E32F4AD4B2486BDE4BB0AB5576798CF673AD07DC5F9DBF61406D3256ECB9098D2805C455B81E711D7E09C32AA3244CF9393CB1640DF9CBA47DFDE98394163F259227199D06760A85A42E4C463514DC5DA084F8E80FEBD396F07DFE06A4A8314E894E437E78228B66933FA05BD8EF0CAD958392AE682691 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojHJI2DMjVra3NaDo0mqpa6w== X-Mailru-Sender: 0E9E14D9EC491FBA05C0DE36F6206CB021C8739F8E7C16A76CDCE5D652B4FCD11AB5AE572836FD8904C9FB44FCBCE9EE92D99EB8CC7091A7ECEABDC5717908DEF544888E8238EB4872D6B4FCE48DF648AE208404248635DF X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 5/5] Revert to trival pow() optimizations to prevent inaccuracies. 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: Maxim Kokryashkin via Tarantool-patches Reply-To: Maxim Kokryashkin Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Sergey! Thanks for the patch! LGTM now, except for a single comment below. Also, see my answers too. On Mon, Aug 21, 2023 at 11:15:22AM +0300, Sergey Kaplun wrote: > Hi, Maxim! > Thanks for the review! > See my answers below. > > On 20.08.23, Maxim Kokryashkin wrote: > > Hi, Sergey! > > Thanks for the patch! > > Please consider my comments below. > > On Tue, Aug 15, 2023 at 12:36:31PM +0300, Sergey Kaplun wrote: > > > From: Mike Pall > > > > > > (cherry-picked from commit 96d6d5032098ea9f0002165394a8774dcaa0c0ce) > > > > > > This patch fixes different misbehaviour between JIT-compiled code and > > Typo: s/misbehaviour/misbehaviours/ > > Fixed. > > > > the interpreter for power operator with the following ways: > > Typo: s/with/in/ > > Fixed. > > > > * Drop folding optimizations for base ^ n => base * base ..., as far as > > > pow(base, n) isn't interchangeable with just multiplicity of numbers > > > and depends on the implementation. > > > * Since the internal power function is inaccurate for very big or small > > > powers, it is dropped, and `pow()` from the standard library is used > > > instead. To save consistency between JIT behaviour and the VM > > Typo: s/VM/VM,/ > > Fixed. > > > > narrowing optimization is dropped, and only trivial folding > > > optimizations are used. Also, `math_extern2` version with two > > > parameters is dropped, since it's no more used. > > Typo: s/more/longer/ > > Fixed. > > > > > > > Also, this fixes failures of the [220/502] lib/string/format/num.lua > > > test [1] from LuaJIT-test suite. > > Typo: s/from/from the/ > > Fixed. > > > > > > > [1]: https://www.exploringbinary.com/incorrect-floating-point-to-decimal-conversions/ > > > > > > Sergey Kaplun: > > > * added the description and the test for the problem > > > > > > Part of tarantool/tarantool#8825 > > > --- > > > > > > > > > +-- -948388 ^ 3 = -0x1.7ad0e8ad7439dp+59. > > Same as in the previous patch, we need some additinal commentary for > > those magic numbers. > > See my answer in the previous reply. I think the same commentary, as one I suggested in reply, will do. > > > > +res = {} > > > +-- XXX: use local variable to prevent folding via parser. > > > +-- XXX: use stack slot out of trace to prevent constant folding. > > > +local corner_case_3 = -948388 > > Naming is misleading, it seems like it is the test case number, > > which it is not. Please also fix this in the previous patch for `corner_case_5`. > > Renamed as the following: > > =================================================================== > diff --git a/test/tarantool-tests/lj-684-pow-inconsistencies.test.lua b/test/tarantool-tests/lj-684-pow-inconsistencies.test.lua > index 003fe957..418a1557 100644 > --- a/test/tarantool-tests/lj-684-pow-inconsistencies.test.lua > +++ b/test/tarantool-tests/lj-684-pow-inconsistencies.test.lua > @@ -52,10 +52,10 @@ test:samevalues(res, ('consistent results for folding (-inf) ^ 0.5')) > res = {} > -- XXX: use local variable to prevent folding via parser. > -- XXX: use stack slot out of trace to prevent constant folding. > -local corner_case_05 = 2921 > +local corner_case_pow_05 = 2921 > jit.on() > for i = 1, 4 do > - res[i] = corner_case_05 ^ 0.5 > + res[i] = corner_case_pow_05 ^ 0.5 > end > > -- XXX: Prevent hotcount side effects. > =================================================================== > > =================================================================== > diff --git a/test/tarantool-tests/lj-684-pow-inconsistencies.test.lua b/test/tarantool-tests/lj-684-pow-inconsistencies.test.lua > index 23dd44e8..57685a72 100644 > --- a/test/tarantool-tests/lj-684-pow-inconsistencies.test.lua > +++ b/test/tarantool-tests/lj-684-pow-inconsistencies.test.lua > @@ -69,10 +69,10 @@ test:samevalues(res, ('consistent results for folding 2921 ^ 0.5')) > res = {} > -- XXX: use local variable to prevent folding via parser. > -- XXX: use stack slot out of trace to prevent constant folding. > -local corner_case_3 = -948388 > +local corner_case_pow_3 = -948388 > jit.on() > for i = 1, 4 do > - res[i] = corner_case_3 ^ 3 > + res[i] = corner_case_pow_3 ^ 3 > end > > -- XXX: Prevent hotcount side effects. > =================================================================== > > Branch is force-pushed. > > > > +jit.on() > > > +for i = 1, 4 do > > > + res[i] = corner_case_3 ^ 3 > > > +end > > > + > > > +-- XXX: Prevent hotcount side effects. > > > +jit.off() > > > +jit.flush() > > > > If you'll succeed making that dedicated function for those test cases in the > > previous patch fix ups, this one should be rewritten too. > > See my answer in the previous reply. As I've already said in the previous reply, I've checked the proposed change by myself and it doesn't seem to work great, although it would increase readability by a great margin. So yep, ignore it. > > > > + > > > +test:samevalues(res, ('consistent results for int pow (-948388) ^ 3')) > > > + > > > -- Narrowing for non-constant base of power operation. > > > local function pow(base, power) > > > return base ^ power > > > -- > > > 2.41.0 > > > > > -- > Best regards, > Sergey Kaplun