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 B40165B2C6F; Thu, 24 Aug 2023 10:31:14 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org B40165B2C6F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1692862274; bh=0HfUwgusjIT1VW8q2U3OXjFPiJ68roAP3AWmejyL9z0=; 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=WGfRH8wSJFOzxbdBWW+6t33YeHnzbcTnbo0WmcwyR/DaS0PDzvDmCz0Ez7baFdYIq kV7u2ANDS58D8+Zjl8DjUGlpAAD62aRIJbgMys5cJiLycRgw1dX9YV/GpMJ2D1hNOP WJOKBOuecEJRAPde7vVLOyuaRypvwzM6SJUc/wqY= Received: from smtpng3.i.mail.ru (smtpng3.i.mail.ru [94.100.177.149]) (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 A66354DE981 for ; Thu, 24 Aug 2023 10:31:13 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A66354DE981 Received: by smtpng3.m.smailru.net with esmtpa (envelope-from ) id 1qZ4oC-00063b-Ol; Thu, 24 Aug 2023 10:31:13 +0300 Date: Thu, 24 Aug 2023 10:26:28 +0300 To: Maksim Kokryashkin Message-ID: References: <20230712095212.20480-1-max.kokryashkin@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230712095212.20480-1-max.kokryashkin@gmail.com> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 78E4E2B564C1792B X-77F55803: 4F1203BC0FB41BD93C8852532D76B9E3B62DC7F09619FD734679E2B1BE667DC6182A05F53808504059B9CBE6E3C8E5857E4552317066C021744061A033F95344A414F6843776DB71 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE766DBE83FD69AB645EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637EA9DEEAA3ECF8E948638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D844E0B720F66A6EE0EBA2904ED3E4CB47117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCD78892F4CDBD81DDA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F44604297287769387670735201E561CDFBCA1751F2CC0D3CB04F14752D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6F7FD1A3A8AE6177F089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A5006EC5628B0A2FE7D0864305E522EF0E26D5CF7D9DE3B0CDF87CCE6106E1FC07E67D4AC08A07B9B06A1CB4668A9CA5FACB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CFB7DBA836D8D899DB3DB140FD11E074FE71CBF9462C92D91F5AD37D28EC61D2F5B94BA8791A30448065277D9847CC050AC000CD366F913C1822C153F5521E422BA74DFFEFA5DC0E7F02C26D483E81D6BE5EF9655DD6DEA7D65774BB76CC95456EEC5B5AD62611EEC62B5AFB4261A09AF0 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojbN4zljfpmjE4rK9ZLHYEbg== X-DA7885C5: E10C30EC486F598768416AF93F945C9661907AB4331F81C462E4F43E991535CE262E2D401490A4A0DB037EFA58388B346E8BC1A9835FDE71 X-Mailru-Sender: 689FA8AB762F73930F533AC2B33E986B82A33DAB523E5BF592AB5A497B82A33F0FBE9A32752B8C9C2AA642CC12EC09F1FB559BB5D741EB962F61BD320559CF1EFD657A8799238ED55FEEDEB644C299C0ED14614B50AE0675 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH] 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: 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 patch! Please consider my comment below. On 12.07.23, Maksim Kokryashkin wrote: > From: Mike Pall > > An unused guarded CONV int.num cannot be omitted in general. > > (cherry-picked from commit 881d02d3117838acaf4fb844332c8e33cc95c8c5) > > In some cases, an unused `CONV` omission may lead to a guard > absence, resulting in invalid control flow branching and > undefined behavior. For a comprehensive example of > the described situation, please refer to the comment > in `test/tarantool-tests/mark-conv-non-weak.test.lua`. > > Maxim Kokryashkin: > * added the description and the test for the problem > > Part of tarantool/tarantool#8825 > --- > + > +jit.opt.start('hotloop=1', 'hotexit=1') > + > +-- When the last trace is recorded, the traced bytecode > +-- is the following before the patch: > +-- ---- TRACE 4 start 2/3 test.lua:6 > +-- 0018 ADDVV 1 1 6 > +-- 0019 ITERC 5 3 3 > +-- 0000 . FUNCC ; ipairs_aux > +-- 0020 JITERL 5 1 > +-- 0021 GGET 2 7 ; "assert" > +-- 0022 ISEQV 1 1 > +-- 0023 JMP 4 => 0026 > +-- 0024 KPRI 4 1 > +-- 0025 JMP 5 => 0027 > +-- 0027 CALL 2 1 2 > +-- 0000 . FUNCC ; assert > +-- > +-- And the following after the patch: > +-- ---- TRACE 4 start 2/2 test.lua:5 > +-- 0016 ISNEV 6 6 > +-- 0017 JMP 7 => 0019 > +-- 0019 ITERC 5 3 3 > +-- 0000 . FUNCC ; ipairs_aux > +-- 0020 JITERL 5 1 > +-- 0021 GGET 2 7 ; "assert" > +-- 0022 ISEQV 1 1 > +-- 0023 JMP 4 => 0026 > +-- 0026 KPRI 4 2 > +-- 0027 CALL 2 1 2 > +-- 0000 . FUNCC ; assert > +-- 0028 RET0 0 1 > +-- > +-- The crucial difference here is the abscent > +-- `ISNEV` in the first case, which produces the > +-- desired guarded `CONV`, when translated to IR. There are two important statements: As you mentioned, the case is observed only in the DUALNUM mode. As mentioned in the commit, this is due to `CONV int.num` IRs. The comment for the test itself is misleading the reason of misbehaviour isn't the wrong bytecode in the recording -- it's the consequence. Let's use the reproducer, but with hotloop=2 to decrease amount of traces: | LUA_PATH="src/?.lua;;" src/luajit -jdump=X -e " | local data = {0.1, 0, 0.1, 0, 0 / 0} | local sum = 0 | | jit.opt.start('hotloop=2', 'hotexit=1') | | for _, val in ipairs(data) do | if val == val then | sum = sum + val | end | end | | math.fmod(1,1) | | print(sum) | " This is output before the patch (see `<-- !!!` marks): | ---- TRACE 1 exit 0 | 000056449701ff26 0000000041bb9880 00000000409cf228 0000000041bb9e94 | 00007fffa700cfa0 00000000409c2378 00000000409cf228 00000000409c3320 | 0000000000000000 0000000000000000 00007fd4e2d38938 00000000fffffffe | 00007fffa700d3e8 000056446cea132a 00000000409c3098 0000000041bb9ee0 | +4.6863058149307e-310 +4.6863058151824e-310 +4.6863058151959e-310 +4.6863058152213e-310 | +4.6863058152444e-310 +4.6863058152873e-310 +4.6863058152729e-310 +4.6863058152444e-310 | +4.6863058152213e-310 +0 -0.66666666666667 +0.7999999995324 | -1.1429096284595 +0 +0 +0 | ---- TRACE 1 exit 3 <-- !!! | 0000000041bb9848 0000000000000006 0000000041bb9828 0000000000000005 | 00007fffa700cfa0 0000000000000006 00000000409cf228 00000000409c3320 | 0000000000000000 0000000000000029 00007fd4e2d3ab28 00000000fffffffe | 00007fffa700d3e8 000056446cea132a 00000000409c3098 0000000041bb9ee0 | +4.6863058149307e-310 +4.6863058151824e-310 +4.6863058151959e-310 +4.6863058152213e-310 | nan +0.2 nan nan <-- !!! | +4.6863058152213e-310 +0 -0.66666666666667 +0.7999999995324 | -1.1429096284595 +0 +0 +0 | nan And this is after: | ---- TRACE 1 exit 0 | 0000556168ccff06 0000000041f8a880 0000000040275220 0000000041f8ae94 | 00007ffda4a82900 0000000040268378 0000000040275220 0000000040269320 | 0000000000000000 0000000000000000 00007fdd97c11938 00000000fffffffe | 00007ffda4a82d48 000055613eb5132a 0000000040269090 0000000041f8aee0 | +4.6380982089931e-310 +4.6380982092477e-310 +4.6380982092617e-310 +4.6380982092839e-310 | +4.6380982093151e-310 +4.6380982093497e-310 +4.6380982093353e-310 +4.6380982093068e-310 | +4.6380982092837e-310 +0 -0.66666666666667 +0.7999999995324 | -1.1429096284595 +0 +0 +0 | ---- TRACE 1 exit 2 <-- !!! | 0000000041f8a848 0000000000000006 0000000041f8a828 0000000041f8ae94 | 00007ffda4a82900 0000000000000005 0000000040275220 0000000080000000 | 0000000000000000 0000000000000029 00007fdd97c13b28 00000000fffffffe | 00007ffda4a82d48 000055613eb5132a 0000000040269090 0000000041f8aee0 | +4.6380982089931e-310 +4.6380982092477e-310 +4.6380982092617e-310 +4.6380982092839e-310 | nan +0.2 nan -2147483648 <-- !!! | +4.6380982092837e-310 +0 -0.66666666666667 +0.7999999995324 | -1.1429096284595 +0 +0 +0 | 0.2 As you can see different side exits are taken, and **this** leads to different bytecodes on the recording of the side trace (this is observed in the dump of the bytecode). So, the second side exit doesn't contain NaN. The IRs of the first trace are the following: | ---- TRACE 1 start (command line):7 | ---- TRACE 1 IR | .... SNAP #0 [ ---- ---- ---- ---- ---- ---- ---- ---- ] | 0001 u8 XLOAD [0x41b864c1] V | 0002 int BAND 0001 +12 | 0003 > int EQ 0002 +0 | 0004 > int SLOAD #7 T | .... SNAP #1 [ ---- ---- ---- ---- ---- ---- ---- ---- ] | 0005 > num SLOAD #2 T | 0006 xmm7 num CONV 0004 num.int | 0007 xmm7 + num ADD 0006 0005 | 0008 > fun SLOAD #3 T | 0009 rdx > tab SLOAD #4 T | 0010 rbp > int SLOAD #5 T | 0011 > fun EQ 0008 ipairs_aux | 0012 rbp + int ADD 0010 +1 | 0013 rcx int FLOAD 0009 tab.asize | 0014 > int ABC 0013 0012 | 0015 rax p32 FLOAD 0009 tab.array | 0016 p32 AREF 0015 0012 | 0017 xmm6 >+ num ALOAD 0016 | .... SNAP #2 [ ---- ---- 0007 ---- ---- 0012 0012 0017 ] | 0018 ------------ LOOP ------------ | 0019 u8 XLOAD [0x41b864c1] V | 0020 int BAND 0019 +12 | 0021 > int EQ 0020 +0 | 0022 rdi > int CONV 0017 int.num | .... SNAP #3 [ ---- ---- 0007 ---- ---- 0012 0012 0017 ] | 0023 xmm7 + num ADD 0017 0007 | 0024 rbp + int ADD 0012 +1 | 0025 > int ABC 0013 0024 | 0026 p32 AREF 0015 0024 | 0027 xmm6 >+ num ALOAD 0026 | 0028 xmm6 num PHI 0017 0027 | 0029 xmm7 num PHI 0007 0023 | 0030 rbp int PHI 0012 0024 | 0031 rbx nil RENAME 0012 #3 | 0032 xmm4 nil RENAME 0017 #2 | 0033 xmm5 nil RENAME 0007 #2 | ---- TRACE 1 stop -> loop The skipped due to DCE conversion is the following: | 0022 rdi > int CONV 0017 int.num Also, from gdb we can check, that omitted conversion has `IRCONV_CHECK` mode (I suppose that only check conversion can't be omitted, but Mike prefer more coarse fix). Also, it should be mentioned that we get this conversion due to type instability in loop carried dependency (see `loop_unrool()` in ): | else if (irt_isnum(irr->t) && irt_isinteger(t)) /* Fix num->int. */ | ref = tref_ref(emitir(IRTGI(IR_CONV), ref, | IRCONV_INT_NUM|IRCONV_CHECK)); All this confusion is because of the too complex traces in the given example. We have 4 traces, but I suppose that should be reduced to only one with the necessary side exit to restore NaN value after missing check. I suggest to look forward for all places, where such int.num check conversion is used: | src/lj_record.c:491: tr[i] = emitir(IRTGI(IR_CONV), tr[i], IRCONV_INT_NUM|IRCONV_CHECK); | src/lj_opt_loop.c:349: IRCONV_INT_NUM|IRCONV_CHECK)); | src/lj_ffrecord.c:529: tr = emitir(IRTGI(IR_CONV), tr, IRCONV_INT_NUM|IRCONV_CHECK); | src/lj_opt_narrow.c:604: rc = emitir(IRTGI(IR_CONV), rc, IRCONV_INT_NUM|IRCONV_CHECK); to create a testcase with one trace. > +-- > +-- Since there is no guard, NaN is added to the sum, > +-- despite the test case logic. > + > +for _, val in ipairs(data) do > + if val == val then > + sum = sum + val > + end > +end > + > +test:ok(sum == sum, 'NaN check was not omitted') > + > +os.exit(test:check() and 0 or 1) > -- > 2.39.2 (Apple Git-143) > -- Best regards, Sergey Kaplun