Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH luajit] Maintain chain invariant in DCE.
@ 2024-05-17 13:29 Sergey Kaplun via Tarantool-patches
  2024-05-26  9:36 ` Maxim Kokryashkin via Tarantool-patches
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-05-17 13:29 UTC (permalink / raw)
  To: Maxim Kokryashkin, Sergey Bronnikov; +Cc: tarantool-patches

From: Mike Pall <mike>

Thanks to Peter Cawley.

(cherry picked from commit f72c19e482b6f918b7cf42b0436e2b117d160a29)

Instructions with strong guards that are sometimes emitted with a guard
and sometimes emitted without a guard (like HREFK, CONV, or SLOAD) may
be eliminated from the IR chain and replaced with the NOP IR. If the
next IR of the same kind on the trace is not eliminated, it may
reference the IR NOP instead of an instruction of the same type. This
may lead to the corresponding assertion failure in the `rec_check_ir()`.

This patch unconditionally links the IRs during chain maintenance in
DCE.

Sergey Kaplun:
* added the description and the test for the problem

Part of tarantool/tarantool#9924
---

Be aware that the reproducer from the ticket does not lead to the
assertion failure (this is why it is omitted in the test). I suppose it
just illustrates the situation when the IR is left off the chain.
Although the reproducer is clumsy, I can't simplify it or make it less
tricky. Please, ideas are welcome :).

Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1094-ir-chain-dce
Related Issues:
* https://github.com/tarantool/tarantool/issues/9924
* https://github.com/LuaJIT/LuaJIT/issues/1094

 src/lj_opt_dce.c                              |  2 +-
 .../lj-1094-ir-chain-dce.test.lua             | 51 +++++++++++++++++++
 2 files changed, 52 insertions(+), 1 deletion(-)
 create mode 100644 test/tarantool-tests/lj-1094-ir-chain-dce.test.lua

diff --git a/src/lj_opt_dce.c b/src/lj_opt_dce.c
index 2417f324..6948179c 100644
--- a/src/lj_opt_dce.c
+++ b/src/lj_opt_dce.c
@@ -44,7 +44,6 @@ static void dce_propagate(jit_State *J)
     IRIns *ir = IR(ins);
     if (irt_ismarked(ir->t)) {
       irt_clearmark(ir->t);
-      pchain[ir->o] = &ir->prev;
     } else if (!ir_sideeff(ir)) {
       *pchain[ir->o] = ir->prev;  /* Reroute original instruction chain. */
       ir->t.irt = IRT_NIL;
@@ -53,6 +52,7 @@ static void dce_propagate(jit_State *J)
       ir->prev = 0;
       continue;
     }
+    pchain[ir->o] = &ir->prev;
     if (ir->op1 >= REF_FIRST) irt_setmark(IR(ir->op1)->t);
     if (ir->op2 >= REF_FIRST) irt_setmark(IR(ir->op2)->t);
   }
diff --git a/test/tarantool-tests/lj-1094-ir-chain-dce.test.lua b/test/tarantool-tests/lj-1094-ir-chain-dce.test.lua
new file mode 100644
index 00000000..3faae7d4
--- /dev/null
+++ b/test/tarantool-tests/lj-1094-ir-chain-dce.test.lua
@@ -0,0 +1,51 @@
+local tap = require('tap')
+
+-- Test file to demonstrate LuaJIT's incorrect maintenance of the
+-- IR chain during DCE.
+-- See also: https://github.com/LuaJIT/LuaJIT/issues/1094.
+
+local test = tap.test('lj-1094-ir-chain-dce'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+test:plan(1)
+
+jit.opt.start('hotloop=1')
+
+-- XXX: The code below is generated by the fuzzer (locally) and
+-- simplified as much as possible.
+
+-- Simple noop function.
+local __newindex = function() end
+debug.setmetatable(0, {
+    __newindex = __newindex,
+})
+
+local counter = 0
+-- luacheck: no unused
+local tab = {}
+while true do
+  -- The loop is still not recorded because the guard always
+  -- fails.
+  -- So, just try to compile it and check that there is no
+  -- assertion failure.
+  if counter > 2 then break end
+  counter = counter + 1
+  -- The pattern `-#{}` allows us to get CONV IRs. The first
+  -- appearance of this IR (in the `(-#{}).key`) is considered
+  -- unused by the compiler due to the corresponding "noop"
+  -- `__newindex` function.
+  -- The second usage of conversion (`tab[-#{}]`) is guarded (to
+  -- the int type), so it can't be eliminated.
+  -- As a result, the 0048 CONV references the 0039 NOP IR after
+  -- DCE in the IR chain.
+  -- XXX: TDUP prevents the corresponding second usage from being
+  -- eliminated since the table insert semantics may change.
+  -- XXX: Use some numbers to simplify reading the `jit.dump`
+  -- output.
+  tab, tab[-#{}], (-#{}).key = {tdup = 'tdup'}, 1, 2
+end
+
+test:ok(true, 'no assertion failure')
+
+test:done(true)
-- 
2.45.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] Maintain chain invariant in DCE.
  2024-05-17 13:29 [Tarantool-patches] [PATCH luajit] Maintain chain invariant in DCE Sergey Kaplun via Tarantool-patches
@ 2024-05-26  9:36 ` Maxim Kokryashkin via Tarantool-patches
  2024-05-27  7:25   ` Sergey Kaplun via Tarantool-patches
  2024-06-14 13:47 ` Sergey Bronnikov via Tarantool-patches
  2024-07-09  8:04 ` Sergey Kaplun via Tarantool-patches
  2 siblings, 1 reply; 8+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2024-05-26  9:36 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: tarantool-patches

Hi, Sergey!
Thanks for the patch!
Please consider my comments below.

On Fri, May 17, 2024 at 04:29:18PM UTC, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Thanks to Peter Cawley.
>
> (cherry picked from commit f72c19e482b6f918b7cf42b0436e2b117d160a29)
>
> Instructions with strong guards that are sometimes emitted with a guard
> and sometimes emitted without a guard (like HREFK, CONV, or SLOAD) may
> be eliminated from the IR chain and replaced with the NOP IR. If the
> next IR of the same kind on the trace is not eliminated, it may
> reference the IR NOP instead of an instruction of the same type. This
> may lead to the corresponding assertion failure in the `rec_check_ir()`.
>
> This patch unconditionally links the IRs during chain maintenance in
> DCE.
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#9924
> ---
>
> Be aware that the reproducer from the ticket does not lead to the
> assertion failure (this is why it is omitted in the test). I suppose it
> just illustrates the situation when the IR is left off the chain.
> Although the reproducer is clumsy, I can't simplify it or make it less
> tricky. Please, ideas are welcome :).

The test doesn't reproduce before the patch on ARM64. Tested on M1 with
the following flags:
| -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON \
| -DLUAJIT_ENABLE_CHECKHOOK=ON`

>
> Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1094-ir-chain-dce
> Related Issues:
> * https://github.com/tarantool/tarantool/issues/9924
> * https://github.com/LuaJIT/LuaJIT/issues/1094
>
>  src/lj_opt_dce.c                              |  2 +-
>  .../lj-1094-ir-chain-dce.test.lua             | 51 +++++++++++++++++++
>  2 files changed, 52 insertions(+), 1 deletion(-)
>  create mode 100644 test/tarantool-tests/lj-1094-ir-chain-dce.test.lua
>
> diff --git a/src/lj_opt_dce.c b/src/lj_opt_dce.c
> index 2417f324..6948179c 100644
> --- a/src/lj_opt_dce.c
> +++ b/src/lj_opt_dce.c
> @@ -44,7 +44,6 @@ static void dce_propagate(jit_State *J)
>      IRIns *ir = IR(ins);
>      if (irt_ismarked(ir->t)) {
>        irt_clearmark(ir->t);
> -      pchain[ir->o] = &ir->prev;
>      } else if (!ir_sideeff(ir)) {
>        *pchain[ir->o] = ir->prev;  /* Reroute original instruction chain. */
>        ir->t.irt = IRT_NIL;
> @@ -53,6 +52,7 @@ static void dce_propagate(jit_State *J)
>        ir->prev = 0;
>        continue;
>      }
> +    pchain[ir->o] = &ir->prev;
>      if (ir->op1 >= REF_FIRST) irt_setmark(IR(ir->op1)->t);
>      if (ir->op2 >= REF_FIRST) irt_setmark(IR(ir->op2)->t);
>    }
> diff --git a/test/tarantool-tests/lj-1094-ir-chain-dce.test.lua b/test/tarantool-tests/lj-1094-ir-chain-dce.test.lua
> new file mode 100644
> index 00000000..3faae7d4
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1094-ir-chain-dce.test.lua
<snipped>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] Maintain chain invariant in DCE.
  2024-05-26  9:36 ` Maxim Kokryashkin via Tarantool-patches
@ 2024-05-27  7:25   ` Sergey Kaplun via Tarantool-patches
  2024-05-27  8:29     ` Maxim Kokryashkin via Tarantool-patches
  0 siblings, 1 reply; 8+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-05-27  7:25 UTC (permalink / raw)
  To: Maxim Kokryashkin; +Cc: tarantool-patches

Hi, Maxim!
Thanks for the review!

On 26.05.24, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the patch!
> Please consider my comments below.
> 
> On Fri, May 17, 2024 at 04:29:18PM UTC, Sergey Kaplun wrote:
> > From: Mike Pall <mike>
> >
> > Thanks to Peter Cawley.
> >
> > (cherry picked from commit f72c19e482b6f918b7cf42b0436e2b117d160a29)
> >
> > Instructions with strong guards that are sometimes emitted with a guard
> > and sometimes emitted without a guard (like HREFK, CONV, or SLOAD) may
> > be eliminated from the IR chain and replaced with the NOP IR. If the
> > next IR of the same kind on the trace is not eliminated, it may
> > reference the IR NOP instead of an instruction of the same type. This
> > may lead to the corresponding assertion failure in the `rec_check_ir()`.
> >
> > This patch unconditionally links the IRs during chain maintenance in
> > DCE.
> >
> > Sergey Kaplun:
> > * added the description and the test for the problem
> >
> > Part of tarantool/tarantool#9924
> > ---
> >
> > Be aware that the reproducer from the ticket does not lead to the
> > assertion failure (this is why it is omitted in the test). I suppose it
> > just illustrates the situation when the IR is left off the chain.
> > Although the reproducer is clumsy, I can't simplify it or make it less
> > tricky. Please, ideas are welcome :).
> 
> The test doesn't reproduce before the patch on ARM64. Tested on M1 with
> the following flags:
> | -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON \
> | -DLUAJIT_ENABLE_CHECKHOOK=ON`

I suppose this is due to the enabled by default DUALNUM mode, so there
are no needed CONV IRs, so there are no IR chains built incorrectly.

<snipped>

-- 
Best regards,
Sergey Kaplun

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] Maintain chain invariant in DCE.
  2024-05-27  7:25   ` Sergey Kaplun via Tarantool-patches
@ 2024-05-27  8:29     ` Maxim Kokryashkin via Tarantool-patches
  0 siblings, 0 replies; 8+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2024-05-27  8:29 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: tarantool-patches

Hi, Sergey!
Thanks for the answer!
On Mon, May 27, 2024 at 10:25:27AM UTC, Sergey Kaplun wrote:
> Hi, Maxim!
> Thanks for the review!
>
> On 26.05.24, Maxim Kokryashkin wrote:
> > Hi, Sergey!
> > Thanks for the patch!
> > Please consider my comments below.
> >
> > On Fri, May 17, 2024 at 04:29:18PM UTC, Sergey Kaplun wrote:
> > > From: Mike Pall <mike>
> > >
> > > Thanks to Peter Cawley.
> > >
> > > (cherry picked from commit f72c19e482b6f918b7cf42b0436e2b117d160a29)
> > >
> > > Instructions with strong guards that are sometimes emitted with a guard
> > > and sometimes emitted without a guard (like HREFK, CONV, or SLOAD) may
> > > be eliminated from the IR chain and replaced with the NOP IR. If the
> > > next IR of the same kind on the trace is not eliminated, it may
> > > reference the IR NOP instead of an instruction of the same type. This
> > > may lead to the corresponding assertion failure in the `rec_check_ir()`.
> > >
> > > This patch unconditionally links the IRs during chain maintenance in
> > > DCE.
> > >
> > > Sergey Kaplun:
> > > * added the description and the test for the problem
> > >
> > > Part of tarantool/tarantool#9924
> > > ---
> > >
> > > Be aware that the reproducer from the ticket does not lead to the
> > > assertion failure (this is why it is omitted in the test). I suppose it
> > > just illustrates the situation when the IR is left off the chain.
> > > Although the reproducer is clumsy, I can't simplify it or make it less
> > > tricky. Please, ideas are welcome :).
> >
> > The test doesn't reproduce before the patch on ARM64. Tested on M1 with
> > the following flags:
> > | -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON \
> > | -DLUAJIT_ENABLE_CHECKHOOK=ON`
>
> I suppose this is due to the enabled by default DUALNUM mode, so there
> are no needed CONV IRs, so there are no IR chains built incorrectly.

Thanks for the clarification! I have no further comments, so the patch LGTM.
>
> <snipped>
>
> --
> Best regards,
> Sergey Kaplun

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] Maintain chain invariant in DCE.
  2024-05-17 13:29 [Tarantool-patches] [PATCH luajit] Maintain chain invariant in DCE Sergey Kaplun via Tarantool-patches
  2024-05-26  9:36 ` Maxim Kokryashkin via Tarantool-patches
@ 2024-06-14 13:47 ` Sergey Bronnikov via Tarantool-patches
  2024-06-16 19:35   ` Sergey Kaplun via Tarantool-patches
  2024-07-09  8:04 ` Sergey Kaplun via Tarantool-patches
  2 siblings, 1 reply; 8+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2024-06-14 13:47 UTC (permalink / raw)
  To: Sergey Kaplun, Maxim Kokryashkin; +Cc: tarantool-patches

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

Hi, Sergey

thanks for the patch! see my comment below

On 17.05.2024 16:29, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Thanks to Peter Cawley.
>
> (cherry picked from commit f72c19e482b6f918b7cf42b0436e2b117d160a29)
>
> Instructions with strong guards that are sometimes emitted with a guard
> and sometimes emitted without a guard (like HREFK, CONV, or SLOAD) may
> be eliminated from the IR chain and replaced with the NOP IR. If the
> next IR of the same kind on the trace is not eliminated, it may
> reference the IR NOP instead of an instruction of the same type. This
> may lead to the corresponding assertion failure in the `rec_check_ir()`.
>
> This patch unconditionally links the IRs during chain maintenance in
> DCE.
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#9924
> ---
>
> Be aware that the reproducer from the ticket does not lead to the
> assertion failure (this is why it is omitted in the test). I suppose it
> just illustrates the situation when the IR is left off the chain.
> Although the reproducer is clumsy, I can't simplify it or make it less
> tricky. Please, ideas are welcome :).
>
> Branch:https://github.com/tarantool/luajit/tree/skaplun/lj-1094-ir-chain-dce
> Related Issues:
> *https://github.com/tarantool/tarantool/issues/9924
> *https://github.com/LuaJIT/LuaJIT/issues/1094
>
>   src/lj_opt_dce.c                              |  2 +-
>   .../lj-1094-ir-chain-dce.test.lua             | 51 +++++++++++++++++++
>   2 files changed, 52 insertions(+), 1 deletion(-)
>   create mode 100644 test/tarantool-tests/lj-1094-ir-chain-dce.test.lua
>
> diff --git a/src/lj_opt_dce.c b/src/lj_opt_dce.c
> index 2417f324..6948179c 100644
> --- a/src/lj_opt_dce.c
> +++ b/src/lj_opt_dce.c
> @@ -44,7 +44,6 @@ static void dce_propagate(jit_State *J)
>       IRIns *ir = IR(ins);
>       if (irt_ismarked(ir->t)) {
>         irt_clearmark(ir->t);
> -      pchain[ir->o] = &ir->prev;
>       } else if (!ir_sideeff(ir)) {
>         *pchain[ir->o] = ir->prev;  /* Reroute original instruction chain. */
>         ir->t.irt = IRT_NIL;
> @@ -53,6 +52,7 @@ static void dce_propagate(jit_State *J)
>         ir->prev = 0;
>         continue;
>       }
> +    pchain[ir->o] = &ir->prev;
>       if (ir->op1 >= REF_FIRST) irt_setmark(IR(ir->op1)->t);
>       if (ir->op2 >= REF_FIRST) irt_setmark(IR(ir->op2)->t);
>     }
> diff --git a/test/tarantool-tests/lj-1094-ir-chain-dce.test.lua b/test/tarantool-tests/lj-1094-ir-chain-dce.test.lua
> new file mode 100644
> index 00000000..3faae7d4
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1094-ir-chain-dce.test.lua
> @@ -0,0 +1,51 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate LuaJIT's incorrect maintenance of the
> +-- IR chain during DCE.
> +-- See also:https://github.com/LuaJIT/LuaJIT/issues/1094.
> +
> +local test = tap.test('lj-1094-ir-chain-dce'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +test:plan(1)
> +
> +jit.opt.start('hotloop=1')
> +
> +-- XXX: The code below is generated by the fuzzer (locally) and
> +-- simplified as much as possible.
> +
> +-- Simple noop function.
> +local __newindex = function() end
> +debug.setmetatable(0, {
> +    __newindex = __newindex,
> +})
> +
> +local counter = 0
> +-- luacheck: no unused
> +local tab = {}
> +while true do
> +  -- The loop is still not recorded because the guard always
> +  -- fails.
> +  -- So, just try to compile it and check that there is no
> +  -- assertion failure.
> +  if counter > 2 then break end
> +  counter = counter + 1
> +  -- The pattern `-#{}` allows us to get CONV IRs. The first
> +  -- appearance of this IR (in the `(-#{}).key`) is considered
> +  -- unused by the compiler due to the corresponding "noop"
> +  -- `__newindex` function.
> +  -- The second usage of conversion (`tab[-#{}]`) is guarded (to
> +  -- the int type), so it can't be eliminated.
> +  -- As a result, the 0048 CONV references the 0039 NOP IR after
> +  -- DCE in the IR chain.
I suppose an IR output would be helpful here. What do you think?
> +  -- XXX: TDUP prevents the corresponding second usage from being
> +  -- eliminated since the table insert semantics may change.
> +  -- XXX: Use some numbers to simplify reading the `jit.dump`
> +  -- output.
> +  tab, tab[-#{}], (-#{}).key = {tdup = 'tdup'}, 1, 2
> +end
> +
> +test:ok(true, 'no assertion failure')
> +
> +test:done(true)

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] Maintain chain invariant in DCE.
  2024-06-14 13:47 ` Sergey Bronnikov via Tarantool-patches
@ 2024-06-16 19:35   ` Sergey Kaplun via Tarantool-patches
  2024-06-17  8:44     ` Sergey Bronnikov via Tarantool-patches
  0 siblings, 1 reply; 8+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-06-16 19:35 UTC (permalink / raw)
  To: Sergey Bronnikov; +Cc: tarantool-patches

Hi, Sergey!
Thanks for the review!
I've added comments and force-pushed the branch.

On 14.06.24, Sergey Bronnikov wrote:
> Hi, Sergey
> 
> thanks for the patch! see my comment below
> 
> On 17.05.2024 16:29, Sergey Kaplun wrote:
> > From: Mike Pall <mike>
> >
> > Thanks to Peter Cawley.
> >
> > (cherry picked from commit f72c19e482b6f918b7cf42b0436e2b117d160a29)
> >
> > Instructions with strong guards that are sometimes emitted with a guard
> > and sometimes emitted without a guard (like HREFK, CONV, or SLOAD) may
> > be eliminated from the IR chain and replaced with the NOP IR. If the
> > next IR of the same kind on the trace is not eliminated, it may
> > reference the IR NOP instead of an instruction of the same type. This
> > may lead to the corresponding assertion failure in the `rec_check_ir()`.
> >
> > This patch unconditionally links the IRs during chain maintenance in
> > DCE.
> >
> > Sergey Kaplun:
> > * added the description and the test for the problem
> >
> > Part of tarantool/tarantool#9924
> > ---
> >

<snipped>

> > +local counter = 0
> > +-- luacheck: no unused
> > +local tab = {}
> > +while true do
> > +  -- The loop is still not recorded because the guard always
> > +  -- fails.
> > +  -- So, just try to compile it and check that there is no
> > +  -- assertion failure.
> > +  if counter > 2 then break end
> > +  counter = counter + 1
> > +  -- The pattern `-#{}` allows us to get CONV IRs. The first
> > +  -- appearance of this IR (in the `(-#{}).key`) is considered
> > +  -- unused by the compiler due to the corresponding "noop"
> > +  -- `__newindex` function.
> > +  -- The second usage of conversion (`tab[-#{}]`) is guarded (to
> > +  -- the int type), so it can't be eliminated.
> > +  -- As a result, the 0048 CONV references the 0039 NOP IR after
> > +  -- DCE in the IR chain.
> I suppose an IR output would be helpful here. What do you think?

I've added the corresponding output:

| -- The IR itself looks like the following
| -- by the `jit.dump` (NOPs are not printed):
| -- 0036    num CONV   0035  num.int
| -- 0037    num NEG    0036  0007
| -- 0042 >  tab TDUP   {0x40154030}
| -- 0043    int FLOAD  0014  tab.hmask
| -- 0044 >  int EQ     0043  +1
| -- 0045    p32 FLOAD  0014  tab.node
| -- 0046 >  p32 HREFK  0045  "__newindex" @0
| -- 0047 >  fun HLOAD  0046
| -- 0048 >  fun EQ     0047  "lj-1094-ir-chain-dce.test.lua":20
| -- 0049 >  int CONV   0037  int.num index

For some reason I reproduced this bug only for GC64 mode, so I've
adjusted a IRs numbers and added the corresponding comment.


> > +  -- XXX: TDUP prevents the corresponding second usage from being
> > +  -- eliminated since the table insert semantics may change.
> > +  -- XXX: Use some numbers to simplify reading the `jit.dump`
> > +  -- output.
> > +  tab, tab[-#{}], (-#{}).key = {tdup = 'tdup'}, 1, 2
> > +end
> > +
> > +test:ok(true, 'no assertion failure')
> > +
> > +test:done(true)

-- 
Best regards,
Sergey Kaplun

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] Maintain chain invariant in DCE.
  2024-06-16 19:35   ` Sergey Kaplun via Tarantool-patches
@ 2024-06-17  8:44     ` Sergey Bronnikov via Tarantool-patches
  0 siblings, 0 replies; 8+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2024-06-17  8:44 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: tarantool-patches

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

Hi, Sergey


thanks for update, LGTM.

On 16.06.2024 22:35, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the review!
> I've added comments and force-pushed the branch.
>
> On 14.06.24, Sergey Bronnikov wrote:
>> Hi, Sergey
>>
>> thanks for the patch! see my comment below
>>
>> On 17.05.2024 16:29, Sergey Kaplun wrote:
>>> From: Mike Pall <mike>
>>>
>>> Thanks to Peter Cawley.
>>>
>>> (cherry picked from commit f72c19e482b6f918b7cf42b0436e2b117d160a29)
>>>
>>> Instructions with strong guards that are sometimes emitted with a guard
>>> and sometimes emitted without a guard (like HREFK, CONV, or SLOAD) may
>>> be eliminated from the IR chain and replaced with the NOP IR. If the
>>> next IR of the same kind on the trace is not eliminated, it may
>>> reference the IR NOP instead of an instruction of the same type. This
>>> may lead to the corresponding assertion failure in the `rec_check_ir()`.
>>>
>>> This patch unconditionally links the IRs during chain maintenance in
>>> DCE.
>>>
>>> Sergey Kaplun:
>>> * added the description and the test for the problem
>>>
>>> Part of tarantool/tarantool#9924
>>> ---
>>>
> <snipped>
>
>>> +local counter = 0
>>> +-- luacheck: no unused
>>> +local tab = {}
>>> +while true do
>>> +  -- The loop is still not recorded because the guard always
>>> +  -- fails.
>>> +  -- So, just try to compile it and check that there is no
>>> +  -- assertion failure.
>>> +  if counter > 2 then break end
>>> +  counter = counter + 1
>>> +  -- The pattern `-#{}` allows us to get CONV IRs. The first
>>> +  -- appearance of this IR (in the `(-#{}).key`) is considered
>>> +  -- unused by the compiler due to the corresponding "noop"
>>> +  -- `__newindex` function.
>>> +  -- The second usage of conversion (`tab[-#{}]`) is guarded (to
>>> +  -- the int type), so it can't be eliminated.
>>> +  -- As a result, the 0048 CONV references the 0039 NOP IR after
>>> +  -- DCE in the IR chain.
>> I suppose an IR output would be helpful here. What do you think?
> I've added the corresponding output:
>
> | -- The IR itself looks like the following
> | -- by the `jit.dump` (NOPs are not printed):
> | -- 0036    num CONV   0035  num.int
> | -- 0037    num NEG    0036  0007
> | -- 0042 >  tab TDUP   {0x40154030}
> | -- 0043    int FLOAD  0014  tab.hmask
> | -- 0044 >  int EQ     0043  +1
> | -- 0045    p32 FLOAD  0014  tab.node
> | -- 0046 >  p32 HREFK  0045  "__newindex" @0
> | -- 0047 >  fun HLOAD  0046
> | -- 0048 >  fun EQ     0047  "lj-1094-ir-chain-dce.test.lua":20
> | -- 0049 >  int CONV   0037  int.num index
>
> For some reason I reproduced this bug only for GC64 mode, so I've
> adjusted a IRs numbers and added the corresponding comment.
>
>
>>> +  -- XXX: TDUP prevents the corresponding second usage from being
>>> +  -- eliminated since the table insert semantics may change.
>>> +  -- XXX: Use some numbers to simplify reading the `jit.dump`
>>> +  -- output.
>>> +  tab, tab[-#{}], (-#{}).key = {tdup = 'tdup'}, 1, 2
>>> +end
>>> +
>>> +test:ok(true, 'no assertion failure')
>>> +
>>> +test:done(true)

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] Maintain chain invariant in DCE.
  2024-05-17 13:29 [Tarantool-patches] [PATCH luajit] Maintain chain invariant in DCE Sergey Kaplun via Tarantool-patches
  2024-05-26  9:36 ` Maxim Kokryashkin via Tarantool-patches
  2024-06-14 13:47 ` Sergey Bronnikov via Tarantool-patches
@ 2024-07-09  8:04 ` Sergey Kaplun via Tarantool-patches
  2 siblings, 0 replies; 8+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-07-09  8:04 UTC (permalink / raw)
  To: Maxim Kokryashkin, Sergey Bronnikov; +Cc: tarantool-patches

I've checked the patchset into all long-term branches in
tarantool/luajit and bumped a new version in master [1], release/3.1 [2]
and release/2.11 [3].

[1]: https://github.com/tarantool/tarantool/pull/10200
[2]: https://github.com/tarantool/tarantool/pull/10201
[3]: https://github.com/tarantool/tarantool/pull/10202

-- 
Best regards,
Sergey Kaplun

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2024-07-09  8:04 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-05-17 13:29 [Tarantool-patches] [PATCH luajit] Maintain chain invariant in DCE Sergey Kaplun via Tarantool-patches
2024-05-26  9:36 ` Maxim Kokryashkin via Tarantool-patches
2024-05-27  7:25   ` Sergey Kaplun via Tarantool-patches
2024-05-27  8:29     ` Maxim Kokryashkin via Tarantool-patches
2024-06-14 13:47 ` Sergey Bronnikov via Tarantool-patches
2024-06-16 19:35   ` Sergey Kaplun via Tarantool-patches
2024-06-17  8:44     ` Sergey Bronnikov via Tarantool-patches
2024-07-09  8:04 ` Sergey Kaplun via Tarantool-patches

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