Re: [Tarantool-patches] [PATCH luajit] Maintain chain invariant in DCE.

[Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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