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

[Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Sergey

thanks for the patch! LGTM

On 1/22/24 14:03, Sergey Kaplun wrote:
> 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)