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 E0F226ECE3; Wed, 22 Jun 2022 16:34:59 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org E0F226ECE3 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1655904900; bh=q4brWorQmfk3Kn6rqqcx7iZcI8Z1limTNNsK+grOdv4=; 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=xRo5XM12NNfhn8Qe3bor+Hx5nydIvPq3D5jNq+zUA0OQPZkCiwxuq29fzV/ce+Myk JQN+wFADugbz0MinQsU8jqJi6Kb2oS1sEDB7JdXNac/GZ68IY0TWkgiHnC4HQC5wdm FqpUskx+sMFTAnIftVOqfgSFd2uitZeGzcPr91+g= 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 85D276ECE3 for ; Wed, 22 Jun 2022 16:34:58 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 85D276ECE3 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1o40VV-0002KO-Iy; Wed, 22 Jun 2022 16:34:58 +0300 Date: Wed, 22 Jun 2022 16:32:38 +0300 To: sergos Message-ID: References: <804A99A3-6D0C-4DA9-A939-26FFED0EC823@tarantool.org> <20210704210647.GA6106@tarantool.org> <56F81FD2-FDD5-492E-9A16-6829EA710561@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <56F81FD2-FDD5-492E-9A16-6829EA710561@tarantool.org> X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD947AAA3FEFDE5AEDDE6FC258494D986E61644F6F5506A94AC182A05F5380850402EC1C6EDA0749C60F5DBC47CAC672D4C7BD3388E547B73E83D433D2046322D29 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE737BB76880A4CA9A4EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637525B22DCF689D4638638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D85F68286F5897F234FC05228C8F6FE7BD117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC8C7ADC89C2F0B2A5A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18BDFBBEFFF4125B51D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B613439FA09F3DCB32089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-8FC586DF: 6EFBBC1D9D64D975 X-C1DE0DAB: 9604B64F49C60606AD91A466A1DEF99B296C473AB1E14218F88A8A6BF20D15678859CA687ABA27BA65126C64885D376F68B9112C9B94FC02589120F7DAE46353205367B2BCC23E5B6EEA0E1E362FEDAA410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D3430FC2BC1B917F018F267A1A4CB7C512E0348FA9987CAA13E586A167D3EF8F990B0C20CEAA947B9AC1D7E09C32AA3244C1A440A13F2CF0E44419722B52899F205D08D48398F32B4A6FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj9/HGj9wIviNDsotLDdsAVA== X-Mailru-Sender: 689FA8AB762F7393CC2E0F076E87284EE96201C0A8D4C0C62D8B163050DFEC050FBE9A32752B8C9C2AA642CC12EC09F1FB559BB5D741EB962F61BD320559CF1EFD657A8799238ED55FEEDEB644C299C0ED14614B50AE0675 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATH luajit] GC64: fix 64-bit constant fusion 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, Sergos! Thanks for the review! On 21.06.22, sergos wrote: > Hi! > > Thanks for working on it! > Just minor nits, > LGTM Fixed. See the iterative diff below. Branch is force pushed. > > Sergos > > > On 16 Feb 2022, at 18:44, Sergey Kaplun wrote: > > > > Hi, Sergos! > > Thanks for the patch! > > > > Hi , Igor! > > Thanks for the review! > > > > On 05.07.21, Igor Munkin wrote: > >> Sergos, > >> > >> Thanks for the patch! Something went wrong with formatting the patch I > >> guess, so I'll share you the recipe how I send the backported patches: > >> 1. cherry-pick the original patch from the upstream > >> 2. format-patch the backported commit > >> 3. Add "From" header with the original author of the changes at the > >> very beginning of the formatted patch > >> 4. Adjust the commit message according to our backporting procedure > >> > >> BTW, I've found neither the branch with the patch in tarantool/luajit > >> nor the branch with the green CI in tarantool/tarantool... > > > > I've updated a little the old Sergos's branch [1] and save results on my > > own branch [2]. > > Also I've added GC64 workflow for branch checkouted from the current > > Tarantool's master branch [3]. > > > > The new commit message and patch is the following: > > > > =================================================================== > > commit 0759d1783ef96c4bb47869c6bc1181b38d95f654 > > Author: Mike Pall > > Date: Mon Aug 28 10:43:37 2017 +0200 > > > > From: Mike Pall Fixed the commit message :) | x64/LJ_GC64: Fix fallback case of asm_fuseloadk64(). | | Contributed by Peter Cawley. | | (chekky picked from commit 6b0824852677cc12570c20a3211fbfe0e4f0ce14) | | Code generation under LJ_GC64 missed an update to the mcode area ... > > > > (chekky picked from commit 6b0824852677cc12570c20a3211fbfe0e4f0ce14) > > > > x64/LJ_GC64: Fix fallback case of asm_fuseloadk64(). > > > > Contributed by Peter Cawley. > > > > Code generation under LJ_GC64 missed an update to the mcode area > > boundaries after a 64-bit constant encoding. It can lead to a > > corruption of the constant value after next trace code generation. > > The constant can be encoded in one of the following ways: > > > > a. If the address of the constant fits into 32-bit one, then encode it > > as a 32-bit displacement (the only option for non-GC64 mode). > > b. If the offset of the constant slot from the dispatch table (pinned > > to r14 that is not changed while trace execution) fits into 32-bit, > > then encode this as a 32-bit displacement relative to r14. > > c. If the offset of the constant slot from the mcode (i.e. rip) fits > > into 32-bit, then encode this as a 32-bit displacement relative to > > rip (considering long mode specifics and RID_RIP hack). > > d. If none of the conditions above are valid, compiler materializes > > this 64-bit constant right at the trace bottom and encodes this the > > same way it does for the previous case. > > > > The mentioned problem appears only with 2Gb distance from the currently > > allocated mcode to the dispatch pointer, which may happen in real life > > in case of long running instance. > > > > Sergey Ostanevich: > > * added the description and the test for the problem > > > > Fixes tarantool/tarantool#4095 > > Fixes tarantool/tarantool#4199 > > Fixes tarantool/tarantool#4614 > > > > diff --git a/src/lj_asm_x86.h b/src/lj_asm_x86.h > > index 767bf6f3..2850aea9 100644 > > --- a/src/lj_asm_x86.h > > +++ b/src/lj_asm_x86.h > > @@ -387,6 +387,7 @@ static Reg asm_fuseloadk64(ASMState *as, IRIns *ir) > > ir->i = (int32_t)(as->mctop - as->mcbot); > > as->mcbot += 8; > > as->mclim = as->mcbot + MCLIM_REDZONE; > > + lj_mcode_commitbot(as->J, as->mcbot); > > } > > as->mrm.ofs = (int32_t)mcpofs(as, as->mctop - ir->i); > > as->mrm.base = RID_RIP; > > diff --git a/test/tarantool-tests/gh-4199-gc64-fuse.test.lua b/test/tarantool-tests/gh-4199-gc64-fuse.test.lua > > new file mode 100644 > > index 00000000..9eabbdba > > --- /dev/null > > +++ b/test/tarantool-tests/gh-4199-gc64-fuse.test.lua > > @@ -0,0 +1,79 @@ > > +-- The test is GC64 only. > > +local ffi = require('ffi') > > +require('utils').skipcond(not ffi.abi('gc64'), 'test is GC64 only') > > + > > +local tap = require('tap') > > +local test = tap.test('gh-4199-gc64-fuse') > > +test:plan(1) > > + > > +collectgarbage() > > +-- Chomp memory in currently allocated GC space. > > +collectgarbage('stop') > > + > > +for _ = 1, 8 do > > + ffi.new('char[?]', 256 * 1024 * 1024) > > +end > > + > > +jit.opt.start('hotloop=1') > > + > > +-- Generate a bunch of traces to trigger constant placement at the > > +-- end of the trace. Since global describing the mcode area in the > ^ ^ > a variable > > +-- jit structure is not updated, the next trace generated will > > +-- invalidate the constant of the previous trace. Then > > +-- execution of the _previous_ trace will use wrong value. > ^ > the > > + > > +-- Keep last two functions generated to compare results. > ^ > the I've updated comments as the following: =================================================================== diff --git a/test/tarantool-tests/gh-4199-gc64-fuse.test.lua b/test/tarantool-tests/gh-4199-gc64-fuse.test.lua index 9eabbdba..b34b4a4f 100644 --- a/test/tarantool-tests/gh-4199-gc64-fuse.test.lua +++ b/test/tarantool-tests/gh-4199-gc64-fuse.test.lua @@ -17,12 +17,13 @@ end jit.opt.start('hotloop=1') -- Generate a bunch of traces to trigger constant placement at the --- end of the trace. Since global describing the mcode area in the --- jit structure is not updated, the next trace generated will --- invalidate the constant of the previous trace. Then --- execution of the _previous_ trace will use wrong value. +-- end of the trace. Since a global variable describing the mcode +-- area in the jit structure is not updated, the next trace +-- generated will invalidate the constant of the previous trace. +-- Then execution of the _previous_ trace will use the wrong +-- value. --- Keep last two functions generated to compare results. +-- Keep the last two functions generated to compare results. local s = {} local test_res = true =================================================================== The branch is force-pushed. > > +local s = {} > > +local test_res = true > > + > > +-- XXX: The number of iteration is fragile, depending on the trace > > +-- length and the amount of currently pre-allocated mcode area. > > +-- Usually works under 100, which doesn't take too long in case of > > +-- success, so I gave up to locate a better way to chomp the mcode > > +-- area. > > + > > +for n = 1, 100 do > > + local src = string.format([[ > > + function f%d(x, y) > > Not sure if indentation is preserved in the patch and should it be here? Yes, this is something strange with offset here. I've used 2 spaces as usual. > > > + local a = {} > >> IM > > > > [1]: https://github.com/tarantool/luajit/tree/sergos/gh-4095-gc64-const-fusion > > [2]: https://github.com/tarantool/luajit/tree/skaplun/gc64-constant-fuse-full-ci > > [3]: https://github.com/tarantool/tarantool/tree/skaplun/gh-4199-gc64-fuse-full-ci > > > > -- > > Best regards, > > Sergey Kaplun > -- Best regards, Sergey Kaplun