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 A161E65D959; Fri, 6 Oct 2023 13:49:22 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A161E65D959 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1696589362; bh=YZohUFixSMQKJrV5NKa9SDjJpb7ARolhtLnMUS0tOVo=; 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=BuZUWrdtMG8oxL2/8f3jyrtar5G0eCEfDlNOhj3VLUF8hqyMmgSYhFKgMd5af1ev2 i7t8Q+tdgJoHxB58wdv+2ZW7+jpeUsvX4DKyN2bx4/z5eSuItVdwbZ9msJoyRIn3pd 5Qm1FB2PqEyB0rfsJWcvP7ahMKzbe/X8fxsr9GWQ= Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [95.163.41.83]) (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 B122E64BAC0 for ; Fri, 6 Oct 2023 13:49:21 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org B122E64BAC0 Received: by smtp45.i.mail.ru with esmtpa (envelope-from ) id 1qoiOV-001IbH-0g; Fri, 06 Oct 2023 13:49:21 +0300 Date: Fri, 6 Oct 2023 13:49:18 +0300 To: Sergey Bronnikov Message-ID: References: <20231003133705.5700-1-max.kokryashkin@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Mailru-Src: smtp X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9FE0487E502468146EC9FB4099193202C3CCBF29BAB5B817F00894C459B0CD1B9E38491352D17C2E551E675E067CB7234C27699C6A6731D0B30CA33227962A514 X-C1DE0DAB: 0D63561A33F958A5D66087D755AE2E0058E8AEEC6D53FF2CA7A26B0606B3661FF87CCE6106E1FC07E67D4AC08A07B9B0B355ED1E20F5346ACB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0AD5177F0B940C8B66ECE892A7B2722663E91682638B966EB3F662256BEEFA9527FA381826CBE184F59FACA98449E5BDE198704A1A4B8A81B5EFDD41E47890FD5F434E465DE12F54AB537FD76D11AF80A0DB5A64443778D1E2A32B41C3BF1044928EA455F16B58544A21C197AAF4D2E4732965026E5D17F6739C77C69D99B9914278E50E1F0597A6FD5CD72808BE417F3B9E0E7457915DAA85F X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojeMEJQfrq7nodg7HBDj6Rkg== X-Mailru-Sender: 11C2EC085EDE56FA38FD4C59F7EFE407BE20C6C8A063455AAB96F90DA268FE4B0E2E03FEF34853A5D51284F0FE6F529ABC7555A253F5B200DF104D74F62EE79D27EC13EC74F6107F4198E0F3ECE9B5443453F38A29522196 X-Mras: OK Subject: Re: [Tarantool-patches] [PATCH luajit v2] Mark CONV as non-weak, to prevent elimination of its side-effect. 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: Maxim Kokryashkin via Tarantool-patches Reply-To: Maxim Kokryashkin Cc: Maksim Kokryashkin , tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" On Tue, Oct 03, 2023 at 07:24:03PM +0300, Sergey Bronnikov wrote: > Hi, Max > > > thanks for the patch! > > On 10/3/23 16:37, Maksim Kokryashkin wrote: > > > > > > --- /dev/null > > +++ b/test/tarantool-tests/mark-conv-non-weak.test.lua > > @@ -0,0 +1,115 @@ > > +local tap = require('tap') > > +local test = tap.test('mark-conv-non-weak'):skipcond({ > > + ['Test requires JIT enabled'] = not jit.status(), > > +}) > > + > > +test:plan(1) > > + > > +local data = {0.1, 0, 0.1, 0, 0 / 0} > > Is it possible to reduce a number of elements in the table? > > I would add a comment where describe why exactly these magic values were > chosen. > > > +local sum = 0 > > + > > +jit.opt.start('hotloop=2', 'hotexit=2') > Why values are equal to 2 and not 1, that we usually set in tests? After a bit of tinkering with the repro I've managed to reduce it further. See the diff below. > > + > > +-- XXX: The test fails before the patch only > > +-- for `DUALNUM` mode. All of the IRs below are > > +-- produced by the corresponding LuaJIT build. > > + > > +-- When the trace is recorded, the IR > > +-- is the following before the patch: > > +---- TRACE 1 IR > > +-- .... SNAP #0 [ ---- ---- ---- ---- ---- ---- ---- ---- ---- ] > > +-- 0001 u8 XLOAD [0x100dac521] V > > +-- 0002 int BAND 0001 +12 > > +-- 0003 > int EQ 0002 +0 > > +-- 0004 > int SLOAD #8 T > > +-- .... SNAP #1 [ ---- ---- ---- ---- ---- ---- ---- ---- ---- ] > > +-- 0005 > num SLOAD #3 T > > +-- 0006 num CONV 0004 num.int > > +-- 0007 + num ADD 0006 0005 > > +-- 0008 > fun SLOAD #4 T > > +-- 0009 > tab SLOAD #5 T > > +-- 0010 > int SLOAD #6 T > > +-- 0011 > fun EQ 0008 ipairs_aux > > +-- 0012 + int ADD 0010 +1 > > +-- 0013 int FLOAD 0009 tab.asize > > +-- 0014 > int ABC 0013 0012 > > +-- 0015 p64 FLOAD 0009 tab.array > > +-- 0016 p64 AREF 0015 0012 > > +-- 0017 >+ num ALOAD 0016 > > +-- .... SNAP #2 [ ---- ---- ---- 0007 ---- ---- 0012 0012 0017 ] > > +-- 0018 ------ LOOP ------------ > > +-- 0019 u8 XLOAD [0x100dac521] V > > +-- 0020 int BAND 0019 +12 > > +-- 0021 > int EQ 0020 +0 > > +-- 0022 > int CONV 0017 int.num > > +-- .... SNAP #3 [ ---- ---- ---- 0007 ---- ---- 0012 0012 0017 ] > > +-- 0023 + num ADD 0017 0007 > > +-- 0024 + int ADD 0012 +1 > > +-- 0025 > int ABC 0013 0024 > > +-- 0026 p64 AREF 0015 0024 > > +-- 0027 >+ num ALOAD 0026 > > +-- 0028 num PHI 0017 0027 > > +-- 0029 num PHI 0007 0023 > > +-- 0030 int PHI 0012 0024 > > +---- TRACE 1 stop -> loop > > + > > +---- TRACE 1 exit 0 > > +---- TRACE 1 exit 3 > > +-- > > +-- And the following after the patch: > > +---- TRACE 1 IR > > +-- .... SNAP #0 [ ---- ---- ---- ---- ---- ---- ---- ---- ---- ] > > +-- 0001 u8 XLOAD [0x102438521] V > > +-- 0002 int BAND 0001 +12 > > +-- 0003 > int EQ 0002 +0 > > +-- 0004 > int SLOAD #8 T > > +-- .... SNAP #1 [ ---- ---- ---- ---- ---- ---- ---- ---- ---- ] > > +-- 0005 > num SLOAD #3 T > > +-- 0006 num CONV 0004 num.int > > +-- 0007 + num ADD 0006 0005 > > +-- 0008 > fun SLOAD #4 T > > +-- 0009 > tab SLOAD #5 T > > +-- 0010 > int SLOAD #6 T > > +-- 0011 > fun EQ 0008 ipairs_aux > > +-- 0012 + int ADD 0010 +1 > > +-- 0013 int FLOAD 0009 tab.asize > > +-- 0014 > int ABC 0013 0012 > > +-- 0015 p64 FLOAD 0009 tab.array > > +-- 0016 p64 AREF 0015 0012 > > +-- 0017 >+ num ALOAD 0016 > > +-- .... SNAP #2 [ ---- ---- ---- 0007 ---- ---- 0012 0012 0017 ] > > +-- 0018 ------ LOOP ------------ > > +-- 0019 u8 XLOAD [0x102438521] V > > +-- 0020 int BAND 0019 +12 > > +-- 0021 > int EQ 0020 +0 > > +-- 0022 > int CONV 0017 int.num > > +-- .... SNAP #3 [ ---- ---- ---- 0007 ---- ---- 0012 0012 0017 ] > > +-- 0023 + num ADD 0017 0007 > > +-- 0024 + int ADD 0012 +1 > > +-- 0025 > int ABC 0013 0024 > > +-- 0026 p64 AREF 0015 0024 > > +-- 0027 >+ num ALOAD 0026 > > +-- 0028 num PHI 0017 0027 > > +-- 0029 num PHI 0007 0023 > > +-- 0030 int PHI 0012 0024 > > +---- TRACE 1 stop -> loop > > + > > +---- TRACE 1 exit 0 > > +---- TRACE 1 exit 2 > > +-- > > +-- Before the patch, the `0022 > int CONV 0017 int.num` > I see that IR "0022 > int CONV ..." is present in both IR traces... Yep, they are omitted due to DCE and it happens on the trace assembly stage. Dropped a comment. > > +-- instruction is omitted due to DCE, which results in the > > +-- third side exit being taken, instead of the second, > > +-- and, hence, incorrect summation. After the patch, `CONV` > > +-- is left intact and is not omitted; it remains as a guarded > > +-- instruction, so the second side exit is taken and sum is > > +-- performed correctly. > > + > > +for _, val in ipairs(data) do > > + if val == val then > > + sum = sum + val > > + end > > +end Here is the diff with changes. Branch is force-pushed: === diff --git a/test/tarantool-tests/mark-conv-non-weak.test.lua b/test/tarantool-tests/mark-conv-non-weak.test.lua index f54f30ba..b71be4da 100644 --- a/test/tarantool-tests/mark-conv-non-weak.test.lua +++ b/test/tarantool-tests/mark-conv-non-weak.test.lua @@ -4,11 +4,13 @@ local test = tap.test('mark-conv-non-weak'):skipcond({ }) test:plan(1) +-- XXX: These values were chosen to create type instability +-- in the loop-carried dependency, so the checked `CONV int.num` +-- instruction is emitted. See `loop_unrool` in `lj_opt_loop.c`. +local data = {0, 0.1, 0, 0 / 0} +local sum = 0.1 -local data = {0.1, 0, 0.1, 0, 0 / 0} -local sum = 0 - -jit.opt.start('hotloop=2', 'hotexit=2') +jit.opt.start('hotloop=1') -- XXX: The test fails before the patch only -- for `DUALNUM` mode. All of the IRs below are @@ -104,6 +106,9 @@ jit.opt.start('hotloop=2', 'hotexit=2') -- is left intact and is not omitted; it remains as a guarded -- instruction, so the second side exit is taken and sum is -- performed correctly. +-- +-- Note that DCE happens on the assembly part of the trace +-- compilation. That is why `CONV` is present in both IRs. for _, val in ipairs(data) do if val == val then === > > + > > +test:ok(sum == sum, 'NaN check was not omitted') > > +test:done(true) > > -- > > 2.39.3 (Apple Git-145) > >