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

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

[Maksim Kokryashkin via Tarantool-patches]

From: Mike Pall <mike>

With DUALNUM mode disabled, unary minus narrowing can produce
inconsistent results on zeros, returning both -0 and 0 on the
same chunk of code.

This patch fixes the inconsistency by adding checks for zeros.

Part of tarantool/tarantool#7230
Resolves tarantool/tarantool#6976
---
Issue: https://github.com/tarantool/tarantool/issues/6976
Branch: https://github.com/tarantool/luajit/tree/fckxorg/gh-6976-narrowing-of-unary-minus
PR: https://github.com/tarantool/tarantool/pull/7662

 src/lj_opt_narrow.c                           |  9 +++--
 .../gh-6976-narrowing-of-unary-minus.test.lua | 34 +++++++++++++++++++
 2 files changed, 41 insertions(+), 2 deletions(-)
 create mode 100644 test/tarantool-tests/gh-6976-narrowing-of-unary-minus.test.lua

diff --git a/src/lj_opt_narrow.c b/src/lj_opt_narrow.c
index cd96ca4b..bb61f97b 100644
--- a/src/lj_opt_narrow.c
+++ b/src/lj_opt_narrow.c
@@ -551,8 +551,13 @@ TRef lj_opt_narrow_unm(jit_State *J, TRef rc, TValue *vc)
 {
   rc = conv_str_tonum(J, rc, vc);
   if (tref_isinteger(rc)) {
-    if ((uint32_t)numberVint(vc) != 0x80000000u)
-      return emitir(IRTGI(IR_SUBOV), lj_ir_kint(J, 0), rc);
+    uint32_t k = (uint32_t)numberVint(vc);
+    if ((LJ_DUALNUM || k != 0) && k != 0x80000000u) {
+      TRef zero = lj_ir_kint(J, 0);
+      if (!LJ_DUALNUM)
+	emitir(IRTGI(IR_NE), rc, zero);
+      return emitir(IRTGI(IR_SUBOV), zero, rc);
+    }
     rc = emitir(IRTN(IR_CONV), rc, IRCONV_NUM_INT);
   }
   return emitir(IRTN(IR_NEG), rc, lj_ir_ksimd(J, LJ_KSIMD_NEG));
diff --git a/test/tarantool-tests/gh-6976-narrowing-of-unary-minus.test.lua b/test/tarantool-tests/gh-6976-narrowing-of-unary-minus.test.lua
new file mode 100644
index 00000000..caae8c34
--- /dev/null
+++ b/test/tarantool-tests/gh-6976-narrowing-of-unary-minus.test.lua
@@ -0,0 +1,34 @@
+require("utils").skipcond(
+  jit.arch ~= "x86" and jit.arch ~= "x64",
+  "DUALNUM mode must be disabled"
+)
+
+local tap = require("tap")
+local test = tap.test("gh-6976-narrowing-of-unary-minus")
+test:plan(1)
+
+jit.off()
+jit.flush()
+jit.opt.start('hotloop=1')
+jit.on()
+
+local has_error = false
+
+-- The first two iterations are needed to write trace in a way
+-- where `a` is treated like a non-zero integer value. After the
+-- first two iterations 'a' becomes a zero integer, but it is still
+-- processed by the same trace. Finally, roughly ten loop
+-- iterations later, a new trace is formed, where `a` is treated
+-- like a number.
+for i=1,20 do
+  local a = 1
+  if i > 2 then
+    a = 0
+  end
+  a = -a
+  has_error = has_error or tostring(a):match("%-%d") == nil
+end
+
+test:ok(has_error == false, "inconsistent unary minus result")
+os.exit(test:check() and 0 or 1)
+
-- 
2.32.1 (Apple Git-133)

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

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!

Thanks for the patch!

Please consider my comments below.

On 09.09.22, Maksim Kokryashkin wrote:
> From: Mike Pall <mike>
> 

Please, don't forget the common cherry-picked header:

| (cherry picked from commit 1e6e8aaa20626ac94cf907c69b0452f76e9f5fa5)

It is necessary to recognize the original commit without grepping of
LuaJIT source code.

> With DUALNUM mode disabled, unary minus narrowing can produce
> inconsistent results on zeros, returning both -0 and 0 on the
> same chunk of code.
> 
> This patch fixes the inconsistency by adding checks for zeros.

I suggest to reformulate these paragraphes like the following:

| 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.
| However, in DUALNUM mode with using of TValues containing integers the
| -0 value is represented in the same way as the 0 value and there is no
| difference between them.
|
| 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.

Feel free to modify it as you wish.

> 

Don't be shy to mention your impact in this patch (description and
tests) like it is done in previous backported commits.

> Part of tarantool/tarantool#7230
> Resolves tarantool/tarantool#6976

Minor: Igor prefers to place the resolved issue first (as far as this is
the main issue to close). I prefer the opposite order. But we've agreed
with Igor's way, so let use it for consistency.

> ---
> Issue: https://github.com/tarantool/tarantool/issues/6976
> Branch: https://github.com/tarantool/luajit/tree/fckxorg/gh-6976-narrowing-of-unary-minus
> PR: https://github.com/tarantool/tarantool/pull/7662
> 
>  src/lj_opt_narrow.c                           |  9 +++--
>  .../gh-6976-narrowing-of-unary-minus.test.lua | 34 +++++++++++++++++++
>  2 files changed, 41 insertions(+), 2 deletions(-)
>  create mode 100644 test/tarantool-tests/gh-6976-narrowing-of-unary-minus.test.lua
> 
> diff --git a/src/lj_opt_narrow.c b/src/lj_opt_narrow.c
> index cd96ca4b..bb61f97b 100644
> --- a/src/lj_opt_narrow.c
> +++ b/src/lj_opt_narrow.c
> @@ -551,8 +551,13 @@ TRef lj_opt_narrow_unm(jit_State *J, TRef rc, TValue *vc)
>  {
>    rc = conv_str_tonum(J, rc, vc);
>    if (tref_isinteger(rc)) {
> -    if ((uint32_t)numberVint(vc) != 0x80000000u)
> -      return emitir(IRTGI(IR_SUBOV), lj_ir_kint(J, 0), rc);
> +    uint32_t k = (uint32_t)numberVint(vc);
> +    if ((LJ_DUALNUM || k != 0) && k != 0x80000000u) {
> +      TRef zero = lj_ir_kint(J, 0);
> +      if (!LJ_DUALNUM)
> +	emitir(IRTGI(IR_NE), rc, zero);
> +      return emitir(IRTGI(IR_SUBOV), zero, rc);
> +    }

Side note:

I wonder how much it is better to use this brancn in narrowing
optimization for non-DUALNUMBER mode, as far as we even add additional
assertion guard for zero check (IR_NE).

Assume we have the following code:
| local r = 0
| for i = 1, 1e9 do
|   local a = -i
|   r = r + a
| end
| print(r)

Before the patch we've had the following IRs for LOOP part:

| ------ LOOP ------------
| 0009 >  int SUBOV  +0    0006
| 0010    num CONV   0009  num.int
| 0011  + num ADD    0010  0005
| 0012  + int ADD    0006  +1
| 0013 >  int LE     0012  +1000000000
| 0014    int PHI    0006  0012
| 0015    num PHI    0005  0011

And after patch:

| ------ LOOP ------------
| 0010 >  int NE     0007  +0
| 0011 >  int SUBOV  +0    0007
| 0012    num CONV   0011  num.int
| 0013  + num ADD    0012  0006
| 0014  + int ADD    0007  +1
| 0015 >  int LE     0014  +1000000000
| 0016    int PHI    0007  0014
| 0017    num PHI    0006  0013

This leads to the following additional instructions:

| test ebp, ebp
| jz 0x561394990018 ->2

I run 10 loops like the aforementioned one with 21e8 iterations.

The average time for 5 runs (of 10 loops) before the patch: 25.763s
The average time for 5 runs (of 10 loops) after  the patch: 25.885s

And, actually, if we skip this optimization with the following patch
(I just skip the branch with SUBOV emitting for non-DUALNUMBER mode):

===================================================================
diff --git a/src/lj_opt_narrow.c b/src/lj_opt_narrow.c
index bb61f97b..aaa5b3cd 100644
--- a/src/lj_opt_narrow.c
+++ b/src/lj_opt_narrow.c
@@ -552,7 +552,7 @@ TRef lj_opt_narrow_unm(jit_State *J, TRef rc, TValue *vc)
   rc = conv_str_tonum(J, rc, vc);
   if (tref_isinteger(rc)) {
     uint32_t k = (uint32_t)numberVint(vc);
-    if ((LJ_DUALNUM || k != 0) && k != 0x80000000u) {
+    if ((LJ_DUALNUM) && k != 0x80000000u) {
       TRef zero = lj_ir_kint(J, 0);
       if (!LJ_DUALNUM)
         emitir(IRTGI(IR_NE), rc, zero);
===================================================================

The IRs for the loop part are the following:
| ------ LOOP ------------
| 0010    num CONV   0007  num.int
| 0011  + num SUB    0006  0010
| 0012  + int ADD    0007  +1
| 0013 >  int LE     0012  +1000000000
| 0014    int PHI    0007  0012
| 0015    num PHI    0006  0011

As you can see we have even 2 IRs instead of 4 (we skip assertion guard
and creation of negative number -- we just use substitution instead of
addition). I suppose for non-DUALNUM mode this is the __common__ case --
operations in doubles, not integers (it may be different for cdata
indexes -- I don't check it).

As far as we have only 3 instructions for this loop payload:

| xorps xmm6, xmm6
| cvtsi2sd xmm6, ebp
| subsd xmm7, xmm6

instead of old 8 instructions:
| test ebp, ebp
| jz 0x555995dc0018 ->2
| xor ebx, ebx
| sub ebx, ebp
| jo 0x555995dc0018 ->2
| xorps xmm6, xmm6
| cvtsi2sd xmm6, ebx
| addsd xmm7, xmm6

the results are slightly better: 25.368s

I understand, that the benchmark is kinda synthetic, but still...

P.S. For others payload the result can be the diffirent, but this code
looks like the most common, as I said before.

I mean constructions like the following:
| local a = -b;
| c = d + a

Also, it is converted to number anyway in HREF:

| 0021 >  int NE     0018  +0
| 0022 >  int SUBOV  +0    0018
| 0023    int FLOAD  0004  tab.asize
| 0024 >  int EQ     0023  +0
| 0025    num CONV   0022  num.int
| 0026    p32 HREF   0004  0025

with the following assembly:

| mov edi, [0x40162540]
| mov esi, [rsp+0x10]
|
| test ebp, ebp
| jz 0x558aff9c0018 ->2
| xor r15d, r15d
| sub r15d, ebp
| jo 0x558aff9c0018 ->2
|
| mov ebx, [rsi+0x18]
| test ebx, ebx
| jnz 0x558aff9c0018        ->2
|
| xorps xmm7, xmm7
| cvtsi2sd xmm7, r15d
|
| movq rdx, xmm7

for the following compiled code:
| local a = tab[-i] -- type(tab) == 'table'

Without NE, SUBOV emitting IRs are the following:
| 0020    num CONV   0017  num.int
| 0021    num NEG    0020  0003
| 0022    int FLOAD  0005  tab.asize
| 0023 >  int EQ     0022  +0
| 0024    p32 HREF   0005  0021

with the following assembly:

| mov edi, [0x40093540]
| mov esi, [rsp+0x10]
|
| xorps xmm7, xmm7
| cvtsi2sd xmm7, ebp
| movsd [rsp+0x8], xmm7
| movaps xmm6, xmm7
| xorps xmm6, [0x400936f0]
|
| mov ebx, [rsi+0x18]
| test ebx, ebx
| jnz 0x55c341180018        ->2
|
| movq rdx, xmm6

Average time when run 3 chunks like the following:
| local r = require"table.new"(1e8, 0)
| for i = 1, 1e8 do
|   local a = r[-i]
|   r[i] = a
| end
| print(r)

With NE and SUBOV: 7.769s
Without: 7.670s

Also, it is obviously more natural to use this cast in DUALNUMBER mode,
where integer TValues are usual.

P.P.S. The patch itself as a bugfix LGTM.

P.P.P.S. Thoughts? :)

>      rc = emitir(IRTN(IR_CONV), rc, IRCONV_NUM_INT);
>    }
>    return emitir(IRTN(IR_NEG), rc, lj_ir_ksimd(J, LJ_KSIMD_NEG));
> diff --git a/test/tarantool-tests/gh-6976-narrowing-of-unary-minus.test.lua b/test/tarantool-tests/gh-6976-narrowing-of-unary-minus.test.lua
> new file mode 100644
> index 00000000..caae8c34
> --- /dev/null
> +++ b/test/tarantool-tests/gh-6976-narrowing-of-unary-minus.test.lua

Side note: This 6976 issue has no description, but OK :)

> @@ -0,0 +1,34 @@
> +require("utils").skipcond(

Minor: Use single quotes instead of double quotes.

> +  jit.arch ~= "x86" and jit.arch ~= "x64",
> +  "DUALNUM mode must be disabled"
> +)

Not sure that we must skip other arches, see argumentation below.

> +
> +local tap = require("tap")
> +local test = tap.test("gh-6976-narrowing-of-unary-minus")
> +test:plan(1)
> +
> +jit.off()
> +jit.flush()
> +jit.opt.start('hotloop=1')
> +jit.on()

Don't see the reason to restart the JIT compiler at this point. Just set
the option is enough.

> +
> +local has_error = false
> +
> +-- The first two iterations are needed to write trace in a way
> +-- where `a` is treated like a non-zero integer value. After the
> +-- first two iterations 'a' becomes a zero integer, but it is still
> +-- processed by the same trace. Finally, roughly ten loop

These ten iterations are needed for side trace compilation.
You can set hotexit=1 to start compiling trace at the first trace exit.
But this is not necessary, see argumentation below.

> +-- iterations later, a new trace is formed, where `a` is treated
> +-- like a number.
> +for i=1,20 do
> +  local a = 1
> +  if i > 2 then
> +    a = 0
> +  end
> +  a = -a
> +  has_error = has_error or tostring(a):match("%-%d") == nil
> +end

Actually, I don't see the reason for such complicated test case.
We can just use the following (with better naming, etc.):

| local res = require'table.new'(3, 0)
|
| for _ = 1, 3 do
|   local a = 0
|   a = -a
|   table.insert(res, tostring(a))
| end

We use `table.new()` here to avoid trace exits due to table rehashing
(such trace exits return us to the interpreter and you see the -0 value
instead of expected 0, when test failure expected without the patch).

In your test case you actually want to verify, that behaviour for JIT
compiled code and interpreter is the same. So we can run the loop above
with JIT on/off and verify those tables' content is the same. From this
point of view, there is no different for the DUALNUMBER mode in this
test -- the results should be equal for the JIT and the interpreter (but
it will be nice manner to write the comment with expected content for
both modes).

> +
> +test:ok(has_error == false, "inconsistent unary minus result")

For now we check only _recording_ for 0.
But we should also check the added assertion guard. I suggest the test
like the following:

| local res2 = require"table.new"(3,0)
|
| for i = 2, 0, -1 do
|   table.insert(res2, tostring(-i))
| end

> +os.exit(test:check() and 0 or 1)
> +
> -- 
> 2.32.1 (Apple Git-133)
> 

-- 
Best regards,
Sergey Kaplun

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

[Maksim Kokryashkin via Tarantool-patches]

From: Mike Pall <mike>

(cherry picked from commit 1e6e8aaa20626ac94cf907c69b0452f76e9f5fa5)

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.
However, in DUALNUM mode with using of TValues containing integers the
-0 value is represented in the same way as the 0 value and there is no
difference between them.

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.

Maxim Kokryashkin:
* added the description and the test for the problem

Resolves tarantool/tarantool#6976
Part of tarantool/tarantool#7230
---

Changes in v2:
- Fixed test and commit message as per review by Sergey

Branch and PR are updated.

 src/lj_opt_narrow.c                           |  9 +++-
 .../gh-6976-narrowing-of-unary-minus.test.lua | 51 +++++++++++++++++++
 2 files changed, 58 insertions(+), 2 deletions(-)
 create mode 100644 test/tarantool-tests/gh-6976-narrowing-of-unary-minus.test.lua

diff --git a/src/lj_opt_narrow.c b/src/lj_opt_narrow.c
index cd96ca4b..bb61f97b 100644
--- a/src/lj_opt_narrow.c
+++ b/src/lj_opt_narrow.c
@@ -551,8 +551,13 @@ TRef lj_opt_narrow_unm(jit_State *J, TRef rc, TValue *vc)
 {
   rc = conv_str_tonum(J, rc, vc);
   if (tref_isinteger(rc)) {
-    if ((uint32_t)numberVint(vc) != 0x80000000u)
-      return emitir(IRTGI(IR_SUBOV), lj_ir_kint(J, 0), rc);
+    uint32_t k = (uint32_t)numberVint(vc);
+    if ((LJ_DUALNUM || k != 0) && k != 0x80000000u) {
+      TRef zero = lj_ir_kint(J, 0);
+      if (!LJ_DUALNUM)
+	emitir(IRTGI(IR_NE), rc, zero);
+      return emitir(IRTGI(IR_SUBOV), zero, rc);
+    }
     rc = emitir(IRTN(IR_CONV), rc, IRCONV_NUM_INT);
   }
   return emitir(IRTN(IR_NEG), rc, lj_ir_ksimd(J, LJ_KSIMD_NEG));
diff --git a/test/tarantool-tests/gh-6976-narrowing-of-unary-minus.test.lua b/test/tarantool-tests/gh-6976-narrowing-of-unary-minus.test.lua
new file mode 100644
index 00000000..18778a55
--- /dev/null
+++ b/test/tarantool-tests/gh-6976-narrowing-of-unary-minus.test.lua
@@ -0,0 +1,51 @@
+local tap = require('tap')
+local test = tap.test('gh-6976-narrowing-of-unary-minus')
+test:plan(2)
+
+jit.opt.start('hotloop=1', 'hotexit=1')
+
+local function check(routine)
+  jit.off()
+  jit.flush()
+  local interp_res = routine()
+  jit.on()
+  local jit_res = routine()
+
+  for i = 1, #interp_res do
+    if interp_res[i] ~= jit_res[i] then
+      return false
+    end
+  end
+
+  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'
+)
+
+os.exit(test:check() and 0 or 1)
-- 
2.32.1 (Apple Git-133)

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

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!

Thanks for the fixes!
LGTM, except a few nits below.

On 20.09.22, Maksim Kokryashkin wrote:
> From: Mike Pall <mike>
> 

<snipped>

>  src/lj_opt_narrow.c                           |  9 +++-
>  .../gh-6976-narrowing-of-unary-minus.test.lua | 51 +++++++++++++++++++
>  2 files changed, 58 insertions(+), 2 deletions(-)
>  create mode 100644 test/tarantool-tests/gh-6976-narrowing-of-unary-minus.test.lua
> 
> diff --git a/src/lj_opt_narrow.c b/src/lj_opt_narrow.c
> index cd96ca4b..bb61f97b 100644
> --- a/src/lj_opt_narrow.c
> +++ b/src/lj_opt_narrow.c

<snipped>

> diff --git a/test/tarantool-tests/gh-6976-narrowing-of-unary-minus.test.lua b/test/tarantool-tests/gh-6976-narrowing-of-unary-minus.test.lua
> new file mode 100644
> index 00000000..18778a55
> --- /dev/null
> +++ b/test/tarantool-tests/gh-6976-narrowing-of-unary-minus.test.lua
> @@ -0,0 +1,51 @@
> +local tap = require('tap')
> +local test = tap.test('gh-6976-narrowing-of-unary-minus')
> +test:plan(2)
> +
> +jit.opt.start('hotloop=1', 'hotexit=1')

As far as we use `table.new` there is no table resizing, so there are no
exits from traces to compile and 'hotexit=1' can be removed.

> +
> +local function check(routine)
> +  jit.off()
> +  jit.flush()
> +  local interp_res = routine()
> +  jit.on()
> +  local jit_res = routine()
> +
> +  for i = 1, #interp_res do
> +    if interp_res[i] ~= jit_res[i] then
> +      return false
> +    end
> +  end
> +
> +  return true
> +end
> +
> +test:ok(

Minor: I suggest the following indentation to make this chunk more
readable:
| 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')

Feel free to ignore.

> +  check(
> +    function()
> +      local res = require('table.new')(3, 0)

Please, add a comment why do we use `table.new` here.

> +      for _ = 1, 3 do
> +        local zero = 0
> +        zero = -zero
> +        table.insert(res, tostring(zero))

Minor: we need `tostring()` routine to get different strings for -0 and
0, as far as 0 == -0 as numbers. Please, add the corresponding comment.

> +      end
> +      return res
> +    end
> +  ),
> +  'incorrect recording for zero'
> +)
> +

Ditto, about indentation.
Feel free to ignore.

> +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'
> +)
> +
> +os.exit(test:check() and 0 or 1)
> -- 
> 2.32.1 (Apple Git-133)
> 

-- 
Best regards,
Sergey Kaplun

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

[sergos via Tarantool-patches]

Hi!

Thanks for the patch!
I have two questions below.

Sergos

> On 20 Sep 2022, at 14:16, Maksim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org> wrote:
> 
> From: Mike Pall <mike>
> 
> (cherry picked from commit 1e6e8aaa20626ac94cf907c69b0452f76e9f5fa5)
> 
> 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.
 
> However, in DUALNUM mode with using of TValues containing integers the
> -0 value is represented in the same way as the 0 value and there is no
> difference between them.
> 
> 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?

> 
> Maxim Kokryashkin:
> * added the description and the test for the problem
> 
> Resolves tarantool/tarantool#6976
> Part of tarantool/tarantool#7230
> ---
> 
> Changes in v2:
> - Fixed test and commit message as per review by Sergey
> 
> Branch and PR are updated.
> 
> src/lj_opt_narrow.c                           |  9 +++-
> .../gh-6976-narrowing-of-unary-minus.test.lua | 51 +++++++++++++++++++
> 2 files changed, 58 insertions(+), 2 deletions(-)
> create mode 100644 test/tarantool-tests/gh-6976-narrowing-of-unary-minus.test.lua
> 
> diff --git a/src/lj_opt_narrow.c b/src/lj_opt_narrow.c
> index cd96ca4b..bb61f97b 100644
> --- a/src/lj_opt_narrow.c
> +++ b/src/lj_opt_narrow.c
> @@ -551,8 +551,13 @@ TRef lj_opt_narrow_unm(jit_State *J, TRef rc, TValue *vc)
> {
>   rc = conv_str_tonum(J, rc, vc);
>   if (tref_isinteger(rc)) {
> -    if ((uint32_t)numberVint(vc) != 0x80000000u)
> -      return emitir(IRTGI(IR_SUBOV), lj_ir_kint(J, 0), rc);
> +    uint32_t k = (uint32_t)numberVint(vc);
> +    if ((LJ_DUALNUM || k != 0) && k != 0x80000000u) {
> +      TRef zero = lj_ir_kint(J, 0);
> +      if (!LJ_DUALNUM)
> +	emitir(IRTGI(IR_NE), rc, zero);
> +      return emitir(IRTGI(IR_SUBOV), zero, rc);
> +    }
>     rc = emitir(IRTN(IR_CONV), rc, IRCONV_NUM_INT);
>   }
>   return emitir(IRTN(IR_NEG), rc, lj_ir_ksimd(J, LJ_KSIMD_NEG));
> diff --git a/test/tarantool-tests/gh-6976-narrowing-of-unary-minus.test.lua b/test/tarantool-tests/gh-6976-narrowing-of-unary-minus.test.lua
> new file mode 100644
> index 00000000..18778a55
> --- /dev/null
> +++ b/test/tarantool-tests/gh-6976-narrowing-of-unary-minus.test.lua
> @@ -0,0 +1,51 @@
> +local tap = require('tap')
> +local test = tap.test('gh-6976-narrowing-of-unary-minus')
> +test:plan(2)
> +
> +jit.opt.start('hotloop=1', 'hotexit=1')
> +
> +local function check(routine)
> +  jit.off()
> +  jit.flush()
> +  local interp_res = routine()
> +  jit.on()
> +  local jit_res = routine()
> +
> +  for i = 1, #interp_res do
> +    if interp_res[i] ~= jit_res[i] then
> +      return false
> +    end
> +  end
> +
> +  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'
> +)
> +
> +os.exit(test:check() and 0 or 1)
> -- 
> 2.32.1 (Apple Git-133)
> 

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

[Maxim Kokryashkin via Tarantool-patches]

[-- 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 --]

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

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!

Thanks for the fixes!
LGTM, just a single nit below.

On 29.09.22, Maxim Kokryashkin via Tarantool-patches wrote:
> 
> Hi, Sergos!
> Thanks for the questions!
> Please consider my answers amd changes below.

<snipped>

> 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

<snipped>

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

Typo: s/,/, /

>  

<snipped>

> Best regards,
> Maxim Kokryashkin
>  

-- 
Best regards,
Sergey Kaplun

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

[Maxim Kokryashkin via Tarantool-patches]

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


Hi, Sergey!
I’ve fixed the nit you mentioned, the branch is force pushed.
 
--
Best regards,
Maxim Kokryashkin
 
  
>Четверг, 29 сентября 2022, 16:11 +03:00 от Sergey Kaplun <skaplun@tarantool.org>:
> 
>Hi, Maxim!
>
>Thanks for the fixes!
>LGTM, just a single nit below.
>
>On 29.09.22, Maxim Kokryashkin via Tarantool-patches wrote:
>>
>> Hi, Sergos!
>> Thanks for the questions!
>> Please consider my answers amd changes below.
>
><snipped>
>
>> 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
>
><snipped>
>
>> +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')
>
>Typo: s/,/, /
>
>>  
>
><snipped>
>
>> Best regards,
>> Maxim Kokryashkin
>>  
>
>--
>Best regards,
>Sergey Kaplun
 

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

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

[sergos via Tarantool-patches]

[-- 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 --]

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

[Igor Munkin via Tarantool-patches]

Max,

I've checked the patches into all long-term branches in tarantool/luajit
and bumped a new version in master, 2.10 and 1.10.

On 09.09.22, Maksim Kokryashkin wrote:
> From: Mike Pall <mike>
> 
> With DUALNUM mode disabled, unary minus narrowing can produce
> inconsistent results on zeros, returning both -0 and 0 on the
> same chunk of code.
> 
> This patch fixes the inconsistency by adding checks for zeros.
> 
> Part of tarantool/tarantool#7230
> Resolves tarantool/tarantool#6976
> ---
> Issue: https://github.com/tarantool/tarantool/issues/6976
> Branch: https://github.com/tarantool/luajit/tree/fckxorg/gh-6976-narrowing-of-unary-minus
> PR: https://github.com/tarantool/tarantool/pull/7662
> 
>  src/lj_opt_narrow.c                           |  9 +++--
>  .../gh-6976-narrowing-of-unary-minus.test.lua | 34 +++++++++++++++++++
>  2 files changed, 41 insertions(+), 2 deletions(-)
>  create mode 100644 test/tarantool-tests/gh-6976-narrowing-of-unary-minus.test.lua
> 

<snipped>

> -- 
> 2.32.1 (Apple Git-133)
> 

-- 
Best regards,
IM