Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Sergey Kaplun <skaplun@tarantool.org>,
	Maxim Kokryashkin <m.kokryashkin@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit] Simplify handling of instable types in TNEW/TDUP load forwarding.
Date: Thu, 25 Jan 2024 10:57:12 +0300	[thread overview]
Message-ID: <0f39d994-a308-4c35-9bc2-3405fedd5072@tarantool.org> (raw)
In-Reply-To: <20240122110344.5401-1-skaplun@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)

  parent reply	other threads:[~2024-01-25  7:57 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-01-22 11:03 Sergey Kaplun via Tarantool-patches
2024-01-23  8:04 ` Sergey Bronnikov via Tarantool-patches
2024-01-23 13:22   ` Sergey Kaplun via Tarantool-patches
2024-01-25  7:56     ` Sergey Bronnikov via Tarantool-patches
2024-01-25  7:57 ` Sergey Bronnikov via Tarantool-patches [this message]
2024-01-29 11:52 ` Maxim Kokryashkin via Tarantool-patches
2024-02-15 13:45 ` 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=0f39d994-a308-4c35-9bc2-3405fedd5072@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=m.kokryashkin@tarantool.org \
    --cc=sergeyb@tarantool.org \
    --cc=skaplun@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH luajit] Simplify handling of instable types in TNEW/TDUP load forwarding.' \
    /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