From: sergos via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Maxim Kokryashkin <m.kokryashkin@tarantool.org> Cc: tarantool-patches@dev.tarantool.org, Maksim Kokryashkin <max.kokryashkin@gmail.com> Subject: Re: [Tarantool-patches] [PATCH luajit v2] Fix narrowing of unary minus. Date: Wed, 30 Nov 2022 13:40:04 +0300 [thread overview] Message-ID: <35C34FB9-EBAF-4F9A-ABC1-877D8A04A684@tarantool.org> (raw) In-Reply-To: <1664445487.810639758@f460.i.mail.ru> [-- Attachment #1: Type: text/plain, Size: 3390 bytes --] Hi! Thanks for the fixes, LGTM now. Sergos > On 29 Sep 2022, at 12:58, Maxim Kokryashkin <m.kokryashkin@tarantool.org> wrote: > > Hi, Sergos! > Thanks for the questions! > Please consider my answers amd changes below. > > > LuaJIT narrowing optimization during BC_UNM recording may ignore > > information about sign of zero for integer types of IR. So far the > > resulting value on a trace is not the same as for the interpreter. > > I didn’t get the point - how is it detected, otherwise than tostring()? > If so - should we change the tostring() instead? > Otherwise - we need a test that exposes this difference > I’ve changed the tests, so it’s now more clear that zero sign can affect arithmetic. > Branch is force-pushed. > Here is the diff: > =============================================== > --- a/test/tarantool-tests/gh-6976-narrowing-of-unary-minus.test.lua > +++ b/test/tarantool-tests/gh-6976-narrowing-of-unary-minus.test.lua > @@ -2,7 +2,7 @@ > local test = tap.test('gh-6976-narrowing-of-unary-minus') > test:plan(2) > > -jit.opt.start('hotloop=1', 'hotexit=1') > +jit.opt.start('hotloop=1') > > local function check(routine) > jit.off() > @@ -20,32 +20,29 @@ > return true > end > > -test:ok( > - check( > - function() > - local res = require('table.new')(3, 0) > - for _ = 1, 3 do > - local zero = 0 > - zero = -zero > - table.insert(res, tostring(zero)) > - end > - return res > - end > - ), > - 'incorrect recording for zero' > -) > - > -test:ok( > - check( > - function() > - local res = require('table.new')(3, 0) > - for i = 2, 0, -1 do > - table.insert(res, tostring(-i)) > - end > - return res > - end > - ), > - 'assertion guard fail' > -) > +test:ok(check(function() > + -- We use `table.new()` here to avoid trace > + -- exits due to table rehashing. > + local res = require('table.new')(3, 0) > + for _ = 1, 3 do > + local zero = 0 > + zero = -zero > + -- There is no difference between 0 and -0 from > + -- arithmetic perspective, unless you try to divide > + -- something by them. > + -- `1 / 0 = inf` and `1 / -0 = -inf` > + table.insert(res, 1 / zero) > + end > + return res > +end), 'incorrect recording for zero') > + > +test:ok(check(function() > + -- See the comment about `table.new()` above. > + local res = require('table.new')(3, 0) > + for i = 2, 0, -1 do > + table.insert(res, 1 / -i) > + end > + return res > +end),'assertion guard fail') > > os.exit(test:check() and 0 or 1) > =============================================== > <snipped> > > > This patch fixes the non-DUALNUM mode behaviour. When the zero value is > > identified during recording it should be cast to number so IR_CONV is > > emitted. Also, this patch adds assertion guard checking that value on > > which operation of unary minus is performed isn't zero. > > Does it mean I will exit the trace every time I met a `x = 0; x = -x` in it? > No, that assertion guard takes you back to the interpreter only if a > trace for unary minus was recorded considering `x` as a non-zero value, > and at some point in this trace `x` became zero. ok, it looks reasonable. > > > Best regards, > Maxim Kokryashkin > [-- Attachment #2: Type: text/html, Size: 5380 bytes --]
next prev parent reply other threads:[~2022-11-30 10:40 UTC|newest] Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-09-09 12:15 [Tarantool-patches] [PATCH luajit] " Maksim Kokryashkin via Tarantool-patches 2022-09-14 22:06 ` Sergey Kaplun via Tarantool-patches 2022-09-20 11:16 ` [Tarantool-patches] [PATCH luajit v2] " Maksim Kokryashkin via Tarantool-patches 2022-09-21 8:48 ` Sergey Kaplun via Tarantool-patches 2022-09-25 21:31 ` sergos via Tarantool-patches 2022-09-29 9:58 ` Maxim Kokryashkin via Tarantool-patches 2022-09-29 13:08 ` Sergey Kaplun via Tarantool-patches 2022-10-03 9:57 ` Maxim Kokryashkin via Tarantool-patches 2022-11-30 10:40 ` sergos via Tarantool-patches [this message] 2022-12-12 9:42 ` [Tarantool-patches] [PATCH luajit] " 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=35C34FB9-EBAF-4F9A-ABC1-877D8A04A684@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=m.kokryashkin@tarantool.org \ --cc=max.kokryashkin@gmail.com \ --cc=sergos@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH luajit v2] Fix narrowing of unary minus.' \ /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