Re: [Tarantool-patches] [PATCH luajit] Fix TDUP load forwarding after table rehash.

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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
>