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

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

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

Analyzed by Sergey Kaplun.

(cherry-picked from commit 9f452bbef5031afc506d8615f5e720c45acd6fdf)

This is a follow-up for the commit
a9d183b2be63fd91be4b8c9494c213c56c491092 ("Fix TNEW load forwarding with
instable types."). It fixes a similar issue, but for TDUP load
forwarding.

Sergey Kaplun:
* added the description and the test for the problem

Part of tarantool/tarantool#8825
---

Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-994-load-fwd-instable-types-tdup
Tarantool PR: https://github.com/tarantool/tarantool/pull/9053
Related issues:
* https://github.com/LuaJIT/LuaJIT/issues/994
* https://github.com/tarantool/tarantool/issues/8825

 src/lj_opt_mem.c                                          | 4 ++--
 .../lj-994-instable-types-during-loop-unroll.test.lua     | 8 +-------
 2 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/src/lj_opt_mem.c b/src/lj_opt_mem.c
index 59fddbdd..3c58d342 100644
--- a/src/lj_opt_mem.c
+++ b/src/lj_opt_mem.c
@@ -194,8 +194,8 @@ static TRef fwd_ahload(jit_State *J, IRRef xref)
 	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);
-	lj_assertJ(itype2irt(tv) == irt_type(fins->t),
-		   "mismatched type in constant table");
+	if (itype2irt(tv) != irt_type(fins->t))
+	  return 0;  /* Type instability in loop-carried dependency. */
 	if (irt_isnum(fins->t))
 	  return lj_ir_knum_u64(J, tv->u64);
 	else if (LJ_DUALNUM && irt_isint(fins->t))
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 730b0e61..6e2cf5ed 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(1)
+test:plan(2)
 
 -- TNEW.
 local result
@@ -34,11 +34,6 @@ end
 test:is(result, nil, 'TNEW load forwarding was successful')
 
 -- TDUP.
---[[
--- FIXME: Disable test case for now. Enable, with another
--- backported commit with a fix for the aforementioned issue
--- (and after patch "Improve assertions.").
--- Test taken trace exits too.
 for _ = 1, 5 do
   local t = slot
   -- Now use constant key slot to get necessary branch.
@@ -48,6 +43,5 @@ for _ = 1, 5 do
   slot = _ % 2 ~= 0 and stored_tab or {true}
 end
 test:is(result, true, 'TDUP load forwarding was successful')
-]]
 
 test:done(true)
-- 
2.42.0

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

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
LGTM
On Tue, Aug 29, 2023 at 12:54:40PM +0300, Sergey Kaplun wrote:
> From: Mike Pall <mike>
> 
> Analyzed by Sergey Kaplun.
> 
> (cherry-picked from commit 9f452bbef5031afc506d8615f5e720c45acd6fdf)
> 
> This is a follow-up for the commit
> a9d183b2be63fd91be4b8c9494c213c56c491092 ("Fix TNEW load forwarding with
> instable types."). It fixes a similar issue, but for TDUP load
> forwarding.
> 
> Sergey Kaplun:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#8825
> ---
> 
> Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-994-load-fwd-instable-types-tdup
> Tarantool PR: https://github.com/tarantool/tarantool/pull/9053
> Related issues:
> * https://github.com/LuaJIT/LuaJIT/issues/994
> * https://github.com/tarantool/tarantool/issues/8825
> 
>  src/lj_opt_mem.c                                          | 4 ++--
>  .../lj-994-instable-types-during-loop-unroll.test.lua     | 8 +-------
>  2 files changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/src/lj_opt_mem.c b/src/lj_opt_mem.c
> index 59fddbdd..3c58d342 100644
> --- a/src/lj_opt_mem.c
> +++ b/src/lj_opt_mem.c
> @@ -194,8 +194,8 @@ static TRef fwd_ahload(jit_State *J, IRRef xref)
>  	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);
> -	lj_assertJ(itype2irt(tv) == irt_type(fins->t),
> -		   "mismatched type in constant table");
> +	if (itype2irt(tv) != irt_type(fins->t))
> +	  return 0;  /* Type instability in loop-carried dependency. */
>  	if (irt_isnum(fins->t))
>  	  return lj_ir_knum_u64(J, tv->u64);
>  	else if (LJ_DUALNUM && irt_isint(fins->t))
> 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 730b0e61..6e2cf5ed 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(1)
> +test:plan(2)
>  
>  -- TNEW.
>  local result
> @@ -34,11 +34,6 @@ end
>  test:is(result, nil, 'TNEW load forwarding was successful')
>  
>  -- TDUP.
> ---[[
> --- FIXME: Disable test case for now. Enable, with another
> --- backported commit with a fix for the aforementioned issue
> --- (and after patch "Improve assertions.").
> --- Test taken trace exits too.
>  for _ = 1, 5 do
>    local t = slot
>    -- Now use constant key slot to get necessary branch.
> @@ -48,6 +43,5 @@ for _ = 1, 5 do
>    slot = _ % 2 ~= 0 and stored_tab or {true}
>  end
>  test:is(result, true, 'TDUP load forwarding was successful')
> -]]
>  
>  test:done(true)
> -- 
> 2.42.0
> 

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

[Sergey Bronnikov via Tarantool-patches]

Hi, Sergey

thanks for the patch! LGTM

And see my minor comments below.


On 8/29/23 12:54, Sergey Kaplun wrote:

<snipped>

>   -- TDUP.
> ---[[
> --- FIXME: Disable test case for now. Enable, with another
> --- backported commit with a fix for the aforementioned issue
> --- (and after patch "Improve assertions.").
> --- Test taken trace exits too.
>   for _ = 1, 5 do
>     local t = slot
>     -- Now use constant key slot to get necessary branch.
> @@ -48,6 +43,5 @@ for _ = 1, 5 do
Nit: assert is triggered with 3 iterations too
>     slot = _ % 2 ~= 0 and stored_tab or {true}
Nit: I would left a comment with explanation why exactly this value is 
assigned to slot.
>   end
>   test:is(result, true, 'TDUP load forwarding was successful')
> -]]
>   
>   test:done(true)

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

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the review!
Fixed your comment, and force-pushed the branch.

On 15.09.23, Sergey Bronnikov wrote:
> Hi, Sergey
> 
> thanks for the patch! LGTM
> 
> And see my minor comments below.
> 
> 
> On 8/29/23 12:54, Sergey Kaplun wrote:
> 
> <snipped>
> 
> >   -- TDUP.
> > ---[[
> > --- FIXME: Disable test case for now. Enable, with another
> > --- backported commit with a fix for the aforementioned issue
> > --- (and after patch "Improve assertions.").
> > --- Test taken trace exits too.
> >   for _ = 1, 5 do
> >     local t = slot
> >     -- Now use constant key slot to get necessary branch.
> > @@ -48,6 +43,5 @@ for _ = 1, 5 do
> Nit: assert is triggered with 3 iterations too

Yes, but I also want to test the trace execution. So, left it as is.

> >     slot = _ % 2 ~= 0 and stored_tab or {true}
> Nit: I would left a comment with explanation why exactly this value is 
> assigned to slot.

Added the corresponding comment.

===================================================================
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 6e2cf5ed..f240bdd2 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
@@ -40,6 +40,8 @@ for _ = 1, 5 do
   -- LJ_TRERR_GFAIL isn't triggered here.
   -- See `fwd_ahload()` in <src/lj_opt_mem.c> for details.
   result = t[1]
+  -- The constant table should contain the key with a different
+  -- type.
   slot = _ % 2 ~= 0 and stored_tab or {true}
 end
 test:is(result, true, 'TDUP load forwarding was successful')
===================================================================

> >   end
> >   test:is(result, true, 'TDUP load forwarding was successful')
> > -]]
> >   
> >   test:done(true)

-- 
Best regards,
Sergey Kaplun

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

[Sergey Bronnikov via Tarantool-patches]

On 9/15/23 16:20, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the review!
> Fixed your comment, and force-pushed the branch.

Thanks!

<snipped>

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

[Igor Munkin via Tarantool-patches]

Sergey,

I've checked the patchset into all long-term branches in
tarantool/luajit and bumped a new version in master, release/2.11 and
release/2.10.

On 29.08.23, Sergey Kaplun via Tarantool-patches wrote:
> From: Mike Pall <mike>
> 
> Analyzed by Sergey Kaplun.
> 
> (cherry-picked from commit 9f452bbef5031afc506d8615f5e720c45acd6fdf)
> 
> This is a follow-up for the commit
> a9d183b2be63fd91be4b8c9494c213c56c491092 ("Fix TNEW load forwarding with
> instable types."). It fixes a similar issue, but for TDUP load
> forwarding.
> 
> Sergey Kaplun:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#8825
> ---
> 
> Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-994-load-fwd-instable-types-tdup
> Tarantool PR: https://github.com/tarantool/tarantool/pull/9053
> Related issues:
> * https://github.com/LuaJIT/LuaJIT/issues/994
> * https://github.com/tarantool/tarantool/issues/8825
> 
>  src/lj_opt_mem.c                                          | 4 ++--
>  .../lj-994-instable-types-during-loop-unroll.test.lua     | 8 +-------
>  2 files changed, 3 insertions(+), 9 deletions(-)
> 

<snipped>

> -- 
> 2.42.0
> 

-- 
Best regards,
IM