From: Vitaliia Ioffe via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: "Igor Munkin" <imun@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit] Fix bytecode register allocation for comparisons.
Date: Tue, 17 Aug 2021 10:36:11 +0300 [thread overview]
Message-ID: <1629185771.534192263@f337.i.mail.ru> (raw)
In-Reply-To: <20210816162715.GA5743@tarantool.org>
[-- Attachment #1: Type: text/plain, Size: 4931 bytes --]
QA LGTM
--
Vitaliia Ioffe
>Понедельник, 16 августа 2021, 19:52 +03:00 от Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>:
>
>Sergey,
>
>Thanks for the fixes! LGTM now.
>
>On 16.08.21, Sergey Kaplun wrote:
>> Igor,
>>
>> Thanks for the review!
>>
>> See the new comment message above:
>> ===================================================================
>> Fix bytecode register allocation for comparisons.
>>
>> (cherry picked from commit 2f3f07882fb4ad9c64967d7088461b1ca0a25d3a)
>>
>> When LuaJIT is built with LJ_FR2 (e.g. with GC64 mode enabled),
>> information about frame takes two slots -- the first takes the TValue
>> with the function to be called, the second takes the framelink. The JIT
>> recording machinery does pretty the same -- the function IR_KGC is
>> loaded in the first slot, and the second is set to TREF_FRAME value.
>> This value should be rewritten after return from a callee. This slot is
>> cleared either by return values or manually (set to zero), when there
>> are no values to return. The latter case is done by the next bytecode
>> with RA dst mode. This obliges that the destination of RA takes the next
>> slot after TREF_FRAME. Hence, this an earlier instruction must use the
>> smallest possible destination register (see `lj_record_ins()` for the
>> details).
>>
>> Bytecode emitter swaps operands for ISGT and ISGE comparisons. As a
>> result, the aforementioned rule for registers allocations may be
>> violated. When it happens for a chunk being recorded, the slot with
>> TREF_FRAME is not rewritten (but the next empty slot after TREF_FRAME
>> is). This leads to JIT slots inconsistency and assertion failure in
>> `rec_check_slots()` during recording of the next bytecode instruction.
>>
>> This patch fixes bytecode register allocation by changing the VM
>> register allocation order in case of ISGT and ISGE bytecodes.
>>
>> Sergey Kaplun:
>> * added the description and the test for the problem
>>
>> Resolves tarantool/tarantool#6227
>> Part of tarantool/tarantool#5629
>> ===================================================================
>>
>> On 16.08.21, Igor Munkin wrote:
>
><snipped>
>
>>
>> >
>> > Furthermore, what does stop you from using local variables?
>>
>> They occupy new slots and make it harder to maintain, see the new
>> comment below.
>
>Meh, OK anyway :)
>
>>
>> >
>
><snipped>
>
>>
>> ===================================================================
>> diff --git a/test/tarantool-tests/gh-6227-bytecode-allocator-for-comparisons.test.lua b/test/tarantool-tests/gh-6227-bytecode-allocator-for-comparisons.test.lua
>> index 66f6885e..9788923a 100644
>> --- a/test/tarantool-tests/gh-6227-bytecode-allocator-for-comparisons.test.lua
>> +++ b/test/tarantool-tests/gh-6227-bytecode-allocator-for-comparisons.test.lua
>> @@ -14,26 +14,39 @@ local function empty() end
>> local uv = 0
>>
>> -- This function needs to reset register enumerating.
>> --- Also set `J->maxslot` to zero.
>> --- The upvalue function to call is loaded to 0 slot.
>> +-- `J->maxslot` is initialized with `nargs` (i.e. zero in this
>> +-- case) in `rec_call_setup()`.
>> local function bump_frame()
>> -- First call function with RET0 to set TREF_FRAME in the
>> -- last slot.
>> empty()
>> + -- The old bytecode to be recorded looks like the following:
>> + -- 0000 . FUNCF 4
>> + -- 0001 . UGET 0 0 ; empty
>> + -- 0002 . CALL 0 1 1
>> + -- 0000 . . JFUNCF 1 1
>> + -- 0001 . . RET0 0 1
>> + -- 0002 . CALL 0 1 1
>> + -- 0003 . UGET 0 0 ; empty
>> + -- 0004 . UGET 3 1 ; uv
>> + -- 0005 . KSHORT 2 1
>> + -- 0006 . ISLT 3 2
>> -- Test ISGE or ISGT bytecode. These bytecodes swap their
>> - -- operands. Also, a constant is always loaded into the slot
>> - -- smaller than upvalue. So, if upvalue loads before KSHORT,
>> - -- then the difference between registers is more than 2 (2 is
>> - -- needed for LJ_FR2) and TREF_FRAME slot is not rewriting by
>> - -- the bytecode after call and return as expected. That leads
>> - -- to recording slots inconsistency and assertion failure at
>> - -- `rec_check_slots()`.
>> + -- operands (consider ISLT above).
>> + -- Two calls of `empty()` function in a row is necessary for 2
>> + -- slot gap in LJ_FR2 mode.
>> + -- Upvalue loads before KSHORT, so the difference between slot
>> + -- for upvalue `empty` (function to be called) and slot for
>> + -- upvalue `uv` is more than 2. Hence, TREF_FRAME slot is not
>> + -- rewritten by the bytecode after return from `empty()`
>> + -- function as expected. That leads to recording slots
>> + -- inconsistency and assertion failure at `rec_check_slots()`.
>> empty(1>uv)
>> end
>>
>> jit.opt.start('hotloop=1')
>>
>> -for _ = 1,3 do
>> +for _ = 1, 3 do
>> bump_frame()
>> end
>> ===================================================================
>>
>
><snipped>
>
>>
>> --
>> Best regards,
>> Sergey Kaplun
>
>--
>Best regards,
>IM
[-- Attachment #2: Type: text/html, Size: 5990 bytes --]
next prev parent reply other threads:[~2021-08-17 7:36 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-19 7:36 Sergey Kaplun via Tarantool-patches
2021-08-01 10:43 ` Igor Munkin via Tarantool-patches
2021-08-01 17:10 ` Sergey Kaplun via Tarantool-patches
2021-08-16 7:20 ` Igor Munkin via Tarantool-patches
2021-08-16 16:40 ` Sergey Kaplun via Tarantool-patches
2021-08-16 16:27 ` Igor Munkin via Tarantool-patches
2021-08-17 7:36 ` Vitaliia Ioffe via Tarantool-patches [this message]
2021-08-10 17:03 ` Sergey Ostanevich via Tarantool-patches
2021-08-16 16:44 ` Sergey Kaplun via Tarantool-patches
2021-08-17 9:24 ` Igor Munkin via Tarantool-patches
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1629185771.534192263@f337.i.mail.ru \
--to=tarantool-patches@dev.tarantool.org \
--cc=imun@tarantool.org \
--cc=v.ioffe@tarantool.org \
--subject='Re: [Tarantool-patches] [PATCH luajit] Fix bytecode register allocation for comparisons.' \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox