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 481AC58E239; Mon, 21 Aug 2023 12:35:53 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 481AC58E239 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1692610553; bh=P4h/rxGcYglxOSABFV55DYY8MWRYEEuHdVxcxuL3w9o=; 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=zloLYoPgVZIjECcfHDrsLLqJIrZ1myjYsVmZiwwvtSi21LtctC5or2zrrqc37FpHC X5eTqgvaNWonsnhybEAWSLxG7sXd3oYVXC5g2mTM1eNN6s+vauWbu1r5roHwX04RS3 a2ixxTFyn56EYyZc+nHjUEpoYZhp27uH7pVS4RrI= Received: from smtp46.i.mail.ru (smtp46.i.mail.ru [95.163.41.84]) (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 184FE58E239 for ; Mon, 21 Aug 2023 12:35:52 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 184FE58E239 Received: by smtp46.i.mail.ru with esmtpa (envelope-from ) id 1qY1KB-00Bhyu-0y; Mon, 21 Aug 2023 12:35:51 +0300 Date: Mon, 21 Aug 2023 12:31:06 +0300 To: Maxim Kokryashkin Message-ID: References: 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: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD93C8852532D76B9E3B62DC7F09619FD73B4ABEF74D25B7E0A182A05F5380850400228C9064EF303A5D903E3FB8A199DD0BA61765EC17B3C165BA0E4D61D364B28 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE72FFC9A718DD021A9EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006373745FD4183B699148638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8A515AB015BA4A2B947412BCB492CC569117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC00E8CE3DD197987DA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18BDFBBEFFF4125B51D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE140C956E756FBB7A07FB45A5F6E725C8D8FC6C240DEA76429C9F4D5AE37F343AA9539A8B242431040A6AB1C7CE11FEE32A336C651863509103F1AB874ED89028C4224003CC836476E2F48590F00D11D6E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407978DA827A17800CE79F72382A8EA570EC2DBA43225CD8A89FB26E97DCB74E6252CE5475246E174218B5C8C57E37DE458BEDA766A37F9254B7 X-C1DE0DAB: 0D63561A33F958A5371BC663635F2C9092449FB8121C8BD36A178B9A15A9CEF5F87CCE6106E1FC07E67D4AC08A07B9B065B78C30F681404DCB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D34DA1FE609583D493C1C8E30E2A49DC5351283BC640B4243F53DBD944A75D220A4A41DE15322601D071D7E09C32AA3244C80305992A428E2224F3482442903361FD08D48398F32B4A685A42E4C463514DC5DA084F8E80FEBD3202CD0F03380D9577A83BD0C44CE203720ABEDE4BBDD9CDD X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojHJI2DMjVra011gBwAsiGYg== X-Mailru-Sender: 11C2EC085EDE56FAC07928AF2646A76935AAEB230696CF6DD903E3FB8A199DD00D95629463F16A6ADEDBA653FF35249392D99EB8CC7091A70E183A470755BFD208F19895AA18418972D6B4FCE48DF648AE208404248635DF X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 4/5] Fix pow() optimization inconsistencies. 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 Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Maxim! Thanks for the answers! Fixed your comment and updated the branch. On 21.08.23, Maxim Kokryashkin wrote: > Hi, Sergey! > Thanks for the fixes! > LGTM now, see my answers below. > On Mon, Aug 21, 2023 at 11:06:32AM +0300, Sergey Kaplun wrote: > > > > > > + > > > > +-- 2921 ^ 0.5 = 0x1.b05ec632536fap+5. > > > We certainly need to add some explanation here about the precision, because > > > it is not obvious why these magic numbers should cause any issues. > > > > I suppose any really intererested in this reader may compare the > > behaviour of the glibc implementation of `sqrt()` and `pow()`. Also, the > > comment should mention this implementation, so it becomes too huge and > > distracts the reader from the test case itself. > Something like the comment below is sufficient: > | This number has no special meaning and is used as one that gives different > | results when its square root is obtained with glibc's `sqrt` and `power` > | operations, thanks to their implementation nuances. > > I strongly suggest adding it to make the test case more understandable. Added. See the iterative patch below: =================================================================== diff --git a/test/tarantool-tests/lj-684-pow-inconsistencies.test.lua b/test/tarantool-tests/lj-684-pow-inconsistencies.test.lua index 418a1557..cfd4860d 100644 --- a/test/tarantool-tests/lj-684-pow-inconsistencies.test.lua +++ b/test/tarantool-tests/lj-684-pow-inconsistencies.test.lua @@ -50,6 +50,9 @@ test:samevalues(res, ('consistent results for folding (-inf) ^ 0.5')) -- 2921 ^ 0.5 = 0x1.b05ec632536fap+5. res = {} +-- This number has no special meaning and is used as one that +-- gives different results when its square root is obtained with +-- glibc's `sqrt()` and `pow()` operations. -- XXX: use local variable to prevent folding via parser. -- XXX: use stack slot out of trace to prevent constant folding. local corner_case_pow_05 = 2921 =================================================================== > > > > Ignoring for now. > > > > > > +res = {} > > > > > > > > > > +test:samevalues(res, ('consistent results for folding 2921 ^ 0.5')) > > > > > > I believe it is possible to make a single function with different > > > parameters for all three cases above. > > > Something like `test_power(value, power, extra_map)`, so you can do > > > | res[i] = extra_map(value ^ power) > > > > I afraid that this function doesn't give any improvement in readability, > > also, it may change the trace semantics, so I prefer to leave it as is. > > > > Ignoring for now. > I've expressed my suggestion incomprehensively, sorry. Here is what I've meant > someting like this: > > | local function pow_test_case(value, power, extra_map) > | jit.on() > | res = {} > | jit.on() > | for i = 1, 4 do > | res[i] = extra_map(value ^ power) > | end > | > | -- XXX: Prevent hotcount side effects. > | jit.off() > | jit.flush() > | > | test:samevalues(res, ('consistent results for <...>')) > | end > > Anyway, I've checked the jit.dump by myself, and even for the simple > cases traces are entirely different. With that in mind, I believe, this > comment should be ignored, even though this is very sad. Yes, also it changes the semantic of trace, since power isn't a constant, fold optimization isn't taken. > > > > > > > > > > + > > > > > > > > > > +-- Need some value near 1, to avoid infinite result. > > > Typo: s/Need/We need/ > > > Typo: s/avoid/avoid an/ > > > > Fixed. > > > > See the iterative patch below. > > > > =================================================================== > > diff --git a/test/tarantool-tests/lj-684-pow-inconsistencies.test.lua b/test/tarantool-tests/lj-684-pow-inconsistencies.test.lua > > index 5129fc45..003fe957 100644 > > --- a/test/tarantool-tests/lj-684-pow-inconsistencies.test.lua > > +++ b/test/tarantool-tests/lj-684-pow-inconsistencies.test.lua > > @@ -18,7 +18,7 @@ jit.off() > > jit.flush() > > > > local res = {} > > --- -0 ^ 0.5 = 0. Test sign with `tostring()`. > > +-- -0 ^ 0.5 = 0. Test the sign with `tostring()`. > > -- XXX: use local variable to prevent folding via parser. > > -- XXX: use stack slot out of trace to prevent constant folding. > > local minus_zero = -0 > > @@ -75,7 +75,7 @@ jit.on() > > pow(1, 2) > > pow(1, 2) > > > > --- Need some value near 1, to avoid infinite result. > > +-- We need some value near 1, to avoid an infinite result. > > local base = 1.0000000001 > > local power = 65536 * 3 > > local resulting_value = pow(base, power) > > =================================================================== > > > > > > +local base = 1.0000000001 > > > > > > > > > > -- > > > > 2.41.0 > > > > > > > > -- > > Best regards, > > Sergey Kaplun -- Best regards, Sergey Kaplun