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

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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>