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 EFDDA6F44D7; Thu, 30 Nov 2023 10:39:11 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org EFDDA6F44D7 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1701329952; bh=cwBzLBHM7/UavjgNTq1i/G3hHhq/T1i/ro5Am/Qb6bk=; 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=PdB4XhctDn1EfQAwedLwbLmlQj6F2gnF+aJfC6Y3nOSJvuBQb9y04C1LrssapTH77 Mqwe1P6+yK0VSLD/vmJeIWzGMTCdGqFc/tu6cAscdY2KQHrshvltvaHFEvou852d5e ZoiZnL3G52tQiIGDJom3M/zSXYgvbvBTQx7Szo44= Received: from smtp31.i.mail.ru (smtp31.i.mail.ru [95.163.41.72]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 80A996F08D7 for ; Thu, 30 Nov 2023 10:39:10 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 80A996F08D7 Received: by smtp31.i.mail.ru with esmtpa (envelope-from ) id 1r8bdd-00B5za-0v; Thu, 30 Nov 2023 10:39:09 +0300 Date: Thu, 30 Nov 2023 10:34:39 +0300 To: Sergey Bronnikov Message-ID: References: <20231128122112.16229-1-skaplun@tarantool.org> <07734b67-027a-4cc8-9c8a-03cd77a54137@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <07734b67-027a-4cc8-9c8a-03cd77a54137@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9D2A6479154BC7F417A3ADCF799DE78622B6E7AFD5C732E26182A05F538085040CF58182A95D02F2C3F1B1E90A10F3B369DFFAB5749D1E9442A4258B52F6DD336 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE779AAD18609327F83EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063724170451E8B6ECF78638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8AB9DDE3326EE40BDFF14BDBD8954B9C0117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC8C7ADC89C2F0B2A5A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD182CC0D3CB04F14752D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269176DF2183F8FC7C0E9CA1980FA7BB05068655334FD4449CB9ECD01F8117BC8BEAAAE862A0553A39223F8577A6DFFEA7C8BDE37D78FCB031643847C11F186F3C59DAA53EE0834AAEE X-C1DE0DAB: 0D63561A33F958A5795B04084E8BED40788216C9A06FB01721659CD763E40EC6F87CCE6106E1FC07E67D4AC08A07B9B065B78C30F681404DCB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D3455049D7B43D89D644512280588ACC12A5A5EB10308EDDB64CA8B7105782BB913909FFF3451D219281D7E09C32AA3244C1018639FD5406E5D04B12FF687D3803760759606DA2E136A85A42E4C463514DC5DA084F8E80FEBD3202CD0F03380D9577A83BD0C44CE203720ABEDE4BBDD9CDD X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojLZJja4YqbLT4We1G6tS9Cw== X-DA7885C5: 220392546A2B8DC02B0B25145D579A62D4D50BC88367C3945C1CFAB35C81FA04262E2D401490A4A0DB037EFA58388B346E8BC1A9835FDE71 X-Mailru-Sender: 689FA8AB762F7393590D8C940224AE338FB90B656A4D0803370FBDB2E2D952BC0FBE9A32752B8C9C2AA642CC12EC09F1FB559BB5D741EB962F61BD320559CF1EFD657A8799238ED55FEEDEB644C299C0ED14614B50AE0675 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] Prevent CSE of a REF_BASE operand across IR_RETF. 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, Sergey! Thanks for the review! I've fixed your comment regarding test comment and force-pushed the branch. On 29.11.23, Sergey Bronnikov wrote: > Hello, Sergey > > thanks for the patch! > > LGTM with a three minor comments below > > On 11/28/23 15:21, Sergey Kaplun wrote: > > From: Mike Pall > > > > Reported by XmiliaH. > > > > (cherry-picked from commit e73916d811710ab02a4dfe447d621c99f4e7186c) > > > > The RETF IR has a side effect: it shifts base when returning to a lower > > frame, i.e., it affects `REF_BASE` IR (0000) (thus, we can say that this > > IR is violating SSA form). So any optimization of IRs with `REF_BASE` as > > an operand across RETF IR may lead to incorrect optimizations (see > > details in the test file). > > > > This patch adds rules to the folding engine to prevent CSE across `IR_RETF` > > for all possible IRs containing REF_BASE. > > > > Sergey Kaplun: > > * added the description and the test for the problem > > > > Part of tarantool/tarantool#9145 > > --- > > > > Branch:https://github.com/tarantool/luajit/tree/skaplun/lj-784-cse-ref-base-over-retf > > Tarantool PR:https://github.com/tarantool/tarantool/pull/9421 > > Related issues: > > *https://github.com/LuaJIT/LuaJIT/issues/784 > > *https://github.com/tarantool/tarantool/issues/9145 > > > > Interested reviewers can mention that only the `SUB any BASE` case is > > tested. > > The reason is that other cases are impossible to record in LuaJIT: > > * EQ any BASE: EQ pgc REF_BASE IR for upvalues is emitted when > > the open upvalue aliases a SSA slot, i.e., it belongs to the frame of > > the currently executed function. In that case, if we want to emit RETF > > IR, we need to leave this function. So we need to record the UCLO > > bytecode, which is NIY in JIT. So, such a type of trace is impossible. > > * SUB BASE any: SUB BASE fr is emitted for the recording of VARG > > Nit: fr -> frame > > or put in backticks if you refer to a variable in source code > > > bytecode, in case varargs are undefined on trace. We need a vararg > > function to call to create an additional frame. But returning to lower > > frames from a vararg function isn't implemented in LuaJIT -- either > > the trace recording is stopped or the error is rased and the trace > > isn't compiled. Also, IINM, fr operands will always be different for > > Nit: fr -> frame > > or put in backticks if you refer to a variable in source code Since this is only a remider for review in the ML, I've not changed it:). I suppose that to mention this information in the commit message is excess and important only for clarification on review. > > > different frames, so there is no possible CSE here. > > > > So, these cases are needed to prevent any regressions in the future. > > > > Please correct me if I've missed something. > > > > src/lj_opt_fold.c | 11 +++ > > .../lj-784-cse-ref-base-over-retf.test.lua | 86 +++++++++++++++++++ > > 2 files changed, 97 insertions(+) > > create mode 100644 test/tarantool-tests/lj-784-cse-ref-base-over-retf.test.lua > > > > diff --git a/src/lj_opt_fold.c b/src/lj_opt_fold.c > > index c5f2232e..750f1c7e 100644 > > --- a/src/lj_opt_fold.c > > +++ b/src/lj_opt_fold.c > > @@ -2313,6 +2313,17 @@ LJFOLDF(xload_kptr) > > LJFOLD(XLOAD any any) > > LJFOLDX(lj_opt_fwd_xload) > > > > +/* -- Frame handling ------------------------------------------------------ */ > > + > > +/* Prevent CSE of a REF_BASE operand across IR_RETF. */ > > +LJFOLD(SUB any BASE) > > +LJFOLD(SUB BASE any) > > +LJFOLD(EQ any BASE) > > +LJFOLDF(fold_base) > > +{ > > + return lj_opt_cselim(J, J->chain[IR_RETF]); > > +} > > + > > /* -- Write barriers ------------------------------------------------------ */ > > > > /* Write barriers are amenable to CSE, but not across any incremental > > diff --git a/test/tarantool-tests/lj-784-cse-ref-base-over-retf.test.lua b/test/tarantool-tests/lj-784-cse-ref-base-over-retf.test.lua > > new file mode 100644 > > index 00000000..095376fc > > --- /dev/null > > +++ b/test/tarantool-tests/lj-784-cse-ref-base-over-retf.test.lua > > @@ -0,0 +1,86 @@ > > +local tap = require('tap') > > + > > +-- Test file to demonstrate incorrect FOLD optimization for IR > > +-- with REF_BASE operand across IR RETF. > > +-- See also,https://github.com/LuaJIT/LuaJIT/issues/784. > > + > > +local test = tap.test('lj-784-cse-ref-base-over-retf'):skipcond({ > > + ['Test requires JIT enabled'] = not jit.status(), > > +}) > > + > > +test:plan(1) > > + > > +-- The RETF IR has a side effect: it shifts base when returning to > > +-- a lower frame, i.e., it affects `REF_BASE` IR (0000) (thus, we > > +-- can say that this IR is violating SSA form). > > +-- So any optimization of IRs with `REF_BASE` as an operand across > > +-- RETF IR may lead to incorrect optimizations. > > +-- In this test, SUB uref REF_BASE IR was eliminated, so instead > > +-- the following trace: > > +-- > > +-- 0004 p32 SUB 0003 0000 > > +-- 0005 > p32 UGT 0004 +32 > > +-- ... > > +-- 0009 > p32 RETF proto: 0x407dc118 [0x407dc194] > > +-- ... > > +-- 0012 p32 SUB 0003 0000 > > +-- 0013 > p32 UGT 0012 +72 > > +-- > > +-- We got the following: > > +-- > > +-- 0004 p32 SUB 0003 0000 > > +-- 0005 > p32 UGT 0004 +32 > > +-- ... > > +-- 0009 > p32 RETF proto: 0x41ffe0c0 [0x41ffe13c] > > +-- ... > > +-- 0012 > p32 UGT 0004 +72 > > +-- > > +-- As you can see, the 0012 SUB IR is eliminated because it is the > > +-- same as the 0004 IR. This leads to incorrect assertion guards > > +-- in the IR below. > > I would rephrase it to "assertion guards in the resulted IR" > > because there is no IR below the comment. Fixed, branch is force-pushed. =================================================================== diff --git a/test/tarantool-tests/lj-784-cse-ref-base-over-retf.test.lua b/test/tarantool-tests/lj-784-cse-ref-base-over-retf.test.lua index 095376fc..d6442cbb 100644 --- a/test/tarantool-tests/lj-784-cse-ref-base-over-retf.test.lua +++ b/test/tarantool-tests/lj-784-cse-ref-base-over-retf.test.lua @@ -37,7 +37,7 @@ test:plan(1) -- -- As you can see, the 0012 SUB IR is eliminated because it is the -- same as the 0004 IR. This leads to incorrect assertion guards --- in the IR below. +-- in the resulted IR 0012 below. local MAGIC = 42 -- XXX: simplify `jit.dump()` output. =================================================================== > > > + > > +local MAGIC = 42 > > +-- XXX: simplify `jit.dump()` output. > > +local fmod = math.fmod > > + > > +local function exit_with_retf(closure) > > + -- Forcify stitch. Any NYI is OK here. > > + fmod(1, 1) > > + -- Call the closure so that we have emitted `uref - REF_BASE`. > > + closure(0) > > + -- Exit with `IR_RETF`. This will change `REF_BASE`. > > +end > > + > > +local function sub_uref_base(closure) > > + local open_upvalue > > + if closure == nil then > > + closure = function(val) > > + local old = open_upvalue > > + open_upvalue = val > > + return old > > + end > > + -- First, create an additional frame, so we got the trace, > > + -- where the open upvalue reference is always < `REF_BASE`. > > + sub_uref_base(closure) > > + end > > + for _ = 1, 4 do > > + -- `closure` function is inherited from the previous frame. > > + exit_with_retf(closure) > > + open_upvalue = MAGIC > > + -- The open upvalue guard will use CSE over `IR_RETF` for > > + -- `uref - REF_BASE`. `IR_RETF` changed the value of > > + -- `REF_BASE`. > > + -- Thus, the guards afterwards take the wrong IR as the first > > + -- operand, so they are not failed, and the wrong value is > > + -- returned from the trace. > > + open_upvalue = closure(0) > > + end > > + return open_upvalue > > +end > > + > > +jit.opt.start('hotloop=1') > > + > > +local res = sub_uref_base() > > +test:is(res, MAGIC, 'no SUB uref REF_BASE CSE across RETF') > > + > > +test:done(true) -- Best regards, Sergey Kaplun