[Tarantool-patches] [PATCH luajit] Fix TNEW load forwarding with instable types.

[Tarantool-patches] [PATCH luajit] Fix TNEW load forwarding with instable types.

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

(cherry picked from commit d4c0c6e17ef7edf1f2893bc807746b80612e63e9)

During loop unrolling IR instructions are coped, substituted and
re-emitted for the FOLD/CSE/etc. pipeline. Assume, we have the following
loop-carried dependency: we create a new table or load some stored one
depends on iteration number. In case when we copy the emitted ALOAD IR
instruction (from from the stored table), we copy it as is, including
loading type. But the ALOAD from the new table created (on the previous
iteration) should have nil type, so the assertion in `fwd_ahload()` is
failed.

This patch replaces the assertion to `return 0` to avoid the assertion
failure and stop forwarding in such situations.

But the emitted type-guarded ALOAD will lead to the persistent failure
of the assertion guard on the trace. Hence, another one fix should be
added. Also, TDUP IR is affected, too.
See also the issue mentioned in the test.

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

Part of tarantool/tarantool#8516
---

Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-994-instable-types-during-loop-unroll
PR: https://github.com/tarantool/tarantool/pull/8642
Related issues:
* https://github.com/tarantool/tarantool/issues/8516
* https://github.com/LuaJIT/LuaJIT/issues/994

I don't mention the 994 intentionally to avoid Mike bothering. Also, it
isn't the origin of this commit. Quite the opposite: as a result of this
backporting the following issue was created.

I prefer to backport the patches (this one and the prospective for 994)
separately. So, after this patch is backported, it doesn't add any
critical bugs (always failing guard just creates a new side trace), but
helps to avoid conflicts for future backporting (sadly remembered
"Improve assertions.").

 src/lj_opt_mem.c                              |  3 +-
 ...instable-types-during-loop-unroll.test.lua | 53 +++++++++++++++++++
 2 files changed, 55 insertions(+), 1 deletion(-)
 create mode 100644 test/tarantool-tests/lj-994-instable-types-during-loop-unroll.test.lua

diff --git a/src/lj_opt_mem.c b/src/lj_opt_mem.c
index cc177d39..c8265b4f 100644
--- a/src/lj_opt_mem.c
+++ b/src/lj_opt_mem.c
@@ -180,7 +180,8 @@ static TRef fwd_ahload(jit_State *J, IRRef xref)
 	}
 	ref = store->prev;
       }
-      lua_assert(ir->o != IR_TNEW || irt_isnil(fins->t));
+      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)) ||
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
new file mode 100644
index 00000000..435f6e0e
--- /dev/null
+++ b/test/tarantool-tests/lj-994-instable-types-during-loop-unroll.test.lua
@@ -0,0 +1,53 @@
+local tap = require('tap')
+
+local test = tap.test('lj-994-instable-types-during-loop-unroll'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+-- Test file to demonstrate LuaJIT misbehaviour during loop
+-- unrolling and load forwarding for newly created tables.
+-- See also https://github.com/LuaJIT/LuaJIT/issues/994.
+
+-- 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)
+
+-- TNEW.
+local result
+local stored_tab = {1}
+local slot = {}
+local key = 1
+
+jit.opt.start('hotloop=1')
+-- The trouble happens during loop unrolling when we copy
+-- `>+ num ALOAD` IR in the context of the table on the previous
+-- iteration instead of a new one created via TNEW containing no
+-- values (so type nil should be used instead of num).
+for _ = 1, 5 do
+  local t = slot
+  -- Use non-constant key to avoid LJ_TRERR_GFAIL and undoing the
+  -- loop.
+  result = t[key]
+  -- Swap table loaded by SLOAD to the created via TNEW.
+  slot = _ % 2 ~= 0 and stored_tab or {}
+end
+test:is(result, nil, 'TNEW load forwarding was successful')
+
+-- TDUP.
+--[[
+-- FIXME: Disable test case for now. Enable, with another one
+-- 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.
+  -- LJ_TRERR_GFAIL isn't triggered here.
+  -- See `fwd_ahload()` in <src/lj_opt_mem.c> for details.
+  result = t[1]
+  slot = _ % 2 ~= 0 and stored_tab or {true}
+end
+test:is(result, true, 'TDUP load forwarding was successful')
+]]
+
+os.exit(test:check() and 0 or 1)
-- 
2.34.1

Re: [Tarantool-patches] [PATCH luajit] Fix TNEW load forwarding with instable types.

[Maxim Kokryashkin via Tarantool-patches]

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


Hi, Sergey!
Thanks for the patch!
LGTM, except for several nits below.
 
> 
>>From: Mike Pall <mike>
>>
>>(cherry picked from commit d4c0c6e17ef7edf1f2893bc807746b80612e63e9)
>>
>>During loop unrolling IR instructions are coped, substituted and
>>re-emitted for the FOLD/CSE/etc. pipeline. Assume, we have the following
>>loop-carried dependency: we create a new table or load some stored one
>>depends on iteration number. In case when we copy the emitted ALOAD IR
>Typo: s/depends on iteration/depending on the iteration/
>Typo: s/In case when/If/
>>instruction (from from the stored table), we copy it as is, including
>Typo: s/from from/from/
>>loading type. But the ALOAD from the new table created (on the previous
>Typo: s/loading/load/
>>iteration) should have nil type, so the assertion in `fwd_ahload()` is
>>failed.
>>
>>This patch replaces the assertion to `return 0` to avoid the assertion
>Typo: s/assertion to/assertion with/
>>failure and stop forwarding in such situations.
>>
>>But the emitted type-guarded ALOAD will lead to the persistent failure
>>of the assertion guard on the trace. Hence, another one fix should be
>Typo: s/another one/another/
>>added. Also, TDUP IR is affected, too.
>>See also the issue mentioned in the test.
>>
>>Sergey Kaplun:
>>* added the description and the test for the problem
>>
>>Part of tarantool/tarantool#8516
>>---
>>
>>Branch:  https://github.com/tarantool/luajit/tree/skaplun/lj-994-instable-types-during-loop-unroll
>>PR:  https://github.com/tarantool/tarantool/pull/8642
>>Related issues:
>>*  https://github.com/tarantool/tarantool/issues/8516
>>*  https://github.com/LuaJIT/LuaJIT/issues/994
>>
>>I don't mention the 994 intentionally to avoid Mike bothering. Also, it
>>isn't the origin of this commit. Quite the opposite: as a result of this
>>backporting the following issue was created.
>>
>>I prefer to backport the patches (this one and the prospective for 994)
>>separately. So, after this patch is backported, it doesn't add any
>>critical bugs (always failing guard just creates a new side trace), but
>>helps to avoid conflicts for future backporting (sadly remembered
>>"Improve assertions.").
>>
>> src/lj_opt_mem.c | 3 +-
>> ...instable-types-during-loop-unroll.test.lua | 53 +++++++++++++++++++
>> 2 files changed, 55 insertions(+), 1 deletion(-)
>> create mode 100644 test/tarantool-tests/lj-994-instable-types-during-loop-unroll.test.lua
>>
>>diff --git a/src/lj_opt_mem.c b/src/lj_opt_mem.c
>>index cc177d39..c8265b4f 100644
>>--- a/src/lj_opt_mem.c
>>+++ b/src/lj_opt_mem.c
>>@@ -180,7 +180,8 @@ static TRef fwd_ahload(jit_State *J, IRRef xref)
>>  }
>>  ref = store->prev;
>>       }
>>- lua_assert(ir->o != IR_TNEW || irt_isnil(fins->t));
>>+ 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)) ||
>>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
>>new file mode 100644
>>index 00000000..435f6e0e
>>--- /dev/null
>>+++ b/test/tarantool-tests/lj-994-instable-types-during-loop-unroll.test.lua
>>@@ -0,0 +1,53 @@
>>+local tap = require('tap')
>>+
>>+local test = tap.test('lj-994-instable-types-during-loop-unroll'):skipcond({
>>+ ['Test requires JIT enabled'] = not jit.status(),
>>+})
>>+
>>+-- Test file to demonstrate LuaJIT misbehaviour during loop
>>+-- unrolling and load forwarding for newly created tables.
>>+-- See also  https://github.com/LuaJIT/LuaJIT/issues/994 .
>>+
>>+-- 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)
>>+
>>+-- TNEW.
>>+local result
>>+local stored_tab = {1}
>>+local slot = {}
>>+local key = 1
>>+
>>+jit.opt.start('hotloop=1')
>>+-- The trouble happens during loop unrolling when we copy
>>+-- `>+ num ALOAD` IR in the context of the table on the previous
>>+-- iteration instead of a new one created via TNEW containing no
>>+-- values (so type nil should be used instead of num).
>>+for _ = 1, 5 do
>>+ local t = slot
>>+ -- Use non-constant key to avoid LJ_TRERR_GFAIL and undoing the
>>+ -- loop.
>Typo: s/Use/Use a/
>>+ result = t[key]
>>+ -- Swap table loaded by SLOAD to the created via TNEW.
>>+ slot = _ % 2 ~= 0 and stored_tab or {}
>>+end
>>+test:is(result, nil, 'TNEW load forwarding was successful')
>Nit: not sure about that assertion. AFAICS, we want to check whether we
>have failed on assertion and not the validity of `result` variable. Feel free
>to ignore.
>>+
>>+-- TDUP.
>>+--[[
>>+-- FIXME: Disable test case for now. Enable, with another one
>Typo: s/another one/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.
>>+ -- LJ_TRERR_GFAIL isn't triggered here.
>>+ -- See `fwd_ahload()` in <src/lj_opt_mem.c> for details.
>>+ result = t[1]
>>+ slot = _ % 2 ~= 0 and stored_tab or {true}
>>+end
>>+test:is(result, true, 'TDUP load forwarding was successful')
>>+]]
>Nit: Not sure if it is good to add this test here. Maybe it is better
>to create a ticket in our repo and move that chunk there. Feel free
>to ignore.
>>+
>>+os.exit(test:check() and 0 or 1)
>>--
>>2.34.1
>--
>Best regards,
>Maxim Kokryashkin
> 

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

Re: [Tarantool-patches] [PATCH luajit] Fix TNEW load forwarding with instable types.

[Maxim Kokryashkin via Tarantool-patches]

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


Hi again!
I forgot to mention another important nit — the
Tarantool CI is incomplete, there are 20 runs missing.
I think you should trigger them again, just to make sure
everything is ok.
--
Best regards,
Maxim Kokryashkin
 
  
>Понедельник, 15 мая 2023, 13:30 +03:00 от Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>:
> 
>Hi, Sergey!
>Thanks for the patch!
>LGTM, except for several nits below.
> 
>> 
>>>From: Mike Pall <mike>
>>>
>>>(cherry picked from commit d4c0c6e17ef7edf1f2893bc807746b80612e63e9)
>>>
>>>During loop unrolling IR instructions are coped, substituted and
>>>re-emitted for the FOLD/CSE/etc. pipeline. Assume, we have the following
>>>loop-carried dependency: we create a new table or load some stored one
>>>depends on iteration number. In case when we copy the emitted ALOAD IR
>>Typo: s/depends on iteration/depending on the iteration/
>>Typo: s/In case when/If/
>>>instruction (from from the stored table), we copy it as is, including
>>Typo: s/from from/from/
>>>loading type. But the ALOAD from the new table created (on the previous
>>Typo: s/loading/load/
>>>iteration) should have nil type, so the assertion in `fwd_ahload()` is
>>>failed.
>>>
>>>This patch replaces the assertion to `return 0` to avoid the assertion
>>Typo: s/assertion to/assertion with/
>>>failure and stop forwarding in such situations.
>>>
>>>But the emitted type-guarded ALOAD will lead to the persistent failure
>>>of the assertion guard on the trace. Hence, another one fix should be
>>Typo: s/another one/another/
>>>added. Also, TDUP IR is affected, too.
>>>See also the issue mentioned in the test.
>>>
>>>Sergey Kaplun:
>>>* added the description and the test for the problem
>>>
>>>Part of tarantool/tarantool#8516
>>>---
>>>
>>>Branch:  https://github.com/tarantool/luajit/tree/skaplun/lj-994-instable-types-during-loop-unroll
>>>PR:  https://github.com/tarantool/tarantool/pull/8642
>>>Related issues:
>>>*  https://github.com/tarantool/tarantool/issues/8516
>>>*  https://github.com/LuaJIT/LuaJIT/issues/994
>>>
>>>I don't mention the 994 intentionally to avoid Mike bothering. Also, it
>>>isn't the origin of this commit. Quite the opposite: as a result of this
>>>backporting the following issue was created.
>>>
>>>I prefer to backport the patches (this one and the prospective for 994)
>>>separately. So, after this patch is backported, it doesn't add any
>>>critical bugs (always failing guard just creates a new side trace), but
>>>helps to avoid conflicts for future backporting (sadly remembered
>>>"Improve assertions.").
>>>
>>> src/lj_opt_mem.c | 3 +-
>>> ...instable-types-during-loop-unroll.test.lua | 53 +++++++++++++++++++
>>> 2 files changed, 55 insertions(+), 1 deletion(-)
>>> create mode 100644 test/tarantool-tests/lj-994-instable-types-during-loop-unroll.test.lua
>>>
>>>diff --git a/src/lj_opt_mem.c b/src/lj_opt_mem.c
>>>index cc177d39..c8265b4f 100644
>>>--- a/src/lj_opt_mem.c
>>>+++ b/src/lj_opt_mem.c
>>>@@ -180,7 +180,8 @@ static TRef fwd_ahload(jit_State *J, IRRef xref)
>>>  }
>>>  ref = store->prev;
>>>       }
>>>- lua_assert(ir->o != IR_TNEW || irt_isnil(fins->t));
>>>+ 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)) ||
>>>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
>>>new file mode 100644
>>>index 00000000..435f6e0e
>>>--- /dev/null
>>>+++ b/test/tarantool-tests/lj-994-instable-types-during-loop-unroll.test.lua
>>>@@ -0,0 +1,53 @@
>>>+local tap = require('tap')
>>>+
>>>+local test = tap.test('lj-994-instable-types-during-loop-unroll'):skipcond({
>>>+ ['Test requires JIT enabled'] = not jit.status(),
>>>+})
>>>+
>>>+-- Test file to demonstrate LuaJIT misbehaviour during loop
>>>+-- unrolling and load forwarding for newly created tables.
>>>+-- See also  https://github.com/LuaJIT/LuaJIT/issues/994 .
>>>+
>>>+-- 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)
>>>+
>>>+-- TNEW.
>>>+local result
>>>+local stored_tab = {1}
>>>+local slot = {}
>>>+local key = 1
>>>+
>>>+jit.opt.start('hotloop=1')
>>>+-- The trouble happens during loop unrolling when we copy
>>>+-- `>+ num ALOAD` IR in the context of the table on the previous
>>>+-- iteration instead of a new one created via TNEW containing no
>>>+-- values (so type nil should be used instead of num).
>>>+for _ = 1, 5 do
>>>+ local t = slot
>>>+ -- Use non-constant key to avoid LJ_TRERR_GFAIL and undoing the
>>>+ -- loop.
>>Typo: s/Use/Use a/
>>>+ result = t[key]
>>>+ -- Swap table loaded by SLOAD to the created via TNEW.
>>>+ slot = _ % 2 ~= 0 and stored_tab or {}
>>>+end
>>>+test:is(result, nil, 'TNEW load forwarding was successful')
>>Nit: not sure about that assertion. AFAICS, we want to check whether we
>>have failed on assertion and not the validity of `result` variable. Feel free
>>to ignore.
>>>+
>>>+-- TDUP.
>>>+--[[
>>>+-- FIXME: Disable test case for now. Enable, with another one
>>Typo: s/another one/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.
>>>+ -- LJ_TRERR_GFAIL isn't triggered here.
>>>+ -- See `fwd_ahload()` in <src/lj_opt_mem.c> for details.
>>>+ result = t[1]
>>>+ slot = _ % 2 ~= 0 and stored_tab or {true}
>>>+end
>>>+test:is(result, true, 'TDUP load forwarding was successful')
>>>+]]
>>Nit: Not sure if it is good to add this test here. Maybe it is better
>>to create a ticket in our repo and move that chunk there. Feel free
>>to ignore.
>>>+
>>>+os.exit(test:check() and 0 or 1)
>>>--
>>>2.34.1
>>--
>>Best regards,
>>Maxim Kokryashkin
>> 
 

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

Re: [Tarantool-patches] [PATCH luajit] Fix TNEW load forwarding with instable types.

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the review!

On 15.05.23, Maxim Kokryashkin wrote:
> 
> Hi, Sergey!
> Thanks for the patch!
> LGTM, except for several nits below.
>  
> > 
> >>From: Mike Pall <mike>
> >>
> >>(cherry picked from commit d4c0c6e17ef7edf1f2893bc807746b80612e63e9)
> >>
> >>During loop unrolling IR instructions are coped, substituted and
> >>re-emitted for the FOLD/CSE/etc. pipeline. Assume, we have the following
> >>loop-carried dependency: we create a new table or load some stored one
> >>depends on iteration number. In case when we copy the emitted ALOAD IR
> >Typo: s/depends on iteration/depending on the iteration/
> >Typo: s/In case when/If/
> >>instruction (from from the stored table), we copy it as is, including
> >Typo: s/from from/from/
> >>loading type. But the ALOAD from the new table created (on the previous
> >Typo: s/loading/load/
> >>iteration) should have nil type, so the assertion in `fwd_ahload()` is
> >>failed.
> >>
> >>This patch replaces the assertion to `return 0` to avoid the assertion
> >Typo: s/assertion to/assertion with/
> >>failure and stop forwarding in such situations.
> >>
> >>But the emitted type-guarded ALOAD will lead to the persistent failure
> >>of the assertion guard on the trace. Hence, another one fix should be
> >Typo: s/another one/another/
> >>added. Also, TDUP IR is affected, too.
> >>See also the issue mentioned in the test.

Thanks for the comments, fixed, branch is force-pushed.
The new commit message is the following:

| Fix TNEW load forwarding with instable types.
|
| (cherry picked from commit d4c0c6e17ef7edf1f2893bc807746b80612e63e9)
|
| During loop unrolling IR instructions are copied, substituted and
| re-emitted for the FOLD/CSE/etc. pipeline. Assume, we have the following
| loop-carried dependency: we create a new table or load some stored one
| depends on the iteration number. If we copy the emitted ALOAD IR
| instruction (from the stored table), we copy it as is, including load
| type. But the ALOAD from the new table created (on the previous
| iteration) should have nil type, so the assertion in `fwd_ahload()` is
| failed.
|
| This patch replaces the assertion with `return 0` to avoid the assertion
| failure and stop forwarding in such situations.
|
| But the emitted type-guarded ALOAD will lead to the persistent failure
| of the assertion guard on the trace. Hence, another fix should be
| added. Also, TDUP IR is affected, too.
| See also the issue mentioned in the test.
|
| Sergey Kaplun:
| * added the description and the test for the problem
|
| Part of tarantool/tarantool#8516

> >>
> >>Sergey Kaplun:
> >>* added the description and the test for the problem
> >>
> >>Part of tarantool/tarantool#8516
> >>---
> >>
> >>Branch:  https://github.com/tarantool/luajit/tree/skaplun/lj-994-instable-types-during-loop-unroll
> >>PR:  https://github.com/tarantool/tarantool/pull/8642
> >>Related issues:
> >>*  https://github.com/tarantool/tarantool/issues/8516
> >>*  https://github.com/LuaJIT/LuaJIT/issues/994
> >>
> >>I don't mention the 994 intentionally to avoid Mike bothering. Also, it
> >>isn't the origin of this commit. Quite the opposite: as a result of this
> >>backporting the following issue was created.
> >>
> >>I prefer to backport the patches (this one and the prospective for 994)
> >>separately. So, after this patch is backported, it doesn't add any
> >>critical bugs (always failing guard just creates a new side trace), but
> >>helps to avoid conflicts for future backporting (sadly remembered
> >>"Improve assertions.").
> >>
> >> src/lj_opt_mem.c | 3 +-
> >> ...instable-types-during-loop-unroll.test.lua | 53 +++++++++++++++++++
> >> 2 files changed, 55 insertions(+), 1 deletion(-)
> >> create mode 100644 test/tarantool-tests/lj-994-instable-types-during-loop-unroll.test.lua
> >>
> >>diff --git a/src/lj_opt_mem.c b/src/lj_opt_mem.c
> >>index cc177d39..c8265b4f 100644
> >>--- a/src/lj_opt_mem.c
> >>+++ b/src/lj_opt_mem.c

<snipped>

> >>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
> >>new file mode 100644
> >>index 00000000..435f6e0e
> >>--- /dev/null
> >>+++ b/test/tarantool-tests/lj-994-instable-types-during-loop-unroll.test.lua

<snipped>

> >>+ -- Use non-constant key to avoid LJ_TRERR_GFAIL and undoing the
> >>+ -- loop.
> >Typo: s/Use/Use a/

Fixed.

> >>+ result = t[key]
> >>+ -- Swap table loaded by SLOAD to the created via TNEW.
> >>+ slot = _ % 2 ~= 0 and stored_tab or {}
> >>+end
> >>+test:is(result, nil, 'TNEW load forwarding was successful')
> >Nit: not sure about that assertion. AFAICS, we want to check whether we
> >have failed on assertion and not the validity of `result` variable. Feel free
> >to ignore.

I want to verify that behaviour isn't changed by JIT, so a leave this
check as is.
Ignoring.

> >>+
> >>+-- TDUP.
> >>+--[[
> >>+-- FIXME: Disable test case for now. Enable, with another one
> >Typo: s/another one/another/

Fixed, thanks!

> >>+-- backported commit with a fix for the aforementioned issue

<snipped>

> >>+test:is(result, true, 'TDUP load forwarding was successful')
> >>+]]
> >Nit: Not sure if it is good to add this test here. Maybe it is better
> >to create a ticket in our repo and move that chunk there. Feel free
> >to ignore.

Me, too, but I'll prefer to place these tests in one test file, since it
is the general issue for TDUP|TNEW IR-s. Also, reference the single
ticket.
Ignoring.

> >>+
> >>+os.exit(test:check() and 0 or 1)
> >>--
> >>2.34.1
> >--
> >Best regards,
> >Maxim Kokryashkin
> > 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit] Fix TNEW load forwarding with instable types.

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for the patch! LGTM, considering the fixes made for the comments
left by Max.

On 11.05.23, Sergey Kaplun via Tarantool-patches wrote:
> From: Mike Pall <mike>
> 
> (cherry picked from commit d4c0c6e17ef7edf1f2893bc807746b80612e63e9)
> 
> During loop unrolling IR instructions are coped, substituted and
> re-emitted for the FOLD/CSE/etc. pipeline. Assume, we have the following
> loop-carried dependency: we create a new table or load some stored one
> depends on iteration number. In case when we copy the emitted ALOAD IR

Side note: Changed the part after the colon the following way
| we create a new table or load the stored one depending on the
| iteration number.

> instruction (from from the stored table), we copy it as is, including
> loading type. But the ALOAD from the new table created (on the previous
> iteration) should have nil type, so the assertion in `fwd_ahload()` is
> failed.
> 
> This patch replaces the assertion to `return 0` to avoid the assertion
> failure and stop forwarding in such situations.
> 
> But the emitted type-guarded ALOAD will lead to the persistent failure
> of the assertion guard on the trace. Hence, another one fix should be

Side note: Removed this sentence, as we discussed.

> added. Also, TDUP IR is affected, too.
> See also the issue mentioned in the test.
> 
> Sergey Kaplun:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#8516
> ---
> 
> Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-994-instable-types-during-loop-unroll
> PR: https://github.com/tarantool/tarantool/pull/8642
> Related issues:
> * https://github.com/tarantool/tarantool/issues/8516
> * https://github.com/LuaJIT/LuaJIT/issues/994
> 
> I don't mention the 994 intentionally to avoid Mike bothering. Also, it
> isn't the origin of this commit. Quite the opposite: as a result of this
> backporting the following issue was created.
> 
> I prefer to backport the patches (this one and the prospective for 994)
> separately. So, after this patch is backported, it doesn't add any
> critical bugs (always failing guard just creates a new side trace), but
> helps to avoid conflicts for future backporting (sadly remembered
> "Improve assertions.").
> 
>  src/lj_opt_mem.c                              |  3 +-
>  ...instable-types-during-loop-unroll.test.lua | 53 +++++++++++++++++++
>  2 files changed, 55 insertions(+), 1 deletion(-)
>  create mode 100644 test/tarantool-tests/lj-994-instable-types-during-loop-unroll.test.lua
> 

<snipped>

> -- 
> 2.34.1
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit] Fix TNEW load forwarding with instable types.

[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 11.05.23, Sergey Kaplun via Tarantool-patches wrote:
> From: Mike Pall <mike>
> 
> (cherry picked from commit d4c0c6e17ef7edf1f2893bc807746b80612e63e9)
> 
> During loop unrolling IR instructions are coped, substituted and
> re-emitted for the FOLD/CSE/etc. pipeline. Assume, we have the following
> loop-carried dependency: we create a new table or load some stored one
> depends on iteration number. In case when we copy the emitted ALOAD IR
> instruction (from from the stored table), we copy it as is, including
> loading type. But the ALOAD from the new table created (on the previous
> iteration) should have nil type, so the assertion in `fwd_ahload()` is
> failed.
> 
> This patch replaces the assertion to `return 0` to avoid the assertion
> failure and stop forwarding in such situations.
> 
> But the emitted type-guarded ALOAD will lead to the persistent failure
> of the assertion guard on the trace. Hence, another one fix should be
> added. Also, TDUP IR is affected, too.
> See also the issue mentioned in the test.
> 
> Sergey Kaplun:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#8516
> ---
> 
> Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-994-instable-types-during-loop-unroll
> PR: https://github.com/tarantool/tarantool/pull/8642
> Related issues:
> * https://github.com/tarantool/tarantool/issues/8516
> * https://github.com/LuaJIT/LuaJIT/issues/994
> 
> I don't mention the 994 intentionally to avoid Mike bothering. Also, it
> isn't the origin of this commit. Quite the opposite: as a result of this
> backporting the following issue was created.
> 
> I prefer to backport the patches (this one and the prospective for 994)
> separately. So, after this patch is backported, it doesn't add any
> critical bugs (always failing guard just creates a new side trace), but
> helps to avoid conflicts for future backporting (sadly remembered
> "Improve assertions.").
> 
>  src/lj_opt_mem.c                              |  3 +-
>  ...instable-types-during-loop-unroll.test.lua | 53 +++++++++++++++++++
>  2 files changed, 55 insertions(+), 1 deletion(-)
>  create mode 100644 test/tarantool-tests/lj-994-instable-types-during-loop-unroll.test.lua
> 

<snipped>

> -- 
> 2.34.1
> 

-- 
Best regards,
IM