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

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

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


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.
 
 
Best regards,
Maxim Kokryashkin
 

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