From: Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Maxim Kokryashkin <m.kokryashkin@tarantool.org>, Sergey Bronnikov <sergeyb@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: [Tarantool-patches] [PATCH luajit] Fix TNEW load forwarding with instable types. Date: Thu, 11 May 2023 00:30:02 +0300 [thread overview] Message-ID: <20230510213002.5153-1-skaplun@tarantool.org> (raw) 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
next reply other threads:[~2023-05-10 21:34 UTC|newest] Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-05-10 21:30 Sergey Kaplun via Tarantool-patches [this message] 2023-05-15 10:30 ` Maxim Kokryashkin via Tarantool-patches 2023-05-15 10:35 ` Maxim Kokryashkin via Tarantool-patches 2023-05-18 20:55 ` Sergey Kaplun via Tarantool-patches 2023-07-03 19:24 ` Igor Munkin via Tarantool-patches 2023-07-04 17:09 ` 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=20230510213002.5153-1-skaplun@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] Fix TNEW load forwarding with instable types.' \ /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