From: Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Maksim Kokryashkin <max.kokryashkin@gmail.com>, tarantool-patches@dev.tarantool.org, skaplun@tarantool.org, m.kokryashkin@tarantool.org Subject: Re: [Tarantool-patches] [PATCH luajit v2] Mark CONV as non-weak, to prevent elimination of its side-effect. Date: Tue, 3 Oct 2023 19:24:03 +0300 [thread overview] Message-ID: <e8455b36-f2c7-3f0b-0f43-10bed212d6b6@tarantool.org> (raw) In-Reply-To: <20231003133705.5700-1-max.kokryashkin@gmail.com> Hi, Max thanks for the patch! On 10/3/23 16:37, Maksim Kokryashkin wrote: <snipped> > --- /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? > + > +-- 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... > +-- 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 > + > +test:ok(sum == sum, 'NaN check was not omitted') > +test:done(true) > -- > 2.39.3 (Apple Git-145) >
next prev parent reply other threads:[~2023-10-03 16:24 UTC|newest] Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-10-03 13:37 Maksim Kokryashkin via Tarantool-patches 2023-10-03 16:24 ` Sergey Bronnikov via Tarantool-patches [this message] 2023-10-06 10:49 ` Maxim Kokryashkin via Tarantool-patches 2023-10-09 9:27 ` Sergey Kaplun via Tarantool-patches 2023-10-13 10:11 ` Maxim Kokryashkin via Tarantool-patches 2023-10-23 14:59 ` Sergey Kaplun via Tarantool-patches 2023-10-03 16:25 ` Sergey Bronnikov via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=e8455b36-f2c7-3f0b-0f43-10bed212d6b6@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=m.kokryashkin@tarantool.org \ --cc=max.kokryashkin@gmail.com \ --cc=sergeyb@tarantool.org \ --cc=skaplun@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH luajit v2] Mark CONV as non-weak, to prevent elimination of its side-effect.' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox