From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 6282A58CD3E; Sun, 20 Aug 2023 15:13:46 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 6282A58CD3E DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1692533626; bh=EmXoWfIE0gxx66AnDId7bbV6QdNZETlqQhtA4fGHG9c=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=MvbjXWZFmOL/kqN7mhDtanZiMhM+eanRb7+AfQrKygtpp2K3ZnczZ62L086dVkdzP piRabTpZhqgYNNgfYVdp/r0gH+rr4XicBGcPbqwtst7WsumrWtfE7QM92E98stdO2A yY7hvnJSOeG0mG76fwpEzN4EPJ5Fn/HqE/RuuNN0= Received: from smtp32.i.mail.ru (smtp32.i.mail.ru [95.163.41.73]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 14C1257B48E for ; Sun, 20 Aug 2023 15:13:45 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 14C1257B48E Received: by smtp32.i.mail.ru with esmtpa (envelope-from ) id 1qXhJP-007U5A-37; Sun, 20 Aug 2023 15:13:44 +0300 Date: Sun, 20 Aug 2023 15:13:40 +0300 To: Sergey Kaplun Message-ID: References: <20230817144641.20088-1-skaplun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230817144641.20088-1-skaplun@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 78E4E2B564C1792B X-77F55803: 4F1203BC0FB41BD93C8852532D76B9E3B40810D8949158306E0746822E25C650182A05F5380850400EC30A79C2FBE4AE69FF694DEC63FFD78BD4262222A614FB8585966BA2981F47 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7F9D3BE5B596754B8C2099A533E45F2D0395957E7521B51C2CFCAF695D4D8E9FCEA1F7E6F0F101C6778DA827A17800CE75644E22E05AA81AE52120BFB3F63BC185F65E78799B30205C33C3ADAEA971F8E611E41BBFE2FEB2B20B3BAB3830D05E00B657C96125D682CA561AD23E0A79E36389733CBF5DBD5E913377AFFFEAFD269176DF2183F8FC7C0FE6165853F4646218941B15DA834481FCF19DD082D7633A0EF3E4896CB9E6436389733CBF5DBD5E9D5E8D9A59859A8B6957A4DEDD2346B42CC7F00164DA146DA6F5DAA56C3B73B237318B6A418E8EAB8D32BA5DBAC0009BE9E8FC8737B5C2249EDF998CB16CCEE6976E601842F6C81A12EF20D2F80756B5FB606B96278B59C4276E601842F6C81A127C277FBC8AE2E8B7FE970921847EE7F3AA81AA40904B5D99C9F4D5AE37F343AD1F44FA8B9022EA23BBE47FD9DD3FB595F5C1EE8F4F765FC72CEEB2601E22B093A03B725D353964BD2DCF9CF1F528DBC35872C767BF85DA227C277FBC8AE2E8B953A8A48A05D51F175ECD9A6C639B01B4E70A05D1297E1BBCB5012B2E24CD356 X-C1DE0DAB: 0D63561A33F958A5EB71617572E4E9940A5550BFE911499FFC7F46EFFC1FFB31F87CCE6106E1FC07E67D4AC08A07B9B065B78C30F681404DCB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF834357150CEEC5D65DA5F3BDBF02B983499CB8EBAF4FB664DD94AF4ED1386141E547CA128C9EF438D9FDDA0AEF943CB3B4B3EDF644C656DEEB11D83AE47454DFE48CAC7CA610320002C26D483E81D6BE64ACE4A408B72B61B0CA6F94E606A667A52EF62A646584F811BD90D3D42C882D43082AE146A756F3 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojGRTUr9c9PlkojlzFUZhaKg== X-Mailru-Sender: 0E9E14D9EC491FBA7D1E237993EE709E8277C6AB274E1F6F69FF694DEC63FFD749EBE2174CCC947904C9FB44FCBCE9EE92D99EB8CC7091A7ECEABDC5717908DEF544888E8238EB4872D6B4FCE48DF648AE208404248635DF X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] Fix TDUP load forwarding after table rehash. X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Maxim Kokryashkin via Tarantool-patches Reply-To: Maxim Kokryashkin Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Sergey! Thanks for the patch! LGTM, except for a few comments below. On Thu, Aug 17, 2023 at 05:46:41PM +0300, Sergey Kaplun wrote: > From: Mike Pall > > Reported by Sergey Kaplun. > > (cherry-picked from commit c7db8255e1eb59f933fac7bc9322f0e4f8ddc6e6) > > After table rehashing number keys loaded via ALOAD may be placed in the > hash part of the table. So, load forwarding analysis missed the > corresponding stores like they are never existed. In such case, either Typo: s/they are/they/ Typo: s/In such/In that/ > we faced an assertion failure in `fwd_ahload()` due to values types Typo: s/values/value/ > mismatch, either we faced an assertion failure in `rec_check_slots()` Typo: s/either/or/ > since forwarded value by the JIT compiler isn't the same as it is in the Typo: s/since/since the/ > interpreter. > > This patch adds a check that there is no any `IR_NEWREF` after table Typo: s/no any/no/ > creation, so it can't be rehashed. > > 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-980-load-fwd-after-table-rehash > PR: https://github.com/tarantool/tarantool/pull/8998 > Related issues: > * https://github.com/LuaJIT/LuaJIT/issues/980 > * https://github.com/tarantool/tarantool/issues/8825 > > src/lj_opt_mem.c | 6 + > ...j-980-load-fwd-after-table-rehash.test.lua | 166 ++++++++++++++++++ > 2 files changed, 172 insertions(+) > create mode 100644 test/tarantool-tests/lj-980-load-fwd-after-table-rehash.test.lua > > diff --git a/src/lj_opt_mem.c b/src/lj_opt_mem.c > index 59fddbdd..7b610506 100644 > --- a/src/lj_opt_mem.c > +++ b/src/lj_opt_mem.c > @@ -157,6 +157,7 @@ static TRef fwd_ahload(jit_State *J, IRRef xref) > if (ir->o == IR_TNEW || (ir->o == IR_TDUP && irref_isk(xr->op2))) { > /* A NEWREF with a number key may end up pointing to the array part. > ** But it's referenced from HSTORE and not found in the ASTORE chain. > + ** Or a NEWREF may rehash the table and move unrelated number keys. > ** For now simply consider this a conflict without forwarding anything. > */ > if (xr->o == IR_AREF) { > @@ -167,6 +168,11 @@ static TRef fwd_ahload(jit_State *J, IRRef xref) > goto cselim; > ref2 = newref->prev; > } > + } else { > + IRIns *key = IR(xr->op2); > + if (key->o == IR_KSLOT) key = IR(key->op1); > + if (irt_isnum(key->t) && J->chain[IR_NEWREF] > tab) > + goto cselim; > } > /* NEWREF inhibits CSE for HREF, and dependent FLOADs from HREFK/AREF. > ** But the above search for conflicting stores was limited by xref. > diff --git a/test/tarantool-tests/lj-980-load-fwd-after-table-rehash.test.lua b/test/tarantool-tests/lj-980-load-fwd-after-table-rehash.test.lua > new file mode 100644 > index 00000000..a27932df > --- /dev/null > +++ b/test/tarantool-tests/lj-980-load-fwd-after-table-rehash.test.lua > @@ -0,0 +1,166 @@ > +local tap = require('tap') > + > +-- Test file to demonstrate LuaJIT misbehaviour during load > +-- forwarding optimization for HLOAD after table rehashing. > +-- See also https://github.com/LuaJIT/LuaJIT/issues/980. > + > +local test = tap.test('lj-980-load-fwd-after-table-rehash'):skipcond({ > + ['Test requires JIT enabled'] = not jit.status(), > +}) > + > +test:plan(6) > + > +jit.opt.start('hotloop=1') > + > +local result > +-- The test for TNEW load forwarding. It doesn't trigger an assert > +-- since the commit "Fix TNEW load forwarding with instable > +-- types.". But still add it to avoid regressions in future. Typo: s/But still add it/But is still added/ Typo: s/in future/in the future/ > +for i = 6, 9 do > + -- Need big enough table to see rehashing. > + -- Also, to simplify logic with AREF, HREF don't use default > + -- 1, 4 (start, stop) values here. > + local t = {i, i, i, i, i, i, i} > + -- Insert via ASTORE. > + t[i] = i > + t[1] = nil > + t[2] = nil > + t[3] = nil > + t[4] = nil > + t[5] = nil > + -- Rehash table. Array part is empty. > + t['1000'] = 1000 > + -- Load via HLOAD. > + result = t[i] > +end > + > +test:is(result, 9, 'TNEW load forwarding') > + > +-- TNEW load forwarding, aliased table. The same label about this test being added only for the sake of regressional testing should be added here too. > +local alias_store = {{}, {}, {}, {}, {}} > +for i = 6, 9 do > + local t = {i, i, i, i, i, i, i} > + alias_store[#alias_store + 1] = t > + local alias = alias_store[i] > + -- Insert via ASTORE. > + alias[i] = i > + alias[1] = nil > + alias[2] = nil > + alias[3] = nil > + alias[4] = nil > + alias[5] = nil > + -- Rehash table. Array part is empty. > + alias['1000'] = 1000 > + -- Load via HLOAD. > + result = t[i] > +end > + > +test:is(result, 9, 'TNEW load forwarding, aliased table') > + > +local expected = 'result' > + > +-- TDUP different types. > +for i = 6, 9 do > + local t = {1, 2, 3, 4, 5, 6, 7, 8} > + t[i] = expected > + t[i + 1] = expected > + t[1] = nil > + t[2] = nil > + t[3] = nil > + t[4] = nil > + t[5] = nil > + t[6] = nil > + -- Rehash table. Array part is empty. > + t['1000'] = 1000 > + -- Result on the recording (i == 8) iteration is 'result'. > + -- Nevertheless, on the last (i == 9) iteration it is 8. > + -- Just check that there is no assert failure here. > + -- Load via HLOAD. > + result = t[8] > +end > + > +-- Checked for assertion guard, on the last iteration we get > +-- the value on initializatoin. > +test:is(result, 8, 'TDUP load forwarding different types') > + > +-- TDUP different types, aliased table. > +alias_store = {{}, {}, {}, {}, {}} > +for i = 6, 9 do > + local t = {1, 2, 3, 4, 5, 6, 7, 8} > + -- Store table, to be aliased later. > + alias_store[#alias_store + 1] = t > + local alias = alias_store[i] > + alias[i] = expected > + alias[i + 1] = expected > + alias[1] = nil > + alias[2] = nil > + alias[3] = nil > + alias[4] = nil > + alias[5] = nil > + alias[6] = nil > + -- Rehash table. Array part is empty. > + alias['1000'] = 1000 > + -- Result on the recording (i == 8) iteration is 'result'. > + -- Nevertheless, on the last (i == 9) iteration it is 8. > + -- Just check that there is no assert failure here. > + -- Load via HLOAD. > + result = t[8] > +end > + > +-- Checked for assertion guard, on the last iteration we get > +-- the value on initializatoin. > +test:is(result, 8, 'TDUP load forwarding different types, aliased table') > + > +-- TDUP same type, different values. > +for i = 6, 9 do > + local t = {1, 2, 3, 4, 5, 6, '7', '8'} > + t[i] = expected > + t[i + 1] = expected > + t[1] = nil > + t[2] = nil > + t[3] = nil > + t[4] = nil > + t[5] = nil > + t[6] = nil > + -- Rehash table. Array part is empty. > + t['1000'] = 1000 > + -- Result on the recording (i == 8) iteration is 'result'. > + -- Nevertheless, on the last (i == 9) iteration it is '8'. > + -- Just check that there is no assert failure here. > + -- Load via HLOAD. > + result = t[8] > +end > + > +-- Checked for assertion guard, on the last iteration we get > +-- the value on initializatoin. > +test:is(result, '8', 'TDUP load forwarding same type, different values') > + > +alias_store = {{}, {}, {}, {}, {}} > +for i = 6, 9 do > + local t = {1, 2, 3, 4, 5, 6, '7', '8'} > + -- Store table, to be aliased later. > + alias_store[#alias_store + 1] = t > + local alias = alias_store[i] > + alias[i] = expected > + alias[i + 1] = expected > + alias[1] = nil > + alias[2] = nil > + alias[3] = nil > + alias[4] = nil > + alias[5] = nil > + alias[6] = nil > + -- Rehash table. Array part is empty. > + alias['1000'] = 1000 > + -- Result on the recording (i == 8) iteration is 'result'. > + -- Nevertheless, on the last (i == 9) iteration it is '8'. > + -- Just check that there is no assert failure here. > + -- Load via HLOAD. > + result = t[8] > +end > + > +-- Checked for assertion guard, on the last iteration we get > +-- the value on initializatoin. > +test:is(result, '8', > + 'TDUP load forwarding same type, different values, aliased table') > + > +test:done(true) > -- > 2.41.0 >