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

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

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