Tarantool development patches archive
 help / color / mirror / Atom feed
From: sergos via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Maksim Kokryashkin <max.kokryashkin@gmail.com>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit v2] Fix narrowing of unary minus.
Date: Mon, 26 Sep 2022 00:31:50 +0300	[thread overview]
Message-ID: <4DD728DB-B92B-4466-A709-E3FD08576876@tarantool.org> (raw)
In-Reply-To: <20220920111649.86190-1-max.kokryashkin@gmail.com>

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


  parent reply	other threads:[~2022-09-25 21:31 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-09 12:15 [Tarantool-patches] [PATCH luajit] " Maksim Kokryashkin via Tarantool-patches
2022-09-14 22:06 ` Sergey Kaplun via Tarantool-patches
2022-09-20 11:16   ` [Tarantool-patches] [PATCH luajit v2] " Maksim Kokryashkin via Tarantool-patches
2022-09-21  8:48     ` Sergey Kaplun via Tarantool-patches
2022-09-25 21:31     ` sergos via Tarantool-patches [this message]
2022-09-29  9:58       ` Maxim Kokryashkin via Tarantool-patches
2022-09-29 13:08         ` Sergey Kaplun via Tarantool-patches
2022-10-03  9:57           ` Maxim Kokryashkin via Tarantool-patches
2022-11-30 10:40         ` sergos via Tarantool-patches
2022-12-12  9:42 ` [Tarantool-patches] [PATCH luajit] " Igor Munkin via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4DD728DB-B92B-4466-A709-E3FD08576876@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=max.kokryashkin@gmail.com \
    --cc=sergos@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH luajit v2] Fix narrowing of unary minus.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox