Re: [Tarantool-patches] [PATCH luajit v2] Mark CONV as non-weak, to prevent elimination of its side-effect.

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Sergey!
Thanks for the review!
See my replies below.

On Mon, Oct 09, 2023 at 12:27:35PM +0300, Sergey Kaplun wrote:
> Hi, Maxim!
> Thanks for the patch!
> LGTM, just a minor nit below.
> 
> On 06.10.23, Maxim Kokryashkin wrote:
> 
> <snipped>
> 
> > > > +-- 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
> 
> Do we really need to start the sum from 0.1, why just not 0?
Yep, we really do. Dropped a comment for that, branch is force-pushed.
Here is the diff:
===
diff --git a/test/tarantool-tests/mark-conv-non-weak.test.lua b/test/tarantool-tests/mark-conv-non-weak.test.lua
index b71be4da..331ca25e 100644
--- a/test/tarantool-tests/mark-conv-non-weak.test.lua
+++ b/test/tarantool-tests/mark-conv-non-weak.test.lua
@@ -8,6 +8,11 @@ test:plan(1)
 -- 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}
+
+--- XXX: The sum is required to be initialized with a non-zero
+-- floating point value; otherwise, `0023  + num ADD    0017  0007`
+-- instruction in the IR below becomes `ADDOV` and the `CONV int.num`
+-- conversion is used by it.
 local sum = 0.1
 
 jit.opt.start('hotloop=1')
===
<snipped>