[Tarantool-patches] [PATCH] fold: keep type of emitted CONV in sync with its mode

[Tarantool-patches] [PATCH] fold: keep type of emitted CONV in sync with its mode

[Igor Munkin]

From: Vyacheslav Egorov <vegorov@google.com>

When emitting CONV make sure that its type matches its destination IRType.

This keeps IR fully internally consistent with respect to types - i.e. if
we push narrowing CONV Dt.St upwards through an arithmetic operation of type
St we end up with arithmetic operation of type Dt and two convertions
CONV Dt.St which narrow the operands.

Igor Munkin:
* added a test for the problem
* backported the original patch to tarantool/luajit repo
* stripped some words not related to the patch itself

Fixes LuaJIT/LuaJIT#524

Signed-off-by: Vyacheslav Egorov <vegorov@google.com>
Signed-off-by: Igor Munkin <imun@tarantool.org>
---

This is a backport for https://github.com/moonjit/moonjit/commit/4dba12d

Branch: https://github.com/tarantool/luajit/tree/imun/backport-fix-LuaJIT-524
Issue: https://github.com/LuaJIT/LuaJIT/issues/524

 src/lj_opt_fold.c                 |  4 ++--
 test/fold_bug_LuaJIT_524.test.lua | 24 ++++++++++++++++++++++++
 2 files changed, 26 insertions(+), 2 deletions(-)
 create mode 100755 test/fold_bug_LuaJIT_524.test.lua

diff --git a/src/lj_opt_fold.c b/src/lj_opt_fold.c
index acbf36a..3c50806 100644
--- a/src/lj_opt_fold.c
+++ b/src/lj_opt_fold.c
@@ -1261,8 +1261,8 @@ LJFOLDF(simplify_conv_narrow)
   IRType t = irt_type(fins->t);
   IRRef op1 = fleft->op1, op2 = fleft->op2, mode = fins->op2;
   PHIBARRIER(fleft);
-  op1 = emitir(IRTI(IR_CONV), op1, mode);
-  op2 = emitir(IRTI(IR_CONV), op2, mode);
+  op1 = emitir(IRT(IR_CONV, t), op1, mode);
+  op2 = emitir(IRT(IR_CONV, t), op2, mode);
   fins->ot = IRT(op, t);
   fins->op1 = op1;
   fins->op2 = op2;
diff --git a/test/fold_bug_LuaJIT_524.test.lua b/test/fold_bug_LuaJIT_524.test.lua
new file mode 100755
index 0000000..b056684
--- /dev/null
+++ b/test/fold_bug_LuaJIT_524.test.lua
@@ -0,0 +1,24 @@
+#!/usr/bin/env tarantool
+
+local tap = require('tap')
+local ffi = require('ffi')
+
+local test = tap.test("LuaJIT 524")
+test:plan(1)
+
+-- Test file to demonstrate LuaJIT folding machinery incorrect behaviour,
+-- details:
+--     https://github.com/LuaJIT/LuaJIT/issues/524
+--     https://github.com/moonjit/moonjit/issues/37
+
+jit.opt.start(0, "fold", "cse", "fwd", "hotloop=1")
+
+local sq = ffi.cast("uint32_t", 42)
+
+for _ = 1, 3 do
+    sq = ffi.cast("uint32_t", sq * sq)
+end
+
+test:is(tonumber(sq), math.fmod(math.pow(42, 8), math.pow(2, 32)))
+
+test:check()
-- 
2.24.0

Re: [Tarantool-patches] [PATCH] fold: keep type of emitted CONV in sync with its mode

[Sergey Ostanevich]

Hi!

Thanks for the patch, I didn't find any similar issues to fix along with
it.

LGTM,
Sergos

On 02 Dec 10:03, Igor Munkin wrote:
> From: Vyacheslav Egorov <vegorov@google.com>
> 
> When emitting CONV make sure that its type matches its destination IRType.
> 
> This keeps IR fully internally consistent with respect to types - i.e. if
> we push narrowing CONV Dt.St upwards through an arithmetic operation of type
> St we end up with arithmetic operation of type Dt and two convertions
> CONV Dt.St which narrow the operands.
> 
> Igor Munkin:
> * added a test for the problem
> * backported the original patch to tarantool/luajit repo
> * stripped some words not related to the patch itself
> 
> Fixes LuaJIT/LuaJIT#524
> 
> Signed-off-by: Vyacheslav Egorov <vegorov@google.com>
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---
> 
> This is a backport for https://github.com/moonjit/moonjit/commit/4dba12d
> 
> Branch: https://github.com/tarantool/luajit/tree/imun/backport-fix-LuaJIT-524
> Issue: https://github.com/LuaJIT/LuaJIT/issues/524
> 
>  src/lj_opt_fold.c                 |  4 ++--
>  test/fold_bug_LuaJIT_524.test.lua | 24 ++++++++++++++++++++++++
>  2 files changed, 26 insertions(+), 2 deletions(-)
>  create mode 100755 test/fold_bug_LuaJIT_524.test.lua
> 
> diff --git a/src/lj_opt_fold.c b/src/lj_opt_fold.c
> index acbf36a..3c50806 100644
> --- a/src/lj_opt_fold.c
> +++ b/src/lj_opt_fold.c
> @@ -1261,8 +1261,8 @@ LJFOLDF(simplify_conv_narrow)
>    IRType t = irt_type(fins->t);
>    IRRef op1 = fleft->op1, op2 = fleft->op2, mode = fins->op2;
>    PHIBARRIER(fleft);
> -  op1 = emitir(IRTI(IR_CONV), op1, mode);
> -  op2 = emitir(IRTI(IR_CONV), op2, mode);
> +  op1 = emitir(IRT(IR_CONV, t), op1, mode);
> +  op2 = emitir(IRT(IR_CONV, t), op2, mode);
>    fins->ot = IRT(op, t);
>    fins->op1 = op1;
>    fins->op2 = op2;
> diff --git a/test/fold_bug_LuaJIT_524.test.lua b/test/fold_bug_LuaJIT_524.test.lua
> new file mode 100755
> index 0000000..b056684
> --- /dev/null
> +++ b/test/fold_bug_LuaJIT_524.test.lua
> @@ -0,0 +1,24 @@
> +#!/usr/bin/env tarantool
> +
> +local tap = require('tap')
> +local ffi = require('ffi')
> +
> +local test = tap.test("LuaJIT 524")
> +test:plan(1)
> +
> +-- Test file to demonstrate LuaJIT folding machinery incorrect behaviour,
> +-- details:
> +--     https://github.com/LuaJIT/LuaJIT/issues/524
> +--     https://github.com/moonjit/moonjit/issues/37
> +
> +jit.opt.start(0, "fold", "cse", "fwd", "hotloop=1")
> +
> +local sq = ffi.cast("uint32_t", 42)
> +
> +for _ = 1, 3 do
> +    sq = ffi.cast("uint32_t", sq * sq)
> +end
> +
> +test:is(tonumber(sq), math.fmod(math.pow(42, 8), math.pow(2, 32)))
> +
> +test:check()
> -- 
> 2.24.0
> 

Re: [Tarantool-patches] [PATCH] fold: keep type of emitted CONV in sync with its mode

[Igor Munkin]

Sergos,

Thanks for your review and the futher research!

Cced Kirill to proceed with the patch.

On 04.12.19, Sergey Ostanevich wrote:
> Hi!
> 
> Thanks for the patch, I didn't find any similar issues to fix along with
> it.
> 
> LGTM,
> Sergos
> 
> On 02 Dec 10:03, Igor Munkin wrote:
> > From: Vyacheslav Egorov <vegorov@google.com>
> > 
> > When emitting CONV make sure that its type matches its destination IRType.
> > 
> > This keeps IR fully internally consistent with respect to types - i.e. if
> > we push narrowing CONV Dt.St upwards through an arithmetic operation of type
> > St we end up with arithmetic operation of type Dt and two convertions
> > CONV Dt.St which narrow the operands.
> > 
> > Igor Munkin:
> > * added a test for the problem
> > * backported the original patch to tarantool/luajit repo
> > * stripped some words not related to the patch itself
> > 
> > Fixes LuaJIT/LuaJIT#524
> > 
> > Signed-off-by: Vyacheslav Egorov <vegorov@google.com>
> > Signed-off-by: Igor Munkin <imun@tarantool.org>
> > ---
> > 
> > This is a backport for https://github.com/moonjit/moonjit/commit/4dba12d
> > 
> > Branch: https://github.com/tarantool/luajit/tree/imun/backport-fix-LuaJIT-524
> > Issue: https://github.com/LuaJIT/LuaJIT/issues/524
> > 
> >  src/lj_opt_fold.c                 |  4 ++--
> >  test/fold_bug_LuaJIT_524.test.lua | 24 ++++++++++++++++++++++++
> >  2 files changed, 26 insertions(+), 2 deletions(-)
> >  create mode 100755 test/fold_bug_LuaJIT_524.test.lua
> > 
> > diff --git a/src/lj_opt_fold.c b/src/lj_opt_fold.c
> > index acbf36a..3c50806 100644
> > --- a/src/lj_opt_fold.c
> > +++ b/src/lj_opt_fold.c
> > @@ -1261,8 +1261,8 @@ LJFOLDF(simplify_conv_narrow)
> >    IRType t = irt_type(fins->t);
> >    IRRef op1 = fleft->op1, op2 = fleft->op2, mode = fins->op2;
> >    PHIBARRIER(fleft);
> > -  op1 = emitir(IRTI(IR_CONV), op1, mode);
> > -  op2 = emitir(IRTI(IR_CONV), op2, mode);
> > +  op1 = emitir(IRT(IR_CONV, t), op1, mode);
> > +  op2 = emitir(IRT(IR_CONV, t), op2, mode);
> >    fins->ot = IRT(op, t);
> >    fins->op1 = op1;
> >    fins->op2 = op2;
> > diff --git a/test/fold_bug_LuaJIT_524.test.lua b/test/fold_bug_LuaJIT_524.test.lua
> > new file mode 100755
> > index 0000000..b056684
> > --- /dev/null
> > +++ b/test/fold_bug_LuaJIT_524.test.lua
> > @@ -0,0 +1,24 @@
> > +#!/usr/bin/env tarantool
> > +
> > +local tap = require('tap')
> > +local ffi = require('ffi')
> > +
> > +local test = tap.test("LuaJIT 524")
> > +test:plan(1)
> > +
> > +-- Test file to demonstrate LuaJIT folding machinery incorrect behaviour,
> > +-- details:
> > +--     https://github.com/LuaJIT/LuaJIT/issues/524
> > +--     https://github.com/moonjit/moonjit/issues/37
> > +
> > +jit.opt.start(0, "fold", "cse", "fwd", "hotloop=1")
> > +
> > +local sq = ffi.cast("uint32_t", 42)
> > +
> > +for _ = 1, 3 do
> > +    sq = ffi.cast("uint32_t", sq * sq)
> > +end
> > +
> > +test:is(tonumber(sq), math.fmod(math.pow(42, 8), math.pow(2, 32)))
> > +
> > +test:check()
> > -- 
> > 2.24.0
> > 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH] fold: keep type of emitted CONV in sync with its mode

[Kirill Yukhin]

Hello,

On 02 дек 10:03, Igor Munkin wrote:
> From: Vyacheslav Egorov <vegorov@google.com>
> 
> When emitting CONV make sure that its type matches its destination IRType.
> 
> This keeps IR fully internally consistent with respect to types - i.e. if
> we push narrowing CONV Dt.St upwards through an arithmetic operation of type
> St we end up with arithmetic operation of type Dt and two convertions
> CONV Dt.St which narrow the operands.
> 
> Igor Munkin:
> * added a test for the problem
> * backported the original patch to tarantool/luajit repo
> * stripped some words not related to the patch itself
> 
> Fixes LuaJIT/LuaJIT#524
> 
> Signed-off-by: Vyacheslav Egorov <vegorov@google.com>
> Signed-off-by: Igor Munkin <imun@tarantool.org>

I've checked your patch into luajit repo and bumped new version
in Tarantool.

--
Regards, Kirill Yukhin