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

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

[-- 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 --]