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 4F1B15B1635; Wed, 16 Aug 2023 16:10:30 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 4F1B15B1635 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1692191430; bh=iX5SzDLWq5f3mF3fBVjU4OCbEZxP6p4SfFgEJrrkQVw=; 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=xChNHlCVOpqSjC91zyXmhBzjKtqjk6My0uMvctpczGrZgHfx/qGa6AoaePstHhPdd dW3hKKPPqlRC2nw7iO4hIx8qMrAACQUi+7ae2GnrZwz/En57CjwyQGtqYfdiSuwMAz 8e7x6AoXL7fpn2v9uxtVbwSEvZUwKsJEXLiNA9VE= Received: from smtpng3.i.mail.ru (smtpng3.i.mail.ru [94.100.177.149]) (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 9339A5B1635 for ; Wed, 16 Aug 2023 16:10:28 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 9339A5B1635 Received: by smtpng3.m.smailru.net with esmtpa (envelope-from ) id 1qWGI7-0003mI-Ki; Wed, 16 Aug 2023 16:10:28 +0300 Date: Wed, 16 Aug 2023 16:05:40 +0300 To: Maxim Kokryashkin Message-ID: References: <7179245cf38c56a88bb8f3aa1bbeaf15402fcd1a.1691592488.git.skaplun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 78E4E2B564C1792B X-77F55803: 4F1203BC0FB41BD9700E0DCE2907754DC31F271F5537F259DF86C48FD8FFD89C182A05F538085040FC3F549F916ED2C18BF34E877846B94DF64A0E194CA1C619AC7338845154CCF4 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE70993CE289E4873FDEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006375D8840FA58F505298638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D839B1D5744CCBF1BC5C16B77BBC6E1383117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCB816BE3345416868389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC82FFDA4F57982C5F4F6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA7E827F84554CEF5019E625A9149C048EE9ECD01F8117BC8BEE2021AF6380DFAD18AA50765F79006378A4BC95AACA28A5322CA9DD8327EE4930A3850AC1BE2E735026D3A1080F4EF5CC4224003CC83647689D4C264860C145E X-C1DE0DAB: 0D63561A33F958A5CD363BD321EFB1BC4B5AE7F0E881BC3EC9BF39C978956D57F87CCE6106E1FC07E67D4AC08A07B9B01DAA61796BF5227BCB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D340DAE5B306C240CF596D772B5E0E6E3A51B8BC07FE0D1F026D50906C7420D90ADF828A255A7D7C5C21D7E09C32AA3244C88D2167298E6B18124FB7647B2B04FB239C99C45E8D137E9BAD658CF5C8AB4025DA084F8E80FEBD3202CD0F03380D9577A83BD0C44CE203720ABEDE4BBDD9CDD X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojHVl7ekwB6hhCHntNOmC5Zw== X-DA7885C5: EF36CF99FF21DEF4DE6FC67625E9A88341A837F81268DECDC829037230B450E6262E2D401490A4A0DB037EFA58388B346E8BC1A9835FDE71 X-Mailru-Sender: 689FA8AB762F73930F533AC2B33E986B0548C6578F69336C1335AAF36F11EDCB0FBE9A32752B8C9C2AA642CC12EC09F1FB559BB5D741EB962F61BD320559CF1EFD657A8799238ED55FEEDEB644C299C0ED14614B50AE0675 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 03/19] MIPS: Fix handling of spare long-range jump slots. 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, Maxim! Thanks for the review! Fixed your comments inline. On 15.08.23, Maxim Kokryashkin wrote: > Hi, Sergey! > Thanks for the patch! > LGTM, except for a few comments below. > On Wed, Aug 09, 2023 at 06:35:52PM +0300, Sergey Kaplun via Tarantool-patches wrote: > > From: Mike Pall > > > > Contributed by Djordje Kovacevic and Stefan Pejic. > > > > (cherry-picked from commit c7c3c4da432ddb543d4b0a9abbb245f11b26afd0) > > > > `asm_setup_jump()` in presumes that `sizeof(MCLink)` > > is 8 bytes, but for MIPS64 its size is 16 bytes. This leads to incorrect > Typo: s/to incorrect/to an incorrect/ Fixed. > > check in `asm_sparejump_setup()`, so mcode bottom is not updated. > Typo: s/so mcode/so the mcode/ Fixed. > > > > This patch fixes check of the MCLink offset from the mcbot. > Typo: s/fixes check/fixes the check/ Fixed. > > Nevertheless, the emitting of spare jump slots is still incorrect, so > > the introduced test still fails due to incorrect iteration through the > Typo: s/due to/due to the/ Fixed. > > sparce table (the last slot is out of mcode range). > > > > This should be fixed via backporting of the commit > > dbb78630169a8106b355a5be8af627e98c362f1e ("MIPS: Fix handling of > > long-range spare jumps."). But it triggers the new unconditional > > assert, that is added in this patch, mentioning that sizemcode is too > > bit. So some workaround should be found, when this test will be enabled > Typo: s/bit/big/ > Typo: s/will be/is/ Fixed, thanks! > > for MIPS. > > > > Since test also validates the behaviour of long-range jumps to side > > traces for arm64 and x64, and we have no testing for MIPS64 (yet), we > > can leave it as is without a skipcond. > > > > Sergey Kaplun: > > * added the description and the test for the problem > > > > Part of tarantool/tarantool#8825 > > --- > > src/lj_asm_mips.h | 9 +-- > > src/lj_jit.h | 6 ++ > > src/lj_mcode.c | 6 -- > > ...x-mips64-spare-side-exit-patching.test.lua | 65 +++++++++++++++++++ > > 4 files changed, 76 insertions(+), 10 deletions(-) > > create mode 100644 test/tarantool-tests/fix-mips64-spare-side-exit-patching.test.lua > > > > diff --git a/test/tarantool-tests/fix-mips64-spare-side-exit-patching.test.lua b/test/tarantool-tests/fix-mips64-spare-side-exit-patching.test.lua > > new file mode 100644 > > index 00000000..fdc826cb > > --- /dev/null > > +++ b/test/tarantool-tests/fix-mips64-spare-side-exit-patching.test.lua > > @@ -0,0 +1,65 @@ > > +local tap = require('tap') > > +local test = tap.test('fix-mips64-spare-side-exit-patching'):skipcond({ > > + ['Test requires JIT enabled'] = not jit.status(), > > + ['Disabled on *BSD due to #4819'] = jit.os == 'BSD', > > + -- Need to fix the MIPS behaviour first. > Typo: s/Need to/We need to/ Fixed. > > + ['Disabled for MIPS architectures'] = jit.arch:match('mips'), > > + -- Allow to use 2000 traces to avoid flushes. > Typo: s/to use/compilation of up to/ Fixed. > > + 'maxtrace=2000', > > + -- Allow to compile 8Mb of mcode to be sure the issue occurs. > Typo: s/to compile/compilation of up to/ Fixed. > > + 'maxmcode=8192', > > + -- Use big mcode area for traces to avoid using different > Typo: s/using/usage of/ Fixed. > > + -- spare slots. > > + 'sizemcode=256' > > +) > > + > > +local MAX_SPARE_SLOT = 4 > A link to the definition in `lj_asm_mips.h` would be nice to have. Added. > > > +local function parent(marker) > > + -- Use several side exit to fill spare exit space (default is > Typo: s/side exit/side exits/ Fixed, thanks! > > + -- 4 slots, each slot has 2 instructions -- jump and nop). > > + -- luacheck: ignore > > + if marker > MAX_SPARE_SLOT then end > > + if marker > 3 then end > > + if marker > 2 then end > > + if marker > 1 then end > > + if marker > 0 then end > > + -- XXX: use `fmod()` to avoid leaving the function and use > > + -- stitching here. > > + return math.fmod(1, 1) > > +end > > + > > +-- Compile parent trace first. > > +parent(0) > > +parent(0) > > + > > +local parent_traceno = frontend.gettraceno(parent) > > +local last_traceno = parent_traceno > > + > > +-- Now generate some mcode to forcify long jump with a spare slot. > > +-- Each iteration provide different addresses and uses a different > Typo: s/provide/provides/ Fixed, thanks! > > +-- spare slot. After it compile and execute new side trace. > Typo: s/After it compile and execute/After that, compiles and executes a/ Fixed. See the iterative patch below. =================================================================== diff --git a/test/tarantool-tests/fix-mips64-spare-side-exit-patching.test.lua b/test/tarantool-tests/fix-mips64-spare-side-exit-patching.test.lua index fdc826cb..62933df9 100644 --- a/test/tarantool-tests/fix-mips64-spare-side-exit-patching.test.lua +++ b/test/tarantool-tests/fix-mips64-spare-side-exit-patching.test.lua @@ -2,7 +2,7 @@ local tap = require('tap') local test = tap.test('fix-mips64-spare-side-exit-patching'):skipcond({ ['Test requires JIT enabled'] = not jit.status(), ['Disabled on *BSD due to #4819'] = jit.os == 'BSD', - -- Need to fix the MIPS behaviour first. + -- We need to fix the MIPS behaviour first. ['Disabled for MIPS architectures'] = jit.arch:match('mips'), }) @@ -18,18 +18,19 @@ jit.opt.start( -- Try to compile all compiled paths as early as JIT can. 'hotloop=1', 'hotexit=1', - -- Allow to use 2000 traces to avoid flushes. + -- Allow compilation of up to 2000 traces to avoid flushes. 'maxtrace=2000', -- Allow to compile 8Mb of mcode to be sure the issue occurs. 'maxmcode=8192', - -- Use big mcode area for traces to avoid using different + -- Use big mcode area for traces to avoid usage of different -- spare slots. 'sizemcode=256' ) +-- See the define in the . local MAX_SPARE_SLOT = 4 local function parent(marker) - -- Use several side exit to fill spare exit space (default is + -- Use several side exits to fill spare exit space (default is -- 4 slots, each slot has 2 instructions -- jump and nop). -- luacheck: ignore if marker > MAX_SPARE_SLOT then end @@ -50,8 +51,9 @@ local parent_traceno = frontend.gettraceno(parent) local last_traceno = parent_traceno -- Now generate some mcode to forcify long jump with a spare slot. --- Each iteration provide different addresses and uses a different --- spare slot. After it compile and execute new side trace. +-- Each iteration provides different addresses and uses a +-- different spare slot. After that, compiles and executes a new +-- side trace. for i = 1, MAX_SPARE_SLOT + 1 do generators.fillmcode(last_traceno, 1024 * 1024) parent(i) =================================================================== > > 2.41.0 > > -- Best regards, Sergey Kaplun