<HTML><BODY><div>Hi, Sergey!</div><div>Thanks for the patch!</div><div>LGTM, except for a few nits regarding the commit message.</div><div> </div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div> <blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_16837223450282789850_BODY">From: Mike Pall <mike><br><br>Reported by Sergey Kaplun.<br><br>(cherry picked from commit 96fc114a7a3be3fd2c227d5a0ac53aa50cfb85d1)<br><br>This commit is a follow-up for the commit<br>f067cf638cf8987ab3b6db372d609a5982e458b5 ("Fix narrowing of unary<br>minus."). Since this commit -0 IR constant is stored as well as +0</div></div></div></div></blockquote><div>Typo: s/as well as/as well as the/</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>constant on the trace. Since IR NEWREF keys don't canonicalizied for -0</div></div></div></div></blockquote><div>Typo: s/don’t canonicalized/don’t get canonicalized/</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>opposed of IR HREFK, it may lead to inconsistencies during trace</div></div></div></div></blockquote><div>Typo: s/opposed of/as opposed to/</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>recording.<br><br>In particular, since -0 and 0 are different IR constants, alias analysis<br>declares that they can't be aliased during folding optimization.<br>Therefore:<br>1) For the IR TNEW we have non-nil value to lookup from the table via<br> HLOAD, when only nil lookup is expected due to alias analysis.<br>2) For the IR TDUP we have non-nil value to lookup from the table via<br> HLOAD, but the template table has no 0 field initiated as far as -0<br> isn't folding to 0 during parsing (see `bcemit_unop()` in<br> <src/lj_parse.c>).<br>These cases lead to the assertion failures in `fwd_ahload()`.</div></div></div></div></blockquote><div>Typo: s/the assertion/assertion/</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>This patch adds the aforementioned canonicalization.<br><br>Sergey Bronnikov:<br>* reported the original issue for the TDUP IR<br><br>Sergey Kaplun:<br>* added the description and the test for the problem<br><br>Part of tarantool/tarantool#8516<br>---<br><br>Side note: I don't mention the 981 issue by intend -- I don't want to<br>bother Mike with force pushes:). I suppose Igor should add this line (if<br>he wants) went this commit will be cherry-picked to our master branch<br>(a.k.a. tarantool).<br><br> src/lj_record.c | 2 +<br> .../tarantool-tests/lj-981-folding-0.test.lua | 57 +++++++++++++++++++<br> 2 files changed, 59 insertions(+)<br> create mode 100644 test/tarantool-tests/lj-981-folding-0.test.lua<br><br>diff --git a/src/lj_record.c b/src/lj_record.c<br>index 9e2e1d9e..cc44db8d 100644<br>--- a/src/lj_record.c<br>+++ b/src/lj_record.c<br>@@ -1474,6 +1474,8 @@ TRef lj_record_idx(jit_State *J, RecordIndex *ix)<br> TRef key = ix->key;<br> if (tref_isinteger(key)) /* NEWREF needs a TValue as a key. */<br> key = emitir(IRTN(IR_CONV), key, IRCONV_NUM_INT);<br>+ else if (tref_isnumber(key) && tref_isk(key) && tvismzero(&ix->keyv))<br>+ key = lj_ir_knum_zero(J); /* Canonicalize -0.0 to +0.0. */<br> xref = emitir(IRT(IR_NEWREF, IRT_PGC), ix->tab, key);<br> keybarrier = 0; /* NEWREF already takes care of the key barrier. */<br> #ifdef LUAJIT_ENABLE_TABLE_BUMP<br>diff --git a/test/tarantool-tests/lj-981-folding-0.test.lua b/test/tarantool-tests/lj-981-folding-0.test.lua<br>new file mode 100644<br>index 00000000..251da24d<br>--- /dev/null<br>+++ b/test/tarantool-tests/lj-981-folding-0.test.lua<br>@@ -0,0 +1,57 @@<br>+local tap = require('tap')<br>+local test = tap.test('lj-981-folding-0'):skipcond({<br>+ ['Test requires JIT enabled'] = not jit.status(),<br>+ ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',<br>+})<br>+<br>+-- Test file to demonstrate LuaJIT misbehaviour on load forwarding<br>+-- for -0 IR constant as table index.<br>+-- See also, <a href="https://github.com/LuaJIT/LuaJIT/issues/981" target="_blank">https://github.com/LuaJIT/LuaJIT/issues/981</a>.<br>+<br>+local jparse = require('utils.jit_parse')<br>+<br>+jit.opt.start('hotloop=1')<br>+<br>+test:plan(4)<br>+<br>+-- Reset traces.<br>+jit.flush()<br>+<br>+jparse.start('i')<br>+local result<br>+local expected = 'result'<br>+-- TNEW:<br>+-- -0 isn't folded during parsing, so it will be set with KSHORT,<br>+-- UNM bytecodes. See <src/lj_parse.c> and bytecode listing<br>+-- for details.<br>+-- Because of it, empty table is created via TNEW.<br>+for _ = 1, 4 do<br>+ result = ({[-0] = expected})[0]<br>+end<br>+<br>+local traces = jparse.finish()<br>+<br>+-- Test that there is no any assertion failure.<br>+test:ok(result == expected, 'TNEW and -0 folding')<br>+-- Test that there is no NEWREF -0 IR.<br>+test:ok(not traces[1]:has_ir('NEWREF.*-0'), '-0 is canonized for TNEW tab')<br>+<br>+-- Reset traces.<br>+jit.flush()<br>+<br>+jparse.start('i')<br>+-- TDUP:<br>+-- Now just add a constant field for the table to use TDUP with<br>+-- template table instead TNEW before -0 is set.<br>+for _ = 1, 4 do<br>+ result = ({[-0] = expected, [1] = 1})[0]<br>+end<br>+<br>+traces = jparse.finish()<br>+<br>+-- Test that there is no any assertion failure.<br>+test:ok(result == expected, 'TDUP and -0 folding')<br>+-- Test that there is no NEWREF -0 IR.<br>+test:ok(not traces[1]:has_ir('NEWREF.*-0'), '-0 is canonized for TDUP tab')<br>+<br>+os.exit(test:check() and 0 or 1)<br>--<br>2.34.1</div></div></div></div></blockquote><div><div>--<br>Best regards,</div><div>Maxim Kokryashkin</div></div></div></blockquote></BODY></HTML>