[Tarantool-patches] [PATCH luajit] Fix TDUP load forwarding after table rehash.
Maxim Kokryashkin
m.kokryashkin at tarantool.org
Sun Aug 20 15:13:40 MSK 2023
Hi, Sergey!
Thanks for the patch!
LGTM, except for a few comments below.
On Thu, Aug 17, 2023 at 05:46:41PM +0300, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Reported by Sergey Kaplun.
>
> (cherry-picked from commit c7db8255e1eb59f933fac7bc9322f0e4f8ddc6e6)
>
> After table rehashing number keys loaded via ALOAD may be placed in the
> hash part of the table. So, load forwarding analysis missed the
> corresponding stores like they are never existed. In such case, either
Typo: s/they are/they/
Typo: s/In such/In that/
> we faced an assertion failure in `fwd_ahload()` due to values types
Typo: s/values/value/
> mismatch, either we faced an assertion failure in `rec_check_slots()`
Typo: s/either/or/
> since forwarded value by the JIT compiler isn't the same as it is in the
Typo: s/since/since the/
> interpreter.
>
> This patch adds a check that there is no any `IR_NEWREF` after table
Typo: s/no any/no/
> creation, so it can't be rehashed.
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#8825
> ---
>
> Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-980-load-fwd-after-table-rehash
> PR: https://github.com/tarantool/tarantool/pull/8998
> Related issues:
> * https://github.com/LuaJIT/LuaJIT/issues/980
> * https://github.com/tarantool/tarantool/issues/8825
>
> src/lj_opt_mem.c | 6 +
> ...j-980-load-fwd-after-table-rehash.test.lua | 166 ++++++++++++++++++
> 2 files changed, 172 insertions(+)
> create mode 100644 test/tarantool-tests/lj-980-load-fwd-after-table-rehash.test.lua
>
> diff --git a/src/lj_opt_mem.c b/src/lj_opt_mem.c
> index 59fddbdd..7b610506 100644
> --- a/src/lj_opt_mem.c
> +++ b/src/lj_opt_mem.c
> @@ -157,6 +157,7 @@ static TRef fwd_ahload(jit_State *J, IRRef xref)
> if (ir->o == IR_TNEW || (ir->o == IR_TDUP && irref_isk(xr->op2))) {
> /* A NEWREF with a number key may end up pointing to the array part.
> ** But it's referenced from HSTORE and not found in the ASTORE chain.
> + ** Or a NEWREF may rehash the table and move unrelated number keys.
> ** For now simply consider this a conflict without forwarding anything.
> */
> if (xr->o == IR_AREF) {
> @@ -167,6 +168,11 @@ static TRef fwd_ahload(jit_State *J, IRRef xref)
> goto cselim;
> ref2 = newref->prev;
> }
> + } else {
> + IRIns *key = IR(xr->op2);
> + if (key->o == IR_KSLOT) key = IR(key->op1);
> + if (irt_isnum(key->t) && J->chain[IR_NEWREF] > tab)
> + goto cselim;
> }
> /* NEWREF inhibits CSE for HREF, and dependent FLOADs from HREFK/AREF.
> ** But the above search for conflicting stores was limited by xref.
> diff --git a/test/tarantool-tests/lj-980-load-fwd-after-table-rehash.test.lua b/test/tarantool-tests/lj-980-load-fwd-after-table-rehash.test.lua
> new file mode 100644
> index 00000000..a27932df
> --- /dev/null
> +++ b/test/tarantool-tests/lj-980-load-fwd-after-table-rehash.test.lua
> @@ -0,0 +1,166 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate LuaJIT misbehaviour during load
> +-- forwarding optimization for HLOAD after table rehashing.
> +-- See also https://github.com/LuaJIT/LuaJIT/issues/980.
> +
> +local test = tap.test('lj-980-load-fwd-after-table-rehash'):skipcond({
> + ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +test:plan(6)
> +
> +jit.opt.start('hotloop=1')
> +
> +local result
> +-- The test for TNEW load forwarding. It doesn't trigger an assert
> +-- since the commit "Fix TNEW load forwarding with instable
> +-- types.". But still add it to avoid regressions in future.
Typo: s/But still add it/But is still added/
Typo: s/in future/in the future/
> +for i = 6, 9 do
> + -- Need big enough table to see rehashing.
> + -- Also, to simplify logic with AREF, HREF don't use default
> + -- 1, 4 (start, stop) values here.
> + local t = {i, i, i, i, i, i, i}
> + -- Insert via ASTORE.
> + t[i] = i
> + t[1] = nil
> + t[2] = nil
> + t[3] = nil
> + t[4] = nil
> + t[5] = nil
> + -- Rehash table. Array part is empty.
> + t['1000'] = 1000
> + -- Load via HLOAD.
> + result = t[i]
> +end
> +
> +test:is(result, 9, 'TNEW load forwarding')
> +
> +-- TNEW load forwarding, aliased table.
The same label about this test being added only for the sake of
regressional testing should be added here too.
> +local alias_store = {{}, {}, {}, {}, {}}
> +for i = 6, 9 do
> + local t = {i, i, i, i, i, i, i}
> + alias_store[#alias_store + 1] = t
> + local alias = alias_store[i]
> + -- Insert via ASTORE.
> + alias[i] = i
> + alias[1] = nil
> + alias[2] = nil
> + alias[3] = nil
> + alias[4] = nil
> + alias[5] = nil
> + -- Rehash table. Array part is empty.
> + alias['1000'] = 1000
> + -- Load via HLOAD.
> + result = t[i]
> +end
> +
> +test:is(result, 9, 'TNEW load forwarding, aliased table')
> +
> +local expected = 'result'
> +
> +-- TDUP different types.
> +for i = 6, 9 do
> + local t = {1, 2, 3, 4, 5, 6, 7, 8}
> + t[i] = expected
> + t[i + 1] = expected
> + t[1] = nil
> + t[2] = nil
> + t[3] = nil
> + t[4] = nil
> + t[5] = nil
> + t[6] = nil
> + -- Rehash table. Array part is empty.
> + t['1000'] = 1000
> + -- Result on the recording (i == 8) iteration is 'result'.
> + -- Nevertheless, on the last (i == 9) iteration it is 8.
> + -- Just check that there is no assert failure here.
> + -- Load via HLOAD.
> + result = t[8]
> +end
> +
> +-- Checked for assertion guard, on the last iteration we get
> +-- the value on initializatoin.
> +test:is(result, 8, 'TDUP load forwarding different types')
> +
> +-- TDUP different types, aliased table.
> +alias_store = {{}, {}, {}, {}, {}}
> +for i = 6, 9 do
> + local t = {1, 2, 3, 4, 5, 6, 7, 8}
> + -- Store table, to be aliased later.
> + alias_store[#alias_store + 1] = t
> + local alias = alias_store[i]
> + alias[i] = expected
> + alias[i + 1] = expected
> + alias[1] = nil
> + alias[2] = nil
> + alias[3] = nil
> + alias[4] = nil
> + alias[5] = nil
> + alias[6] = nil
> + -- Rehash table. Array part is empty.
> + alias['1000'] = 1000
> + -- Result on the recording (i == 8) iteration is 'result'.
> + -- Nevertheless, on the last (i == 9) iteration it is 8.
> + -- Just check that there is no assert failure here.
> + -- Load via HLOAD.
> + result = t[8]
> +end
> +
> +-- Checked for assertion guard, on the last iteration we get
> +-- the value on initializatoin.
> +test:is(result, 8, 'TDUP load forwarding different types, aliased table')
> +
> +-- TDUP same type, different values.
> +for i = 6, 9 do
> + local t = {1, 2, 3, 4, 5, 6, '7', '8'}
> + t[i] = expected
> + t[i + 1] = expected
> + t[1] = nil
> + t[2] = nil
> + t[3] = nil
> + t[4] = nil
> + t[5] = nil
> + t[6] = nil
> + -- Rehash table. Array part is empty.
> + t['1000'] = 1000
> + -- Result on the recording (i == 8) iteration is 'result'.
> + -- Nevertheless, on the last (i == 9) iteration it is '8'.
> + -- Just check that there is no assert failure here.
> + -- Load via HLOAD.
> + result = t[8]
> +end
> +
> +-- Checked for assertion guard, on the last iteration we get
> +-- the value on initializatoin.
> +test:is(result, '8', 'TDUP load forwarding same type, different values')
> +
> +alias_store = {{}, {}, {}, {}, {}}
> +for i = 6, 9 do
> + local t = {1, 2, 3, 4, 5, 6, '7', '8'}
> + -- Store table, to be aliased later.
> + alias_store[#alias_store + 1] = t
> + local alias = alias_store[i]
> + alias[i] = expected
> + alias[i + 1] = expected
> + alias[1] = nil
> + alias[2] = nil
> + alias[3] = nil
> + alias[4] = nil
> + alias[5] = nil
> + alias[6] = nil
> + -- Rehash table. Array part is empty.
> + alias['1000'] = 1000
> + -- Result on the recording (i == 8) iteration is 'result'.
> + -- Nevertheless, on the last (i == 9) iteration it is '8'.
> + -- Just check that there is no assert failure here.
> + -- Load via HLOAD.
> + result = t[8]
> +end
> +
> +-- Checked for assertion guard, on the last iteration we get
> +-- the value on initializatoin.
> +test:is(result, '8',
> + 'TDUP load forwarding same type, different values, aliased table')
> +
> +test:done(true)
> --
> 2.41.0
>
More information about the Tarantool-patches
mailing list