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 355CB6EC58; Mon, 2 Aug 2021 16:58:05 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 355CB6EC58 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1627912685; bh=XJFm1PFByXzpMQe5jPYbb+97c+q1BYOMZjCvbSPTd1k=; 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=tnQX6zPYO/PADGvspV06fftYGgyyTQ8PCI6PYQh5L0yspSA0UbPK3gf6HV0ooNLGx reYIkkU2MVSIsTldqaJXq86xOg1lXo19byWYJXID2itJSxftbhh93+DxvPRQn+92Xl C2FKzr8WdGgC1H6Gr0FSkiNL9QQ4Ey8Idl+3Ho18= Received: from smtpng2.i.mail.ru (smtpng2.i.mail.ru [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 32E876EC58 for ; Mon, 2 Aug 2021 16:58:04 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 32E876EC58 Received: by smtpng2.m.smailru.net with esmtpa (envelope-from ) id 1mAYSA-00086B-Tt; Mon, 02 Aug 2021 16:58:03 +0300 Date: Mon, 2 Aug 2021 16:34:25 +0300 To: Sergey Kaplun Message-ID: <20210802133425.GB27855@tarantool.org> References: <5fdb4899061156f0fb4c53027d55f93be3a24759.1627144350.git.imun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.10.1 (2018-07-13) X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD941C43E597735A9C351B198F4576AC7B20CA14D9DFB46B94A182A05F53808504009D302E1F66E5A107CBE5197533C48D28164C2C66695930E413CDAD195184FF1 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE70186A8DAAA7CBDB1EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006371A00CE7BDCE207EB8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D88D4F46242930406B2A03AB7C1AC38F2D117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC9FC99A4BA45EE8B4A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352026055571C92BF10F618001F51B5FD3F9D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6D0C9BB9AE6BD5D69089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A5F8BA1A8938C5C23DEEE9F13BFAB86194378AEC19A994A576D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA751B940EDA0DFB0535410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D347130F804358653A64F67A567BB5F05F48F145AA19CBDB8499301CC05F85EFAA11BD121DD44561BE11D7E09C32AA3244C5CAC6BD8F8CA9EC243B469373A1F199BBBA718C7E6A9E042927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj9N286KAyvN4+yqOIW62+9w== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5D275DBDE3A66F0C01B490A548757F2666A7C8D0F45F857DBFE9F1EFEE2F478337FB559BB5D741EB964C8C2C849690F8E70A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] Detect inconsistent renames even in the presence of sunk values. 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: Igor Munkin via Tarantool-patches Reply-To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Sergey, Thanks for your review! On 01.08.21, Sergey Kaplun wrote: > Hi, Igor! > Thanks for the patch! > > Is there no point to simplify the test -- we have 5 different traces, > when really need the only one (with RENAME between two possible jump > to fallback branches with restoration from snapshot)? Well... It was a tough issue to provide a stable reproducer to Mike. Reducing this one is a much more complex issue: we need to compile a trace for a loop with a rename emitted between two guards with the same exitno in a variant part and leave the compiled loop via the guard at the first iteration before the emitted RENAME. Sounds more complex than even the existing test, doesn't it? > Feel free to ignore. Ignoring. > > Side note: also, we should test that sunk optimization still works, > shouldn't we? Emm, nothing in the patch affects sink optimization per se, so if it's OK without the patch, it's still OK with it. > Feel free to ignore. Ignoring. > > Otherwise, LGTM. Added your tag: | Reviewed-by: Sergey Kaplun > > On 24.07.21, Igor Munkin wrote: > > From: Mike Pall > > > > Reported by Igor Munkin. > > > > (cherry picked from commit 33e3f4badfde8cd9c202cedd1f4ed9275bc92e7d) > > > > Side exits with the same exitno use the same snapshot for restoring > > guest stack values. This obliges all guards related to the particular > > snapshot use the same RegSP mapping for the values to be restored at the > > trace exit. RENAME emitted prior to the guard for the same snapshot > > leads to the aforementioned invariant violation. > > This sentence a little bit unclear to me: > > 1) Leads how? RENAME changes the effective register to be used prior to the particular snapshot[1]. Hence if RENAME is emitted between the guards with the same exitno, RegSP mapping is inconsistent for the former one. > 2) Do you mean it in terms of recording (i. e. the reverse instrucions > recording order) or not? The trace is recorded in a direct order, but *assembled* in a reverse order. I implies the latter one. > > > The easy way to save > > > the snapshot consistency is spilling the renamed IR reference, that is > > done in scope of . > > > > However, the previous implementation considers > > only the IR references explicitly mentioned in the snapshot. E.g. if > > there is a sunk[1] object to be restored at the trace exit, and the > > renamed reference is a *STORE to that object, the spill slot is not > > allocated. As a result an invalid value is stored while unsinking that > > object at all corresponding side exits prior to the emitted renaming. > > > > To handle also those IR references implicitly used in the snapshot, all > > non-constant and non-sunk references are added to the Bloom filter (it's > > worth to mention that two hash functions are used to reduce collisions > > for the cases when the number of IR references emitted between two > > different snapshots exceeds the filter size). New > > implementation tests whether the renamed IR reference is in the filter > > and forces a spill slot for it as a result. > > > > [1]: http://wiki.luajit.org/Allocation-Sinking-Optimization > > > > Igor Munkin: > > * added the description and the test for the problem > > > > Resolves tarantool/tarantool#5118 > > Follows up tarantool/tarantool#4252 > > > > Signed-off-by: Igor Munkin > > --- > > > > Related issues: > > * https://github.com/tarantool/tarantool/issues/5118 > > * https://github.com/tarantool/tarantool/issues/4252 > > * https://github.com/LuaJIT/LuaJIT/issues/584 > > Branch: https://github.com/tarantool/luajit/tree/imun/lj-584-bad-renames-for-sunk-values > > CI: https://github.com/tarantool/tarantool/commit/b35e2ee > > > > src/lj_asm.c | 25 ++++--- > > ...j-584-bad-renames-for-sunk-values.test.lua | 69 +++++++++++++++++++ > > 2 files changed, 81 insertions(+), 13 deletions(-) > > create mode 100644 test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua > > > > -- > > 2.25.0 > > > > -- > Best regards, > Sergey Kaplun [1]: http://wiki.luajit.org/SSA-IR-2.0#miscellaneous-ops -- Best regards, IM