Re: [Tarantool-patches] [PATCH luajit] Simplify handling of instable types in TNEW/TDUP load forwarding.

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 4128 bytes --]


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

[-- Attachment #2: Type: text/html, Size: 5389 bytes --]