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 55A866EC40; Mon, 16 Aug 2021 19:52:29 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 55A866EC40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1629132749; bh=nLg9tc0LoHNR2o+Bw0SZRHue+LHL98eVBnvrte2+nag=; 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=sePj8MiUCwDK/Jtqsouh5LMFv2rFJ+fdIgZG3pDK4t1X2vz876TbkxAa0utmauL9M 1ikyKmLKz11wmt3rgo0hqQBuKrvQSfB3Ip28uu2jIOsxVvPpznEzxjT46ZTklwQnU7 3/tnVDPL+qxiD2SlfWv3BUiW5V6wpJJyksjOxvNY= Received: from smtpng1.i.mail.ru (smtpng1.i.mail.ru [94.100.181.251]) (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 CA2766EC40 for ; Mon, 16 Aug 2021 19:52:27 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org CA2766EC40 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1mFfqc-0001W3-OI; Mon, 16 Aug 2021 19:52:27 +0300 Date: Mon, 16 Aug 2021 19:27:15 +0300 To: Sergey Kaplun Message-ID: <20210816162715.GA5743@tarantool.org> References: <20210719073632.12008-1-skaplun@tarantool.org> <20210801104318.GZ27855@tarantool.org> <20210816072007.GR27855@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: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD92087353F0EC44DD9D5AC6413C25DCF08CC98B8FCC5CD86F3182A05F5380850404B1FA786FBC089E3EB731D0747CC83D1D4C6C555D520598D4F4A0749F3D738C3 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE75B37E0A1C175363BEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006373AE27A3DEAB6268F8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8ABEED0796ADD372006D9AFB9DE686612117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCEA77C8EAE1CE44B0A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F4460429728776938767073520B1593CA6EC85F86D2CC0D3CB04F14752D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6BF3059D42242344A089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A53A4749273249D8C1462C48B3688A9682EB2E9E0B86E045BAD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7567C209D01CC1E34B410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34AF12ADB97C97CD897FE5F6B5151627C8711D0A9DBDD304611534741EB3074AA47E551CF33F8E29D01D7E09C32AA3244C09D5CE2FB34E44777F3F99CE94B43C0A81560E2432555DBB927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojIrFL/N5KnVGwwx3aEYA+cQ== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5D9C48D90B802088981B497AEF332C6DB2A7C8D0F45F857DBFE9F1EFEE2F478337FB559BB5D741EB964C8C2C849690F8E70A04DAD6CC59E33667EA787935ED9F1B 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: 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 the fixes! LGTM now. On 16.08.21, Sergey Kaplun wrote: > Igor, > > Thanks for the review! > > See the new comment message above: > =================================================================== > Fix bytecode register allocation for comparisons. > > (cherry picked from commit 2f3f07882fb4ad9c64967d7088461b1ca0a25d3a) > > When LuaJIT is built with LJ_FR2 (e.g. with GC64 mode enabled), > information about frame takes two slots -- the first takes the TValue > with the function to be called, the second takes the framelink. The JIT > recording machinery does 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. This slot is > cleared either by return values or manually (set to zero), when there > are no values to return. The latter case is done by the next bytecode > with RA dst mode. This obliges that the destination of RA takes the next > slot after TREF_FRAME. Hence, this an earlier instruction must use the > smallest possible destination register (see `lj_record_ins()` for the > details). > > Bytecode emitter swaps operands for ISGT and ISGE comparisons. As a > result, the aforementioned rule for registers allocations may be > violated. When it happens for a chunk being recorded, the slot with > TREF_FRAME is not rewritten (but the next empty slot after TREF_FRAME > is). This leads to JIT slots inconsistency and assertion failure in > `rec_check_slots()` during recording of the next bytecode instruction. > > This patch fixes bytecode register allocation by changing the VM > register allocation order in case of ISGT and ISGE bytecodes. > > Sergey Kaplun: > * added the description and the test for the problem > > Resolves tarantool/tarantool#6227 > Part of tarantool/tarantool#5629 > =================================================================== > > On 16.08.21, Igor Munkin wrote: > > > > > Furthermore, what does stop you from using local variables? > > They occupy new slots and make it harder to maintain, see the new > comment below. Meh, OK anyway :) > > > > > =================================================================== > 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 > index 66f6885e..9788923a 100644 > --- a/test/tarantool-tests/gh-6227-bytecode-allocator-for-comparisons.test.lua > +++ b/test/tarantool-tests/gh-6227-bytecode-allocator-for-comparisons.test.lua > @@ -14,26 +14,39 @@ 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. > +-- `J->maxslot` is initialized with `nargs` (i.e. zero in this > +-- case) in `rec_call_setup()`. > local function bump_frame() > -- First call function with RET0 to set TREF_FRAME in the > -- last slot. > empty() > + -- The old bytecode to be recorded looks like the following: > + -- 0000 . FUNCF 4 > + -- 0001 . UGET 0 0 ; empty > + -- 0002 . CALL 0 1 1 > + -- 0000 . . JFUNCF 1 1 > + -- 0001 . . RET0 0 1 > + -- 0002 . CALL 0 1 1 > + -- 0003 . UGET 0 0 ; empty > + -- 0004 . UGET 3 1 ; uv > + -- 0005 . KSHORT 2 1 > + -- 0006 . ISLT 3 2 > -- 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()`. > + -- operands (consider ISLT above). > + -- Two calls of `empty()` function in a row is necessary for 2 > + -- slot gap in LJ_FR2 mode. > + -- Upvalue loads before KSHORT, so the difference between slot > + -- for upvalue `empty` (function to be called) and slot for > + -- upvalue `uv` is more than 2. Hence, TREF_FRAME slot is not > + -- rewritten by the bytecode after return from `empty()` > + -- function 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 > +for _ = 1, 3 do > bump_frame() > end > =================================================================== > > > -- > Best regards, > Sergey Kaplun -- Best regards, IM