Re: [Tarantool-patches] [PATCH luajit v2] Fix narrowing of unary minus.

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

[-- Attachment #1: Type: text/plain, Size: 3390 bytes --]

Hi!

Thanks for the fixes, LGTM now.

Sergos


> On 29 Sep 2022, at 12:58, Maxim Kokryashkin <m.kokryashkin@tarantool.org> wrote:
> 
> Hi, Sergos!
> Thanks for the questions!
> Please consider my answers amd changes below.
> 
> > LuaJIT narrowing optimization during BC_UNM recording may ignore
> > information about sign of zero for integer types of IR. So far the
> > resulting value on a trace is not the same as for the interpreter.
> 
> I didn’t get the point - how is it detected, otherwise than tostring()?
> If so - should we change the tostring() instead?
> Otherwise - we need a test that exposes this difference
> I’ve changed the tests, so it’s now more clear that zero sign can affect arithmetic.
> Branch is force-pushed.
> Here is the diff:
> ===============================================
> --- a/test/tarantool-tests/gh-6976-narrowing-of-unary-minus.test.lua
> +++ b/test/tarantool-tests/gh-6976-narrowing-of-unary-minus.test.lua
> @@ -2,7 +2,7 @@
>  local test = tap.test('gh-6976-narrowing-of-unary-minus')
>  test:plan(2)
>  
> -jit.opt.start('hotloop=1', 'hotexit=1')
> +jit.opt.start('hotloop=1')
>  
>  local function check(routine)
>    jit.off()
> @@ -20,32 +20,29 @@
>    return true
>  end
>  
> -test:ok(
> -  check(
> -    function()
> -      local res = require('table.new')(3, 0)
> -      for _ = 1, 3 do
> -        local zero = 0
> -        zero = -zero
> -        table.insert(res, tostring(zero))
> -      end
> -      return res
> -    end
> -  ),
> -  'incorrect recording for zero'
> -)
> -
> -test:ok(
> -  check(
> -    function()
> -      local res = require('table.new')(3, 0)
> -      for i = 2, 0, -1 do
> -        table.insert(res, tostring(-i))
> -      end
> -      return res
> -    end
> -  ),
> -  'assertion guard fail'
> -)
> +test:ok(check(function()
> +  -- We use `table.new()` here to avoid trace
> +  -- exits due to table rehashing.
> +  local res = require('table.new')(3, 0)
> +  for _ = 1, 3 do
> +    local zero = 0
> +    zero = -zero
> +    -- There is no difference between 0 and -0 from
> +    -- arithmetic perspective, unless you try to divide
> +    -- something by them.
> +    -- `1 / 0 = inf` and `1 / -0 = -inf`
> +    table.insert(res, 1 / zero)
> +  end
> +  return res
> +end), 'incorrect recording for zero')
> +
> +test:ok(check(function()
> +  -- See the comment about `table.new()` above.
> +  local res = require('table.new')(3, 0)
> +  for i = 2, 0, -1 do
> +    table.insert(res, 1 / -i)
> +  end
> +  return res
> +end),'assertion guard fail')
>  
>  os.exit(test:check() and 0 or 1)
> ===============================================
> <snipped>
> 
> > This patch fixes the non-DUALNUM mode behaviour. When the zero value is
> > identified during recording it should be cast to number so IR_CONV is
> > emitted. Also, this patch adds assertion guard checking that value on
> > which operation of unary minus is performed isn't zero.
> 
> Does it mean I will exit the trace every time I met a `x = 0; x = -x` in it?
> No, that assertion guard takes you back to the interpreter only if a
> trace for unary minus was recorded considering `x` as a non-zero value,
> and at some point in this trace `x` became zero.

ok, it looks reasonable.

>  
>  
> Best regards,
> Maxim Kokryashkin
>  


[-- Attachment #2: Type: text/html, Size: 5380 bytes --]