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

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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?

>  
> -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')

<snipped>

-- 
Best regards,
Sergey Kaplun