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