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 538EA6EC58; Sun, 1 Aug 2021 20:11:42 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 538EA6EC58 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1627837902; bh=/dKL82CW+g7kq2qJCCsh/0H1a4jHAN5sObnTdX9mSL0=; 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=dUMxz57l1LbZBa7CmJKXRpyVvWqoB4pDSEamPtDB2zZ9YAp65lSwRUzSxdBlwE+oF QLj5NFmwj5lpnXhT77bRMDKLBTONvfYK2VmsacNPP+OidGDYf7jSw0jf6onAs7F+SY xAQOiX19wiQXLsAl6deVU30Ivcu/EOwXTWnNt3ec= Received: from smtp47.i.mail.ru (smtp47.i.mail.ru [94.100.177.107]) (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 846466EC58 for ; Sun, 1 Aug 2021 20:11:40 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 846466EC58 Received: by smtp47.i.mail.ru with esmtpa (envelope-from ) id 1mAEzz-0005FO-JG; Sun, 01 Aug 2021 20:11:40 +0300 Date: Sun, 1 Aug 2021 20:10:26 +0300 To: Igor Munkin Message-ID: References: <20210719073632.12008-1-skaplun@tarantool.org> <20210801104318.GZ27855@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210801104318.GZ27855@tarantool.org> X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD941C43E597735A9C354866C15C72ED952BE56FFA0EFAF5B8C182A05F538085040593E19FA4AA0D64B2133CF5563141076FAECD6B2BE9F0504212C1E374D303458 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7922D113DFDC6D5A3EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637BA14B6CBCF09CA1D8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8E112F9FD905AF37F5996389F290E0DC8117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC974A882099E279BDA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD186FD1C55BDD38FC3FD2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6A45692FFBBD75A6A089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A5C10C6ACA51DFF2FDAEF438E69A1F3CFCEB65C67579CED0D5D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA751B940EDA0DFB0535410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34E3E1DC0F9BD55309B179901459ECBD3D9AE41B8BB1044BDA38D81DA32882A578DBDD92589470DD0E1D7E09C32AA3244C719828AEAEC1EE48938BF21AD37D69B230363D8B7DA7DD44FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojMCfuYI4PredN6BoetJ+scQ== X-Mailru-Sender: 3B9A0136629DC91206CBC582EFEF4CB4922C546EDF1DAAB1E40DEA3F2AC2C2375BF527D83450CF42F2400F607609286E924004A7DEC283833C7120B22964430C52B393F8C72A41A89437F6177E88F7363CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] Fix bytecode register allocation for comparisons. 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: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Igor! Thanks for the review! On 01.08.21, Igor Munkin wrote: > Sergey, > > Thanks for the patch! Please consider the comments below. I didn't check > the test yet, since I don't get the JIT peculiarities from your commit > message and comments. Please provide a clearer description and I'll > proceed with the review of the test case then. > > On 19.07.21, Sergey Kaplun wrote: > > From: Mike Pall > > > > (cherry picked from commit 2f3f07882fb4ad9c64967d7088461b1ca0a25d3a) > > > > When LuaJIT is build with LJ_FR2 (GC64), information about frame takes > > two slots -- the first takes the TValue with the function to call, the > > second takes the additional frame information. The recording JIT > > Minor: The second slot is the framelink in LuaJIT terms. Yes, because it takes the additional frame information. How do you want to modify this line? > > > machinery works pretty the same -- the function IR_KGC is loaded in the > > first slot, and the second is set to TREF_FRAME value. This value > > should be rewritten after return from a callee. It is done either by the > > return values either this slot is cleared (set to zero) manually with > > the next bytecode with RA dst mode with the assumption, that the dst RA > > takes the next slot after TREF_FRAME, i.e. an earlier instruction uses > > the smallest possible destination register (see `lj_record_ins()` for > > the details). > > The main point lies in the monstrous 5-line sentence. I've read several > times, but still don't get it. Could you please reword it in a not such > complex sentence? The first option is rewrite this slot by return values from the function. The second option is clearing slot (i.e. set to zero) manually, when there is no values to return. It is done by the next bytecode having RA dst mode. This obliges that the destination of RA takes the next slot after TREF_FRAME. For this an earlier instruction must use the smallest possible destination register (see `lj_record_ins()` for the details). > > > > > Bytecode allocator swaps operands for ISGT and ISGE comparisons. > > When it happens, the aforementioned rule for registers allocations > > may be violated. When it happens, and this chunk is recording, the slot > > with TREF_FRAME is not rewritten (but the next empty slot after > > TREF_FRAME is) during bytecode recording. This leads to JIT slots > > inconsistency and assertion failure in `rec_check_slots()` during > > recording the next bytecode instruction. > > > > This patch fixes bytecode register allocation by changing the register > > allocation order in case of ISGT and ISGE bytecodes. > > It's better to use "virtual register" or even "VM register" to avoid > ambiguous plain "register" usage. Changed to VM register. > > > > > Sergey Kaplun: > > * added the description and the test for the problem > > > > Resolves tarantool/tarantool#6227 > > Minor: Why #5629 is not mentioned? Added. Branch is updated and force-pushed. > > > --- > > > > Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-6227-fix-bytecode-allocator-for-comp > > Tarantool branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-6227-fix-bytecode-allocator-for-comp > > Issue: https://github.com/tarantool/tarantool/issues/6227 > > > > src/lj_parse.c | 7 +++- > > ...ytecode-allocator-for-comparisons.test.lua | 41 +++++++++++++++++++ > > 2 files changed, 46 insertions(+), 2 deletions(-) > > create mode 100644 test/tarantool-tests/gh-6227-bytecode-allocator-for-comparisons.test.lua > > > > diff --git a/src/lj_parse.c b/src/lj_parse.c > > index 08f7cfa6..a6325a76 100644 > > --- a/src/lj_parse.c > > +++ b/src/lj_parse.c > > @@ -853,9 +853,12 @@ static void bcemit_comp(FuncState *fs, BinOpr opr, ExpDesc *e1, ExpDesc *e2) > > e1 = e2; e2 = eret; /* Swap operands. */ > > op = ((op-BC_ISLT)^3)+BC_ISLT; > > expr_toval(fs, e1); > > + ra = expr_toanyreg(fs, e1); > > + rd = expr_toanyreg(fs, e2); > > + } else { > > + rd = expr_toanyreg(fs, e2); > > + ra = expr_toanyreg(fs, e1); > > } > > - rd = expr_toanyreg(fs, e2); > > - ra = expr_toanyreg(fs, e1); > > ins = BCINS_AD(op, ra, rd); > > } > > /* Using expr_free might cause asserts if the order is wrong. */ > > diff --git a/test/tarantool-tests/gh-6227-bytecode-allocator-for-comparisons.test.lua b/test/tarantool-tests/gh-6227-bytecode-allocator-for-comparisons.test.lua > > new file mode 100644 > > index 00000000..66f6885e > > --- /dev/null > > +++ b/test/tarantool-tests/gh-6227-bytecode-allocator-for-comparisons.test.lua > > @@ -0,0 +1,41 @@ > > +local tap = require('tap') > > +local test = tap.test('gh-6227-bytecode-allocator-for-comparisons') > > +test:plan(1) > > + > > +-- Test file to demonstrate assertion failure during recording > > +-- wrong allocated bytecode for comparisons. > > +-- See also https://github.com/tarantool/tarantool/issues/6227. > > + > > +-- Need function with RET0 bytecode to avoid reset of > > +-- the first JIT slot with frame info. Also need no assignments > > +-- by the caller. > > +local function empty() end > > + > > +local uv = 0 > > + > > +-- This function needs to reset register enumerating. > > +-- Also set `J->maxslot` to zero. > > +-- The upvalue function to call is loaded to 0 slot. > > +local function bump_frame() > > + -- First call function with RET0 to set TREF_FRAME in the > > + -- last slot. > > + empty() > > + -- Test ISGE or ISGT bytecode. These bytecodes swap their > > + -- operands. Also, a constant is always loaded into the slot > > + -- smaller than upvalue. So, if upvalue loads before KSHORT, > > + -- then the difference between registers is more than 2 (2 is > > + -- needed for LJ_FR2) and TREF_FRAME slot is not rewriting by > > + -- the bytecode after call and return as expected. That leads > > + -- to recording slots inconsistency and assertion failure at > > + -- `rec_check_slots()`. > > + empty(1>uv) > > +end > > + > > +jit.opt.start('hotloop=1') > > + > > +for _ = 1,3 do > > + bump_frame() > > +end > > + > > +test:ok(true) > > +os.exit(test:check() and 0 or 1) > > -- > > 2.31.0 > > > > -- > Best regards, > IM -- Best regards, Sergey Kaplun