<HTML><BODY><div>Hi, Sergey!</div><div>Thanks for the patch!</div><div>LGTM</div><div data-signature-widget="container"><div data-signature-widget="content"><div>--<br>Best regards,</div><div>Maxim Kokryashkin</div></div></div><div> </div><div> </div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">Понедельник, 22 января 2024, 14:08 +03:00 от Sergey Kaplun <skaplun@tarantool.org>:<br> <div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_17059216811420956643_BODY">From: Mike Pall <mike><br><br>Thanks to Peter Cawley.<br><br>(cherry picked from commit c42c62e71a45a677b8b1cbf749bd33cf4d5918ff)<br><br>This patch is a follow-up for the commit<br>b89186cb03e79359847cdbeb000a830cc464db35 ("Fix handling of instable<br>types in TNEW/TDUP load forwarding."). The problem still occurs when<br>TDUP load forwarding is performed for the boolean value. The<br>`fwd_ahload()` always takes the type of IR without checking the type of<br>the TValue for this iteration.<br><br>This patch solves the issue by returning the expected type of the TValue<br>from the constant table (or `nil` for TNEW IR) instead. In the case of<br>type instability, the error in `loop_unroll()` will be raised, and the<br>loop will be unrolled for one more iteration to prevent inconsistencies.<br><br>Sergey Kaplun:<br>* added the description and the test for the problem<br><br>Part of tarantool/tarantool#9595<br>---<br><br>Branch: <a href="https://github.com/tarantool/luajit/tree/skaplun/lj-994-instable-pri-types" target="_blank">https://github.com/tarantool/luajit/tree/skaplun/lj-994-instable-pri-types</a><br>Tarantool PR: <a href="https://github.com/tarantool/tarantool/pull/9608" target="_blank">https://github.com/tarantool/tarantool/pull/9608</a><br>Related issues:<br>* <a href="https://github.com/tarantool/tarantool/issues/9595" target="_blank">https://github.com/tarantool/tarantool/issues/9595</a><br>* <a href="https://github.com/LuaJIT/LuaJIT/issues/994" target="_blank">https://github.com/LuaJIT/LuaJIT/issues/994</a><br><br> src/lj_opt_mem.c | 20 +++++++++----------<br> ...instable-types-during-loop-unroll.test.lua | 15 +++++++++++++-<br> 2 files changed, 23 insertions(+), 12 deletions(-)<br><br>diff --git a/src/lj_opt_mem.c b/src/lj_opt_mem.c<br>index 24a490d5..9044f09a 100644<br>--- a/src/lj_opt_mem.c<br>+++ b/src/lj_opt_mem.c<br>@@ -217,25 +217,23 @@ static TRef fwd_ahload(jit_State *J, IRRef xref)<br> }<br> ref = store->prev;<br> }<br>- if (ir->o == IR_TNEW && !irt_isnil(fins->t))<br>- return 0; /* Type instability in loop-carried dependency. */<br>- if (irt_ispri(fins->t)) {<br>- return TREF_PRI(irt_type(fins->t));<br>- } else if (irt_isnum(fins->t) || (LJ_DUALNUM && irt_isint(fins->t)) ||<br>- irt_isstr(fins->t)) {<br>+ /* Simplified here: let loop_unroll() figure out any type instability. */<br>+ if (ir->o == IR_TNEW) {<br>+ return TREF_NIL;<br>+ } else {<br> TValue keyv;<br> cTValue *tv;<br> IRIns *key = IR(xr->op2);<br> if (key->o == IR_KSLOT) key = IR(key->op1);<br> lj_ir_kvalue(J->L, &keyv, key);<br> tv = lj_tab_get(J->L, ir_ktab(IR(ir->op1)), &keyv);<br>- if (itype2irt(tv) != irt_type(fins->t))<br>- return 0; /* Type instability in loop-carried dependency. */<br>- if (irt_isnum(fins->t))<br>+ if (tvispri(tv))<br>+ return TREF_PRI(itype2irt(tv));<br>+ else if (tvisnum(tv))<br> return lj_ir_knum_u64(J, tv->u64);<br>- else if (LJ_DUALNUM && irt_isint(fins->t))<br>+ else if (tvisint(tv))<br> return lj_ir_kint(J, intV(tv));<br>- else<br>+ else if (tvisgcv(tv))<br> return lj_ir_kstr(J, strV(tv));<br> }<br> /* Othwerwise: don't intern as a constant. */<br>diff --git a/test/tarantool-tests/lj-994-instable-types-during-loop-unroll.test.lua b/test/tarantool-tests/lj-994-instable-types-during-loop-unroll.test.lua<br>index f240bdd2..a0cc8487 100644<br>--- a/test/tarantool-tests/lj-994-instable-types-during-loop-unroll.test.lua<br>+++ b/test/tarantool-tests/lj-994-instable-types-during-loop-unroll.test.lua<br>@@ -10,7 +10,7 @@ local test = tap.test('lj-994-instable-types-during-loop-unroll'):skipcond({<br> <br> -- TODO: test that compiled traces don't always exit by the type<br> -- guard. See also the comment for the TDUP test chunk.<br>-test:plan(2)<br>+test:plan(3)<br> <br> -- TNEW.<br> local result<br>@@ -46,4 +46,17 @@ for _ = 1, 5 do<br> end<br> test:is(result, true, 'TDUP load forwarding was successful')<br> <br>+-- TDUP, primitive types.<br>+for i = 1, 5 do<br>+ local t = slot<br>+ -- Now use constant key slot to get necessary branch.<br>+ -- LJ_TRERR_GFAIL isn't triggered here.<br>+ -- See `fwd_ahload()` in <src/lj_opt_mem.c> for details.<br>+ result = t[1]<br>+ -- The constant tables should contain different booleans<br>+ -- (primitive types).<br>+ slot = i % 2 ~= 0 and {false} or {true}<br>+end<br>+test:is(result, true, 'TDUP load forwarding (primitive types) was successful')<br>+<br> test:done(true)<br>--<br>2.43.0</div></div></div></div></blockquote><div> </div></BODY></HTML>