From: Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Igor Munkin <imun@tarantool.org>, Sergey Bronnikov <sergeyb@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: [Tarantool-patches] [PATCH luajit 17/19] MIPS64: Fix register allocation in assembly of HREF. Date: Wed, 9 Aug 2023 18:36:06 +0300 [thread overview] Message-ID: <37c2435a3529beb36c2e428f9c8e8b5c007c68e7.1691592488.git.skaplun@tarantool.org> (raw) In-Reply-To: <cover.1691592488.git.skaplun@tarantool.org> From: Mike Pall <mike> Contributed by James Cowgill. (cherry-picked from commit 99cdfbf6a1e8856f64908072ef10443a7eab14f2) The issue is observed for the following merged IRs: | p64 HREF 0001 "a" ; or other keys | > p64 EQ 0002 [0x4002d0c528] ; nilnode Sometimes, when we need to rematerialize a constant during evicting of the register. So, the instruction related to constant rematerialization is placed in the delay branch slot, which suppose to contain the loads of trace exit number to the `$ra` register. The resulting assembly is the following (for example): | beq ra, r1, 0x400abee9b0 ->exit | lui r1, 65531 ; delay slot without setting of the `ra` This leading to the assertion failure during trace exit in `lj_trace_exit()`, since a trace number is incorrect. This patch moves the constant register allocations above the main instruction emitting code in `asm_href()`. Sergey Kaplun: * added the description and the test for the problem Part of tarantool/tarantool#8825 --- src/lj_asm_mips.h | 42 +++++--- ...-mips64-href-delay-slot-side-exit.test.lua | 101 ++++++++++++++++++ 2 files changed, 126 insertions(+), 17 deletions(-) create mode 100644 test/tarantool-tests/lj-362-mips64-href-delay-slot-side-exit.test.lua diff --git a/src/lj_asm_mips.h b/src/lj_asm_mips.h index c27d8413..23ffc3aa 100644 --- a/src/lj_asm_mips.h +++ b/src/lj_asm_mips.h @@ -859,6 +859,9 @@ static void asm_href(ASMState *as, IRIns *ir, IROp merge) Reg dest = ra_dest(as, ir, allow); Reg tab = ra_alloc1(as, ir->op1, rset_clear(allow, dest)); Reg key = RID_NONE, type = RID_NONE, tmpnum = RID_NONE, tmp1 = RID_TMP, tmp2; +#if LJ_64 + Reg cmp64 = RID_NONE; +#endif IRRef refkey = ir->op2; IRIns *irkey = IR(refkey); int isk = irref_isk(refkey); @@ -901,6 +904,26 @@ static void asm_href(ASMState *as, IRIns *ir, IROp merge) #endif tmp2 = ra_scratch(as, allow); rset_clear(allow, tmp2); +#if LJ_64 + if (LJ_SOFTFP || !irt_isnum(kt)) { + /* Allocate cmp64 register used for 64-bit comparisons */ + if (LJ_SOFTFP && irt_isnum(kt)) { + cmp64 = key; + } else if (!isk && irt_isaddr(kt)) { + cmp64 = tmp2; + } else { + int64_t k; + if (isk && irt_isaddr(kt)) { + k = ((int64_t)irt_toitype(irkey->t) << 47) | irkey[1].tv.u64; + } else { + lua_assert(irt_ispri(kt) && !irt_isnil(kt)); + k = ~((int64_t)~irt_toitype(ir->t) << 47); + } + cmp64 = ra_allock(as, k, allow); + rset_clear(allow, cmp64); + } + } +#endif /* Key not found in chain: jump to exit (if merged) or load niltv. */ l_end = emit_label(as); @@ -943,24 +966,9 @@ static void asm_href(ASMState *as, IRIns *ir, IROp merge) emit_dta(as, MIPSI_DSRA32, tmp1, tmp1, 15); emit_tg(as, MIPSI_DMTC1, tmp1, tmpnum); emit_tsi(as, MIPSI_LD, tmp1, dest, (int32_t)offsetof(Node, key.u64)); - } else if (LJ_SOFTFP && irt_isnum(kt)) { - emit_branch(as, MIPSI_BEQ, tmp1, key, l_end); - emit_tsi(as, MIPSI_LD, tmp1, dest, (int32_t)offsetof(Node, key.u64)); - } else if (irt_isaddr(kt)) { - Reg refk = tmp2; - if (isk) { - int64_t k = ((int64_t)irt_toitype(irkey->t) << 47) | irkey[1].tv.u64; - refk = ra_allock(as, k, allow); - rset_clear(allow, refk); - } - emit_branch(as, MIPSI_BEQ, tmp1, refk, l_end); - emit_tsi(as, MIPSI_LD, tmp1, dest, offsetof(Node, key)); } else { - Reg pri = ra_allock(as, ~((int64_t)~irt_toitype(ir->t) << 47), allow); - rset_clear(allow, pri); - lua_assert(irt_ispri(kt) && !irt_isnil(kt)); - emit_branch(as, MIPSI_BEQ, tmp1, pri, l_end); - emit_tsi(as, MIPSI_LD, tmp1, dest, offsetof(Node, key)); + emit_branch(as, MIPSI_BEQ, tmp1, cmp64, l_end); + emit_tsi(as, MIPSI_LD, tmp1, dest, (int32_t)offsetof(Node, key.u64)); } *l_loop = MIPSI_BNE | MIPSF_S(tmp1) | ((as->mcp-l_loop-1) & 0xffffu); if (!isk && irt_isaddr(kt)) { diff --git a/test/tarantool-tests/lj-362-mips64-href-delay-slot-side-exit.test.lua b/test/tarantool-tests/lj-362-mips64-href-delay-slot-side-exit.test.lua new file mode 100644 index 00000000..8c75e69c --- /dev/null +++ b/test/tarantool-tests/lj-362-mips64-href-delay-slot-side-exit.test.lua @@ -0,0 +1,101 @@ +local tap = require('tap') +-- Test file to demonstrate the incorrect JIT behaviour for HREF +-- IR compilation on mips64. +-- See also https://github.com/LuaJIT/LuaJIT/pull/362. +local test = tap.test('lj-362-mips64-href-delay-slot-side-exit'):skipcond({ + ['Test requires JIT enabled'] = not jit.status(), +}) + +test:plan(1) + +-- To reproduce the issue we need to compile a trace with +-- `IR_HREF`, with a lookup of constant hash key GC value. To +-- prevent an `IR_HREFK` to be emitted instead, we need a table +-- with a huge hash part. Delta of address between the start of +-- the hash part of the table and the current node to lookup must +-- be more than `(1024 * 64 - 1) * sizeof(Node)`. +-- See <src/lj_record.c>, for details. +-- XXX: This constant is well suited to prevent test to be flaky, +-- because the aforementioned delta is always large enough. +-- Also, this constant avoids table rehashing, when inserting new +-- keys. +local N_HASH_FIELDS = 2 ^ 16 + 2 ^ 15 + +-- XXX: don't set `hotexit` to prevent compilation of trace after +-- exiting the main test cycle. +jit.opt.start('hotloop=1') + +-- Don't use `table.new()`, here by intence -- this leads to the +-- allocation failure for the mcode memory, so traces are not +-- compiled. +local filled_tab = {} +-- Filling-up the table with GC values to minimize the amount of +-- hash collisions and increase delta between the start of the +-- hash part of the table and currently stored node. +for _ = 1, N_HASH_FIELDS do + filled_tab[1LL] = 1 +end + +-- luacheck: no unused +local tab_value_a +local tab_value_b +local tab_value_c +local tab_value_d +local tab_value_e +local tab_value_f +local tab_value_g +local tab_value_h +local tab_value_i + +-- The function for this trace has a bunch of the following IRs: +-- p64 HREF 0001 "a" ; or other keys +-- > p64 EQ 0002 [0x4002d0c528] ; nilnode +-- Sometimes, when we need to rematerialize a constant during +-- evicting of the register. So, the instruction related to +-- constant rematerialization is placed in the delay branch slot, +-- which suppose to contain the loads of trace exit number to the +-- `$ra` register. This leading to the assertion failure during +-- trace exit in `lj_trace_exit()`, since a trace number is +-- incorrect. The amount of the side exit to check is empirical +-- (even a little bit more, than necessary just in case). +local function href_const(tab) + tab_value_a = tab.a + tab_value_b = tab.b + tab_value_c = tab.c + tab_value_d = tab.d + tab_value_e = tab.e + tab_value_f = tab.f + tab_value_g = tab.g + tab_value_h = tab.h + tab_value_i = tab.i +end + +-- Compile main trace first. +href_const(filled_tab) +href_const(filled_tab) + +-- Now brute-force side exits to check that they are compiled +-- correct. Take side exits in the reverse order to take a new +-- side exit each time. +filled_tab.i = 'i' +href_const(filled_tab) +filled_tab.h = 'h' +href_const(filled_tab) +filled_tab.g = 'g' +href_const(filled_tab) +filled_tab.f = 'f' +href_const(filled_tab) +filled_tab.e = 'e' +href_const(filled_tab) +filled_tab.d = 'd' +href_const(filled_tab) +filled_tab.c = 'c' +href_const(filled_tab) +filled_tab.b = 'b' +href_const(filled_tab) +filled_tab.a = 'a' +href_const(filled_tab) + +test:ok(true, 'no assertion failures during trace exits') + +test:done(true) -- 2.41.0
next prev parent reply other threads:[~2023-08-09 15:49 UTC|newest] Thread overview: 97+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-08-09 15:35 [Tarantool-patches] [PATCH luajit 00/19] Prerequisites for improve assertions Sergey Kaplun via Tarantool-patches 2023-08-09 15:35 ` [Tarantool-patches] [PATCH luajit 01/19] MIPS: Use precise search for exit jump patching Sergey Kaplun via Tarantool-patches 2023-08-15 9:36 ` Maxim Kokryashkin via Tarantool-patches 2023-08-16 12:40 ` Sergey Kaplun via Tarantool-patches 2023-08-16 13:25 ` Sergey Bronnikov via Tarantool-patches 2023-08-09 15:35 ` [Tarantool-patches] [PATCH luajit 02/19] test: introduce mcode generator for tests Sergey Kaplun via Tarantool-patches 2023-08-15 10:14 ` Maxim Kokryashkin via Tarantool-patches 2023-08-16 12:55 ` Sergey Kaplun via Tarantool-patches 2023-08-16 13:06 ` Maxim Kokryashkin via Tarantool-patches 2023-08-16 14:32 ` Sergey Bronnikov via Tarantool-patches 2023-08-16 15:20 ` Sergey Kaplun via Tarantool-patches 2023-08-16 16:08 ` Sergey Bronnikov via Tarantool-patches 2023-08-09 15:35 ` [Tarantool-patches] [PATCH luajit 03/19] MIPS: Fix handling of spare long-range jump slots Sergey Kaplun via Tarantool-patches 2023-08-15 11:13 ` Maxim Kokryashkin via Tarantool-patches 2023-08-16 13:05 ` Sergey Kaplun via Tarantool-patches 2023-08-16 15:02 ` Sergey Bronnikov via Tarantool-patches 2023-08-16 15:32 ` Sergey Kaplun via Tarantool-patches 2023-08-16 16:08 ` Sergey Bronnikov via Tarantool-patches 2023-08-09 15:35 ` [Tarantool-patches] [PATCH luajit 04/19] MIPS64: Add soft-float support to JIT compiler backend Sergey Kaplun via Tarantool-patches 2023-08-15 11:27 ` Maxim Kokryashkin via Tarantool-patches 2023-08-16 13:10 ` Sergey Kaplun via Tarantool-patches 2023-08-16 16:07 ` Sergey Bronnikov via Tarantool-patches 2023-08-09 15:35 ` [Tarantool-patches] [PATCH luajit 05/19] PPC: Add soft-float support to interpreter Sergey Kaplun via Tarantool-patches 2023-08-15 11:40 ` Maxim Kokryashkin via Tarantool-patches 2023-08-16 13:13 ` Sergey Kaplun via Tarantool-patches 2023-08-17 14:53 ` Sergey Bronnikov via Tarantool-patches 2023-08-09 15:35 ` [Tarantool-patches] [PATCH luajit 06/19] PPC: Add soft-float support to JIT compiler backend Sergey Kaplun via Tarantool-patches 2023-08-15 11:46 ` Maxim Kokryashkin via Tarantool-patches 2023-08-16 13:21 ` Sergey Kaplun via Tarantool-patches 2023-08-17 14:33 ` Sergey Bronnikov via Tarantool-patches 2023-08-09 15:35 ` [Tarantool-patches] [PATCH luajit 07/19] build: fix non-Linux/macOS builds Sergey Kaplun via Tarantool-patches 2023-08-15 11:58 ` Maxim Kokryashkin via Tarantool-patches 2023-08-16 13:40 ` Sergey Kaplun via Tarantool-patches 2023-08-17 14:31 ` Sergey Bronnikov via Tarantool-patches 2023-08-09 15:35 ` [Tarantool-patches] [PATCH luajit 08/19] Windows: Add UWP support, part 1 Sergey Kaplun via Tarantool-patches 2023-08-15 12:09 ` Maxim Kokryashkin via Tarantool-patches 2023-08-16 13:50 ` Sergey Kaplun via Tarantool-patches 2023-08-16 16:40 ` Sergey Bronnikov via Tarantool-patches 2023-08-09 15:35 ` [Tarantool-patches] [PATCH luajit 09/19] FFI: Eliminate hardcoded string hashes Sergey Kaplun via Tarantool-patches 2023-08-15 13:07 ` Maxim Kokryashkin via Tarantool-patches 2023-08-16 13:52 ` Sergey Kaplun via Tarantool-patches 2023-08-16 17:04 ` Sergey Bronnikov via Tarantool-patches 2023-08-09 15:35 ` [Tarantool-patches] [PATCH luajit 10/19] Cleanup math function compilation and fix inconsistencies Sergey Kaplun via Tarantool-patches 2023-08-11 8:06 ` Sergey Kaplun via Tarantool-patches 2023-08-15 13:10 ` Maxim Kokryashkin via Tarantool-patches 2023-08-16 17:15 ` Sergey Bronnikov via Tarantool-patches 2023-08-09 15:36 ` [Tarantool-patches] [PATCH luajit 11/19] Fix GCC 7 -Wimplicit-fallthrough warnings Sergey Kaplun via Tarantool-patches 2023-08-15 13:17 ` Maxim Kokryashkin via Tarantool-patches 2023-08-16 13:59 ` Sergey Kaplun via Tarantool-patches 2023-08-17 7:37 ` Sergey Bronnikov via Tarantool-patches 2023-08-09 15:36 ` [Tarantool-patches] [PATCH luajit 12/19] DynASM: Fix warning Sergey Kaplun via Tarantool-patches 2023-08-15 13:21 ` Maxim Kokryashkin via Tarantool-patches 2023-08-16 14:01 ` Sergey Kaplun via Tarantool-patches 2023-08-17 7:39 ` Sergey Bronnikov via Tarantool-patches 2023-08-17 7:51 ` Sergey Bronnikov via Tarantool-patches 2023-08-17 7:58 ` Sergey Kaplun via Tarantool-patches 2023-08-09 15:36 ` [Tarantool-patches] [PATCH luajit 13/19] ARM: Fix GCC 7 -Wimplicit-fallthrough warnings Sergey Kaplun via Tarantool-patches 2023-08-15 13:25 ` Maxim Kokryashkin via Tarantool-patches 2023-08-16 14:08 ` Sergey Kaplun via Tarantool-patches 2023-08-17 7:44 ` Sergey Bronnikov via Tarantool-patches 2023-08-17 8:01 ` Sergey Kaplun via Tarantool-patches 2023-08-09 15:36 ` [Tarantool-patches] [PATCH luajit 14/19] Fix debug.getinfo() argument check Sergey Kaplun via Tarantool-patches 2023-08-15 13:35 ` Maxim Kokryashkin via Tarantool-patches 2023-08-16 14:20 ` Sergey Kaplun via Tarantool-patches 2023-08-16 20:13 ` Maxim Kokryashkin via Tarantool-patches 2023-08-17 8:29 ` Sergey Bronnikov via Tarantool-patches 2023-08-09 15:36 ` [Tarantool-patches] [PATCH luajit 15/19] Fix LJ_MAX_JSLOTS assertion in rec_check_slots() Sergey Kaplun via Tarantool-patches 2023-08-15 14:07 ` Maxim Kokryashkin via Tarantool-patches 2023-08-16 14:22 ` Sergey Kaplun via Tarantool-patches 2023-08-17 8:57 ` Sergey Bronnikov via Tarantool-patches 2023-08-17 8:57 ` Sergey Kaplun via Tarantool-patches 2023-08-09 15:36 ` [Tarantool-patches] [PATCH luajit 16/19] Prevent integer overflow while parsing long strings Sergey Kaplun via Tarantool-patches 2023-08-15 14:38 ` Maxim Kokryashkin via Tarantool-patches 2023-08-16 14:52 ` Sergey Kaplun via Tarantool-patches 2023-08-17 10:53 ` Sergey Bronnikov via Tarantool-patches 2023-08-17 13:57 ` Sergey Kaplun via Tarantool-patches 2023-08-17 14:28 ` Sergey Bronnikov via Tarantool-patches 2023-08-09 15:36 ` Sergey Kaplun via Tarantool-patches [this message] 2023-08-16 9:01 ` [Tarantool-patches] [PATCH luajit 17/19] MIPS64: Fix register allocation in assembly of HREF Maxim Kokryashkin via Tarantool-patches 2023-08-16 15:17 ` Sergey Kaplun via Tarantool-patches 2023-08-16 20:14 ` Maxim Kokryashkin via Tarantool-patches 2023-08-17 11:06 ` Sergey Bronnikov via Tarantool-patches 2023-08-17 13:50 ` Sergey Kaplun via Tarantool-patches 2023-08-17 14:30 ` Sergey Bronnikov via Tarantool-patches 2023-08-09 15:36 ` [Tarantool-patches] [PATCH luajit 18/19] DynASM/MIPS: Fix shadowed variable Sergey Kaplun via Tarantool-patches 2023-08-16 9:03 ` Maxim Kokryashkin via Tarantool-patches 2023-08-16 15:22 ` Sergey Kaplun via Tarantool-patches 2023-08-17 12:01 ` Sergey Bronnikov via Tarantool-patches 2023-08-09 15:36 ` [Tarantool-patches] [PATCH luajit 19/19] MIPS: Add MIPS64 R6 port Sergey Kaplun via Tarantool-patches 2023-08-16 9:16 ` Maxim Kokryashkin via Tarantool-patches 2023-08-16 15:24 ` Sergey Kaplun via Tarantool-patches 2023-08-17 13:03 ` Sergey Bronnikov via Tarantool-patches 2023-08-17 13:59 ` Sergey Kaplun via Tarantool-patches 2023-08-16 15:35 ` [Tarantool-patches] [PATCH luajit 00/19] Prerequisites for improve assertions Sergey Kaplun via Tarantool-patches 2023-08-17 14:06 ` Maxim Kokryashkin via Tarantool-patches 2023-08-17 14:38 ` Sergey Bronnikov via Tarantool-patches 2023-08-31 15:17 ` 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=37c2435a3529beb36c2e428f9c8e8b5c007c68e7.1691592488.git.skaplun@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=imun@tarantool.org \ --cc=sergeyb@tarantool.org \ --cc=skaplun@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH luajit 17/19] MIPS64: Fix register allocation in assembly of HREF.' \ /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