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 0A95448571F; Thu, 18 May 2023 23:59:18 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 0A95448571F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1684443558; bh=9fbZU5qYV94isAwNSbMOJ0bfwdKibN1kRQkgOYxfI2Y=; 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=O/nZINlSjAbJb8qPkDt/10XXqq4FV4gydCyaE+w9IC8UH4Gr+7XfHBAiVkPPlM01v rAldo8B3RPyl1KWFmmJSgOg4t6jAYi9GTkBBnqYnzbHrnXJ0y6dU5txRZ9frbICPS+ JJXoaUC6EqivtA/A5Ige8bHEIN2vYaz0VBrKrwOY= 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 CECCD485700 for ; Thu, 18 May 2023 23:59:16 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org CECCD485700 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1pzkiR-00070w-Si; Thu, 18 May 2023 23:59:16 +0300 Date: Thu, 18 May 2023 23:55:12 +0300 To: Maxim Kokryashkin Message-ID: References: <20230510213002.5153-1-skaplun@tarantool.org> <1684146617.920168662@f492.i.mail.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1684146617.920168662@f492.i.mail.ru> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD921E8753A900160F1A884BFCCACF6817A855E73339B4573DA182A05F538085040EBD1E1C2D564FD04BCBD3510EE3C97CD79524BF99FAFED16C9E0923C5CED22A7 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7F65C230EDDCD559EEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637D3A9DC970DD6E2F6EA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38B6F1F7B995052D5CE95F833B316CD794D5E0490819CC3430ACC7F00164DA146DAFE8445B8C89999728AA50765F7900637A596F99EC23ACBE8389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC83916E35B8AF55EAFF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA7E827F84554CEF5019E625A9149C048EE9ECD01F8117BC8BEE2021AF6380DFAD18AA50765F790063735872C767BF85DA227C277FBC8AE2E8B569F1129A2C6445075ECD9A6C639B01B4E70A05D1297E1BBCB5012B2E24CD356 X-C1DE0DAB: 0D63561A33F958A590C8509F419386487E0A2D0FD4E04E23CAA23EA20D6BCE59F87CCE6106E1FC07E67D4AC08A07B9B06A1CB4668A9CA5FACB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D34D8C933888226C841F4AFA81D38A87AC875A05CFA2B07EDB2D1EBCD1DA5523B5219CA60A92603E4081D7E09C32AA3244C8E1936EFF4BAA9B9BA7C5D58FE92AD0E39C99C45E8D137E9FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojXj30btAgf0bD4lhpM7cDiQ== X-DA7885C5: E186F0CA77083D162CF674DD72187FC1D5EBD39F0312F06847F09832E2296AE7262E2D401490A4A0DB037EFA58388B346E8BC1A9835FDE71 X-Mailru-Sender: 689FA8AB762F73933AF1F914F131DBF5F407048FACFEEEB88EE48FD91DC2AE450FBE9A32752B8C9C2AA642CC12EC09F1FB559BB5D741EB962F61BD320559CF1EFD657A8799238ED55FEEDEB644C299C0ED14614B50AE0675 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] Fix TNEW load forwarding with instable types. 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! On 15.05.23, Maxim Kokryashkin wrote: > > Hi, Sergey! > Thanks for the patch! > LGTM, except for several nits below. >   > >  > >>From: Mike Pall > >> > >>(cherry picked from commit d4c0c6e17ef7edf1f2893bc807746b80612e63e9) > >> > >>During loop unrolling IR instructions are coped, substituted and > >>re-emitted for the FOLD/CSE/etc. pipeline. Assume, we have the following > >>loop-carried dependency: we create a new table or load some stored one > >>depends on iteration number. In case when we copy the emitted ALOAD IR > >Typo: s/depends on iteration/depending on the iteration/ > >Typo: s/In case when/If/ > >>instruction (from from the stored table), we copy it as is, including > >Typo: s/from from/from/ > >>loading type. But the ALOAD from the new table created (on the previous > >Typo: s/loading/load/ > >>iteration) should have nil type, so the assertion in `fwd_ahload()` is > >>failed. > >> > >>This patch replaces the assertion to `return 0` to avoid the assertion > >Typo: s/assertion to/assertion with/ > >>failure and stop forwarding in such situations. > >> > >>But the emitted type-guarded ALOAD will lead to the persistent failure > >>of the assertion guard on the trace. Hence, another one fix should be > >Typo: s/another one/another/ > >>added. Also, TDUP IR is affected, too. > >>See also the issue mentioned in the test. Thanks for the comments, fixed, branch is force-pushed. The new commit message is the following: | Fix TNEW load forwarding with instable types. | | (cherry picked from commit d4c0c6e17ef7edf1f2893bc807746b80612e63e9) | | During loop unrolling IR instructions are copied, substituted and | re-emitted for the FOLD/CSE/etc. pipeline. Assume, we have the following | loop-carried dependency: we create a new table or load some stored one | depends on the iteration number. If we copy the emitted ALOAD IR | instruction (from the stored table), we copy it as is, including load | type. But the ALOAD from the new table created (on the previous | iteration) should have nil type, so the assertion in `fwd_ahload()` is | failed. | | This patch replaces the assertion with `return 0` to avoid the assertion | failure and stop forwarding in such situations. | | But the emitted type-guarded ALOAD will lead to the persistent failure | of the assertion guard on the trace. Hence, another fix should be | added. Also, TDUP IR is affected, too. | See also the issue mentioned in the test. | | Sergey Kaplun: | * added the description and the test for the problem | | Part of tarantool/tarantool#8516 > >> > >>Sergey Kaplun: > >>* added the description and the test for the problem > >> > >>Part of tarantool/tarantool#8516 > >>--- > >> > >>Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-994-instable-types-during-loop-unroll > >>PR: https://github.com/tarantool/tarantool/pull/8642 > >>Related issues: > >>* https://github.com/tarantool/tarantool/issues/8516 > >>* https://github.com/LuaJIT/LuaJIT/issues/994 > >> > >>I don't mention the 994 intentionally to avoid Mike bothering. Also, it > >>isn't the origin of this commit. Quite the opposite: as a result of this > >>backporting the following issue was created. > >> > >>I prefer to backport the patches (this one and the prospective for 994) > >>separately. So, after this patch is backported, it doesn't add any > >>critical bugs (always failing guard just creates a new side trace), but > >>helps to avoid conflicts for future backporting (sadly remembered > >>"Improve assertions."). > >> > >> src/lj_opt_mem.c | 3 +- > >> ...instable-types-during-loop-unroll.test.lua | 53 +++++++++++++++++++ > >> 2 files changed, 55 insertions(+), 1 deletion(-) > >> create mode 100644 test/tarantool-tests/lj-994-instable-types-during-loop-unroll.test.lua > >> > >>diff --git a/src/lj_opt_mem.c b/src/lj_opt_mem.c > >>index cc177d39..c8265b4f 100644 > >>--- a/src/lj_opt_mem.c > >>+++ b/src/lj_opt_mem.c > >>diff --git a/test/tarantool-tests/lj-994-instable-types-during-loop-unroll.test.lua b/test/tarantool-tests/lj-994-instable-types-during-loop-unroll.test.lua > >>new file mode 100644 > >>index 00000000..435f6e0e > >>--- /dev/null > >>+++ b/test/tarantool-tests/lj-994-instable-types-during-loop-unroll.test.lua > >>+ -- Use non-constant key to avoid LJ_TRERR_GFAIL and undoing the > >>+ -- loop. > >Typo: s/Use/Use a/ Fixed. > >>+ result = t[key] > >>+ -- Swap table loaded by SLOAD to the created via TNEW. > >>+ slot = _ % 2 ~= 0 and stored_tab or {} > >>+end > >>+test:is(result, nil, 'TNEW load forwarding was successful') > >Nit: not sure about that assertion. AFAICS, we want to check whether we > >have failed on assertion and not the validity of `result` variable. Feel free > >to ignore. I want to verify that behaviour isn't changed by JIT, so a leave this check as is. Ignoring. > >>+ > >>+-- TDUP. > >>+--[[ > >>+-- FIXME: Disable test case for now. Enable, with another one > >Typo: s/another one/another/ Fixed, thanks! > >>+-- backported commit with a fix for the aforementioned issue > >>+test:is(result, true, 'TDUP load forwarding was successful') > >>+]] > >Nit: Not sure if it is good to add this test here. Maybe it is better > >to create a ticket in our repo and move that chunk there. Feel free > >to ignore. Me, too, but I'll prefer to place these tests in one test file, since it is the general issue for TDUP|TNEW IR-s. Also, reference the single ticket. Ignoring. > >>+ > >>+os.exit(test:check() and 0 or 1) > >>-- > >>2.34.1 > >-- > >Best regards, > >Maxim Kokryashkin > >  -- Best regards, Sergey Kaplun