From: sergos via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Maksim Kokryashkin <max.kokryashkin@gmail.com> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH luajit v2] Fix narrowing of unary minus. Date: Mon, 26 Sep 2022 00:31:50 +0300 [thread overview] Message-ID: <4DD728DB-B92B-4466-A709-E3FD08576876@tarantool.org> (raw) In-Reply-To: <20220920111649.86190-1-max.kokryashkin@gmail.com> Hi! Thanks for the patch! I have two questions below. Sergos > On 20 Sep 2022, at 14:16, Maksim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org> wrote: > > From: Mike Pall <mike> > > (cherry picked from commit 1e6e8aaa20626ac94cf907c69b0452f76e9f5fa5) > > 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. > However, in DUALNUM mode with using of TValues containing integers the > -0 value is represented in the same way as the 0 value and there is no > difference between them. > > 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? > > Maxim Kokryashkin: > * added the description and the test for the problem > > Resolves tarantool/tarantool#6976 > Part of tarantool/tarantool#7230 > --- > > Changes in v2: > - Fixed test and commit message as per review by Sergey > > Branch and PR are updated. > > src/lj_opt_narrow.c | 9 +++- > .../gh-6976-narrowing-of-unary-minus.test.lua | 51 +++++++++++++++++++ > 2 files changed, 58 insertions(+), 2 deletions(-) > create mode 100644 test/tarantool-tests/gh-6976-narrowing-of-unary-minus.test.lua > > diff --git a/src/lj_opt_narrow.c b/src/lj_opt_narrow.c > index cd96ca4b..bb61f97b 100644 > --- a/src/lj_opt_narrow.c > +++ b/src/lj_opt_narrow.c > @@ -551,8 +551,13 @@ TRef lj_opt_narrow_unm(jit_State *J, TRef rc, TValue *vc) > { > rc = conv_str_tonum(J, rc, vc); > if (tref_isinteger(rc)) { > - if ((uint32_t)numberVint(vc) != 0x80000000u) > - return emitir(IRTGI(IR_SUBOV), lj_ir_kint(J, 0), rc); > + uint32_t k = (uint32_t)numberVint(vc); > + if ((LJ_DUALNUM || k != 0) && k != 0x80000000u) { > + TRef zero = lj_ir_kint(J, 0); > + if (!LJ_DUALNUM) > + emitir(IRTGI(IR_NE), rc, zero); > + return emitir(IRTGI(IR_SUBOV), zero, rc); > + } > rc = emitir(IRTN(IR_CONV), rc, IRCONV_NUM_INT); > } > return emitir(IRTN(IR_NEG), rc, lj_ir_ksimd(J, LJ_KSIMD_NEG)); > diff --git a/test/tarantool-tests/gh-6976-narrowing-of-unary-minus.test.lua b/test/tarantool-tests/gh-6976-narrowing-of-unary-minus.test.lua > new file mode 100644 > index 00000000..18778a55 > --- /dev/null > +++ b/test/tarantool-tests/gh-6976-narrowing-of-unary-minus.test.lua > @@ -0,0 +1,51 @@ > +local tap = require('tap') > +local test = tap.test('gh-6976-narrowing-of-unary-minus') > +test:plan(2) > + > +jit.opt.start('hotloop=1', 'hotexit=1') > + > +local function check(routine) > + jit.off() > + jit.flush() > + local interp_res = routine() > + jit.on() > + local jit_res = routine() > + > + for i = 1, #interp_res do > + if interp_res[i] ~= jit_res[i] then > + return false > + end > + end > + > + 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' > +) > + > +os.exit(test:check() and 0 or 1) > -- > 2.32.1 (Apple Git-133) >
next prev parent reply other threads:[~2022-09-25 21:31 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 [this message] 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 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=4DD728DB-B92B-4466-A709-E3FD08576876@tarantool.org \ --to=tarantool-patches@dev.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