[Tarantool-patches] [PATCH luajit] Detect inconsistent renames even in the presence of sunk values.
imun at tarantool.org
Mon Aug 2 16:34:25 MSK 2021
Thanks for your review!
On 01.08.21, Sergey Kaplun wrote:
> Hi, Igor!
> Thanks for the patch!
> Is there no point to simplify the test -- we have 5 different traces,
> when really need the only one (with RENAME between two possible jump
> to fallback branches with restoration from snapshot)?
Well... It was a tough issue to provide a stable reproducer to Mike.
Reducing this one is a much more complex issue: we need to compile a
trace for a loop with a rename emitted between two guards with the same
exitno in a variant part and leave the compiled loop via the guard at
the first iteration before the emitted RENAME. Sounds more complex than
even the existing test, doesn't it?
> Feel free to ignore.
> Side note: also, we should test that sunk optimization still works,
> shouldn't we?
Emm, nothing in the patch affects sink optimization per se, so if it's
OK without the patch, it's still OK with it.
> Feel free to ignore.
> Otherwise, LGTM.
Added your tag:
| Reviewed-by: Sergey Kaplun <skaplun at tarantool.org>
> On 24.07.21, Igor Munkin wrote:
> > From: Mike Pall <mike>
> > Reported by Igor Munkin.
> > (cherry picked from commit 33e3f4badfde8cd9c202cedd1f4ed9275bc92e7d)
> > Side exits with the same exitno use the same snapshot for restoring
> > guest stack values. This obliges all guards related to the particular
> > snapshot use the same RegSP mapping for the values to be restored at the
> > trace exit. RENAME emitted prior to the guard for the same snapshot
> > leads to the aforementioned invariant violation.
> This sentence a little bit unclear to me:
> 1) Leads how?
RENAME changes the effective register to be used prior to the particular
snapshot. Hence if RENAME is emitted between the guards with the same
exitno, RegSP mapping is inconsistent for the former one.
> 2) Do you mean it in terms of recording (i. e. the reverse instrucions
> recording order) or not?
The trace is recorded in a direct order, but *assembled* in a reverse
order. I implies the latter one.
> > The easy way to save
> > the snapshot consistency is spilling the renamed IR reference, that is
> > done in scope of <asm_snap_checkrename>.
> > However, the previous <asm_snap_checkrename> implementation considers
> > only the IR references explicitly mentioned in the snapshot. E.g. if
> > there is a sunk object to be restored at the trace exit, and the
> > renamed reference is a *STORE to that object, the spill slot is not
> > allocated. As a result an invalid value is stored while unsinking that
> > object at all corresponding side exits prior to the emitted renaming.
> > To handle also those IR references implicitly used in the snapshot, all
> > non-constant and non-sunk references are added to the Bloom filter (it's
> > worth to mention that two hash functions are used to reduce collisions
> > for the cases when the number of IR references emitted between two
> > different snapshots exceeds the filter size). New <asm_snap_checkrename>
> > implementation tests whether the renamed IR reference is in the filter
> > and forces a spill slot for it as a result.
> > : http://wiki.luajit.org/Allocation-Sinking-Optimization
> > Igor Munkin:
> > * added the description and the test for the problem
> > Resolves tarantool/tarantool#5118
> > Follows up tarantool/tarantool#4252
> > Signed-off-by: Igor Munkin <imun at tarantool.org>
> > ---
> > Related issues:
> > * https://github.com/tarantool/tarantool/issues/5118
> > * https://github.com/tarantool/tarantool/issues/4252
> > * https://github.com/LuaJIT/LuaJIT/issues/584
> > Branch: https://github.com/tarantool/luajit/tree/imun/lj-584-bad-renames-for-sunk-values
> > CI: https://github.com/tarantool/tarantool/commit/b35e2ee
> > src/lj_asm.c | 25 ++++---
> > ...j-584-bad-renames-for-sunk-values.test.lua | 69 +++++++++++++++++++
> > 2 files changed, 81 insertions(+), 13 deletions(-)
> > create mode 100644 test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua
> > --
> > 2.25.0
> Best regards,
> Sergey Kaplun
More information about the Tarantool-patches