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 4083CBB27DE; Fri, 14 Jun 2024 16:47:11 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 4083CBB27DE DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1718372831; bh=NOT189LpkObqS40cTtpDsZZ6OEG5k5cVr1gkRsjMErA=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=HpHpCuwQp3iXUTJfVFeauCIcPPv5SopKRlPDN3EVjeDhgqC5Oq09BBilumYpBjzdz 7c5rVWcRwzCccBlzLqEhKeACbB0OQZzxXBmjWNaYa1QgKchs5E4XOP6yT2OSd2Y9CP 70BRJyCMKSYcITa789rRnv9Cyo5yVvNnSMPVHXiQ= Received: from smtp51.i.mail.ru (smtp51.i.mail.ru [95.163.41.87]) (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 D32EABB27DD for ; Fri, 14 Jun 2024 16:47:09 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org D32EABB27DD Received: by smtp51.i.mail.ru with esmtpa (envelope-from ) id 1sI7Gj-00000009wpm-1Lsn; Fri, 14 Jun 2024 16:47:06 +0300 Content-Type: multipart/alternative; boundary="------------rYr00jHyLzjILY6a8UPrIH1X" Message-ID: Date: Fri, 14 Jun 2024 16:47:05 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Sergey Kaplun , Maxim Kokryashkin Cc: tarantool-patches@dev.tarantool.org References: <20240517132918.25926-1-skaplun@tarantool.org> In-Reply-To: <20240517132918.25926-1-skaplun@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD9F14564E980236A0727CECA7D5E958B875065EDC63782142F1313CFAB8367EF908E2BE116634AD74D88C2895605D7AE9A38ACD839565B2E138D5C4A2943711B3A09172136ECDB0B47F8D9167951636918 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7A20935EE237A17ECEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006372B866E63D69A26578638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8399C3CDF8EF3007EB478D7AEA49BC05EE6574994516A8BB2CC7F00164DA146DAFE8445B8C89999728AA50765F7900637DCE3DBD6F8E38AFD389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC8C2B5EEE3591E0D35F6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947C2D01283D1ACF37BAC0837EA9F3D197644AD6D5ED66289B523666184CF4C3C14F6136E347CC761E07725E5C173C3A84C36E36DCD5FF651F90BA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CF3D321E7403792E342EB15956EA79C166A417C69337E82CC275ECD9A6C639B01B78DA827A17800CE73D56AD9F5B48EAD3731C566533BA786AA5CC5B56E945C8DA X-C1DE0DAB: 0D63561A33F958A55D26AD8A4C730BF35002B1117B3ED696263B9B675606EE8A54BB1175C6E7DD94823CB91A9FED034534781492E4B8EEADA91A6E18C88C5E2F X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D340DAE5B306C240CF55EFECFD7E618922855B2E25A67AC1FB14230120B3A6A5A580CDE93590E2309B51D7E09C32AA3244CB474E805740D6B1AE70D6F05539DA95A159995DAA6A394F6EA455F16B58544A2557BDE0DD54B3590A5AE236DF995FB59978A700BF655EAEEED6A17656DB59BCAD427812AF56FC65B X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojqIk+/laaskAu1gQJL48rNw== X-Mailru-Sender: 520A125C2F17F0B1E52FEF5D219D614088C2895605D7AE9A38ACD839565B2E13DD747B6878DD72570152A3D17938EB451EB5A0BCEC6A560B3DDE9B364B0DF289BE2DA36745F2EEB5CEBA01FB949A1F1EEAB4BC95F72C04283CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] Maintain chain invariant in DCE. 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 Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" This is a multi-part message in MIME format. --------------rYr00jHyLzjILY6a8UPrIH1X Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Hi, Sergey thanks for the patch! see my comment below On 17.05.2024 16:29, Sergey Kaplun wrote: > From: Mike Pall > > Thanks to Peter Cawley. > > (cherry picked from commit f72c19e482b6f918b7cf42b0436e2b117d160a29) > > Instructions with strong guards that are sometimes emitted with a guard > and sometimes emitted without a guard (like HREFK, CONV, or SLOAD) may > be eliminated from the IR chain and replaced with the NOP IR. If the > next IR of the same kind on the trace is not eliminated, it may > reference the IR NOP instead of an instruction of the same type. This > may lead to the corresponding assertion failure in the `rec_check_ir()`. > > This patch unconditionally links the IRs during chain maintenance in > DCE. > > Sergey Kaplun: > * added the description and the test for the problem > > Part of tarantool/tarantool#9924 > --- > > Be aware that the reproducer from the ticket does not lead to the > assertion failure (this is why it is omitted in the test). I suppose it > just illustrates the situation when the IR is left off the chain. > Although the reproducer is clumsy, I can't simplify it or make it less > tricky. Please, ideas are welcome :). > > Branch:https://github.com/tarantool/luajit/tree/skaplun/lj-1094-ir-chain-dce > Related Issues: > *https://github.com/tarantool/tarantool/issues/9924 > *https://github.com/LuaJIT/LuaJIT/issues/1094 > > src/lj_opt_dce.c | 2 +- > .../lj-1094-ir-chain-dce.test.lua | 51 +++++++++++++++++++ > 2 files changed, 52 insertions(+), 1 deletion(-) > create mode 100644 test/tarantool-tests/lj-1094-ir-chain-dce.test.lua > > diff --git a/src/lj_opt_dce.c b/src/lj_opt_dce.c > index 2417f324..6948179c 100644 > --- a/src/lj_opt_dce.c > +++ b/src/lj_opt_dce.c > @@ -44,7 +44,6 @@ static void dce_propagate(jit_State *J) > IRIns *ir = IR(ins); > if (irt_ismarked(ir->t)) { > irt_clearmark(ir->t); > - pchain[ir->o] = &ir->prev; > } else if (!ir_sideeff(ir)) { > *pchain[ir->o] = ir->prev; /* Reroute original instruction chain. */ > ir->t.irt = IRT_NIL; > @@ -53,6 +52,7 @@ static void dce_propagate(jit_State *J) > ir->prev = 0; > continue; > } > + pchain[ir->o] = &ir->prev; > if (ir->op1 >= REF_FIRST) irt_setmark(IR(ir->op1)->t); > if (ir->op2 >= REF_FIRST) irt_setmark(IR(ir->op2)->t); > } > diff --git a/test/tarantool-tests/lj-1094-ir-chain-dce.test.lua b/test/tarantool-tests/lj-1094-ir-chain-dce.test.lua > new file mode 100644 > index 00000000..3faae7d4 > --- /dev/null > +++ b/test/tarantool-tests/lj-1094-ir-chain-dce.test.lua > @@ -0,0 +1,51 @@ > +local tap = require('tap') > + > +-- Test file to demonstrate LuaJIT's incorrect maintenance of the > +-- IR chain during DCE. > +-- See also:https://github.com/LuaJIT/LuaJIT/issues/1094. > + > +local test = tap.test('lj-1094-ir-chain-dce'):skipcond({ > + ['Test requires JIT enabled'] = not jit.status(), > +}) > + > +test:plan(1) > + > +jit.opt.start('hotloop=1') > + > +-- XXX: The code below is generated by the fuzzer (locally) and > +-- simplified as much as possible. > + > +-- Simple noop function. > +local __newindex = function() end > +debug.setmetatable(0, { > + __newindex = __newindex, > +}) > + > +local counter = 0 > +-- luacheck: no unused > +local tab = {} > +while true do > + -- The loop is still not recorded because the guard always > + -- fails. > + -- So, just try to compile it and check that there is no > + -- assertion failure. > + if counter > 2 then break end > + counter = counter + 1 > + -- The pattern `-#{}` allows us to get CONV IRs. The first > + -- appearance of this IR (in the `(-#{}).key`) is considered > + -- unused by the compiler due to the corresponding "noop" > + -- `__newindex` function. > + -- The second usage of conversion (`tab[-#{}]`) is guarded (to > + -- the int type), so it can't be eliminated. > + -- As a result, the 0048 CONV references the 0039 NOP IR after > + -- DCE in the IR chain. I suppose an IR output would be helpful here. What do you think? > + -- XXX: TDUP prevents the corresponding second usage from being > + -- eliminated since the table insert semantics may change. > + -- XXX: Use some numbers to simplify reading the `jit.dump` > + -- output. > + tab, tab[-#{}], (-#{}).key = {tdup = 'tdup'}, 1, 2 > +end > + > +test:ok(true, 'no assertion failure') > + > +test:done(true) --------------rYr00jHyLzjILY6a8UPrIH1X Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 7bit

Hi, Sergey

thanks for the patch! see my comment below

On 17.05.2024 16:29, Sergey Kaplun wrote:
From: Mike Pall <mike>

Thanks to Peter Cawley.

(cherry picked from commit f72c19e482b6f918b7cf42b0436e2b117d160a29)

Instructions with strong guards that are sometimes emitted with a guard
and sometimes emitted without a guard (like HREFK, CONV, or SLOAD) may
be eliminated from the IR chain and replaced with the NOP IR. If the
next IR of the same kind on the trace is not eliminated, it may
reference the IR NOP instead of an instruction of the same type. This
may lead to the corresponding assertion failure in the `rec_check_ir()`.

This patch unconditionally links the IRs during chain maintenance in
DCE.

Sergey Kaplun:
* added the description and the test for the problem

Part of tarantool/tarantool#9924
---

Be aware that the reproducer from the ticket does not lead to the
assertion failure (this is why it is omitted in the test). I suppose it
just illustrates the situation when the IR is left off the chain.
Although the reproducer is clumsy, I can't simplify it or make it less
tricky. Please, ideas are welcome :).

Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1094-ir-chain-dce
Related Issues:
* https://github.com/tarantool/tarantool/issues/9924
* https://github.com/LuaJIT/LuaJIT/issues/1094

 src/lj_opt_dce.c                              |  2 +-
 .../lj-1094-ir-chain-dce.test.lua             | 51 +++++++++++++++++++
 2 files changed, 52 insertions(+), 1 deletion(-)
 create mode 100644 test/tarantool-tests/lj-1094-ir-chain-dce.test.lua

diff --git a/src/lj_opt_dce.c b/src/lj_opt_dce.c
index 2417f324..6948179c 100644
--- a/src/lj_opt_dce.c
+++ b/src/lj_opt_dce.c
@@ -44,7 +44,6 @@ static void dce_propagate(jit_State *J)
     IRIns *ir = IR(ins);
     if (irt_ismarked(ir->t)) {
       irt_clearmark(ir->t);
-      pchain[ir->o] = &ir->prev;
     } else if (!ir_sideeff(ir)) {
       *pchain[ir->o] = ir->prev;  /* Reroute original instruction chain. */
       ir->t.irt = IRT_NIL;
@@ -53,6 +52,7 @@ static void dce_propagate(jit_State *J)
       ir->prev = 0;
       continue;
     }
+    pchain[ir->o] = &ir->prev;
     if (ir->op1 >= REF_FIRST) irt_setmark(IR(ir->op1)->t);
     if (ir->op2 >= REF_FIRST) irt_setmark(IR(ir->op2)->t);
   }
diff --git a/test/tarantool-tests/lj-1094-ir-chain-dce.test.lua b/test/tarantool-tests/lj-1094-ir-chain-dce.test.lua
new file mode 100644
index 00000000..3faae7d4
--- /dev/null
+++ b/test/tarantool-tests/lj-1094-ir-chain-dce.test.lua
@@ -0,0 +1,51 @@
+local tap = require('tap')
+
+-- Test file to demonstrate LuaJIT's incorrect maintenance of the
+-- IR chain during DCE.
+-- See also: https://github.com/LuaJIT/LuaJIT/issues/1094.
+
+local test = tap.test('lj-1094-ir-chain-dce'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+test:plan(1)
+
+jit.opt.start('hotloop=1')
+
+-- XXX: The code below is generated by the fuzzer (locally) and
+-- simplified as much as possible.
+
+-- Simple noop function.
+local __newindex = function() end
+debug.setmetatable(0, {
+    __newindex = __newindex,
+})
+
+local counter = 0
+-- luacheck: no unused
+local tab = {}
+while true do
+  -- The loop is still not recorded because the guard always
+  -- fails.
+  -- So, just try to compile it and check that there is no
+  -- assertion failure.
+  if counter > 2 then break end
+  counter = counter + 1
+  -- The pattern `-#{}` allows us to get CONV IRs. The first
+  -- appearance of this IR (in the `(-#{}).key`) is considered
+  -- unused by the compiler due to the corresponding "noop"
+  -- `__newindex` function.
+  -- The second usage of conversion (`tab[-#{}]`) is guarded (to
+  -- the int type), so it can't be eliminated.
+  -- As a result, the 0048 CONV references the 0039 NOP IR after
+  -- DCE in the IR chain.
I suppose an IR output would be helpful here. What do you think?
+  -- XXX: TDUP prevents the corresponding second usage from being
+  -- eliminated since the table insert semantics may change.
+  -- XXX: Use some numbers to simplify reading the `jit.dump`
+  -- output.
+  tab, tab[-#{}], (-#{}).key = {tdup = 'tdup'}, 1, 2
+end
+
+test:ok(true, 'no assertion failure')
+
+test:done(true)
--------------rYr00jHyLzjILY6a8UPrIH1X--