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

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

[Sergey Kaplun via Tarantool-patches]

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
we faced an assertion failure in `fwd_ahload()` due to values types
mismatch, either we faced an assertion failure in `rec_check_slots()`
since forwarded value by the JIT compiler isn't the same as it is in the
interpreter.

This patch adds a check that there is no any `IR_NEWREF` after table
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.
+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.
+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

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

[Maxim Kokryashkin via Tarantool-patches]

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
> 

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

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for your review!
I fixed your comments and force-pushed the branch.

On 20.08.23, Maxim Kokryashkin wrote:
> 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/

Fixed.

> > we faced an assertion failure in `fwd_ahload()` due to values types
> Typo: s/values/value/

Fixed.

> > mismatch, either we faced an assertion failure in `rec_check_slots()`
> Typo: s/either/or/

Fixed.

> > since forwarded value by the JIT compiler isn't the same as it is in the
> Typo: s/since/since the/

Fixed.

> > interpreter.
> > 
> > This patch adds a check that there is no any `IR_NEWREF` after table
> Typo: s/no any/no/

Fixes.

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

<snipped>

> > +-- 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/

Fixed.

> > +for i = 6, 9 do

<snipped>

> > +
> > +-- 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.

Fixed, see iterative patch below. Branch is force-pushed.

===================================================================
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
index a27932df..643b7823 100644
--- 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
@@ -15,7 +15,8 @@ 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.
+-- types.". But is still added it to avoid regressions 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
@@ -36,7 +37,10 @@ end
 
 test:is(result, 9, 'TNEW load forwarding')
 
--- TNEW load forwarding, aliased table.
+-- TNEW load forwarding, aliased table. It doesn't trigger an
+-- assert since the commit "Fix TNEW load forwarding with instable
+-- types.". But is still added it to avoid regressions in the
+-- future.
 local alias_store = {{}, {}, {}, {}, {}}
 for i = 6, 9 do
   local t = {i, i, i, i, i, i, i}
===================================================================

> > +local alias_store = {{}, {}, {}, {}, {}}

<snipped>

> > 2.41.0
> > 

-- 
Best regards,
Sergey Kaplun

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

[Sergey Bronnikov via Tarantool-patches]

Sergey,


thanks for the patch. See my comment inline.


On 8/17/23 17:46, Sergey Kaplun wrote:

<snipped>


> +++ 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.
> +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.
> +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)


Is it possible to fold loop bodies to a single function?

Otherwise it is too much duplicate code in the test.

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

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the review!
Please, see my answers below.

On 28.08.23, Sergey Bronnikov wrote:
> Sergey,
> 
> 
> thanks for the patch. See my comment inline.
> 

<snipped>

> 
> 
> Is it possible to fold loop bodies to a single function?
> 
> Otherwise it is too much duplicate code in the test.

I prefer not to do it since:
* It's complicated traces, so it's harder to understand what is this
  test about.
* Some function's may be recorded instead, so the issue will not
  occure. Or test becomes flaky.

-- 
Best regards,
Sergey Kaplun

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

[Sergey Bronnikov via Tarantool-patches]

On 8/28/23 17:26, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the review!
> Please, see my answers below.
>
> On 28.08.23, Sergey Bronnikov wrote:
>> Sergey,
>>
>>
>> thanks for the patch. See my comment inline.
>>
> <snipped>
>
>>
>> Is it possible to fold loop bodies to a single function?
>>
>> Otherwise it is too much duplicate code in the test.
> I prefer not to do it since:
> * It's complicated traces, so it's harder to understand what is this
>    test about.
> * Some function's may be recorded instead, so the issue will not
>    occure. Or test becomes flaky.
Okay, accepted. LGTM

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

[Igor Munkin via Tarantool-patches]

Sergey,

I've checked the patchset into all long-term branches in
tarantool/luajit and bumped a new version in master, release/2.11 and
release/2.10.

On 17.08.23, Sergey Kaplun via Tarantool-patches 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
> we faced an assertion failure in `fwd_ahload()` due to values types
> mismatch, either we faced an assertion failure in `rec_check_slots()`
> since forwarded value by the JIT compiler isn't the same as it is in the
> interpreter.
> 
> This patch adds a check that there is no any `IR_NEWREF` after table
> 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
> 

<snipped>

> -- 
> 2.41.0
> 

-- 
Best regards,
IM