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