Re: [Tarantool-patches] [PATCH luajit] LJ_GC64: Fix ir_khash for non-string GCobj.

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Maxim!
Thanks for the review!

Fixed your comments, branch is force pushed.

On 12.12.22, Maxim Kokryashkin wrote:
> 
> Hi, Sergey!
> Thanks for the patch!
> LGTM, except for a few nits below.
>  
> > 
> >>From: Mike Pall <mike>
> >>
> >>Contributed by Peter Cawley.
> >>
> >>(cherry picked from commit b4ed3219a1a98dd9fe7d1e3eeea3b82f5a780948)
> >>
> >>When emitting `IR_HREF` for constant value to lookup the `ir_khash()`
> >Typo: s/for constant/for a constant
> >>function is used to calculate hash for the corresponding object.
> >Typo: s/hash/the hash
> >>This calculation must be the same as in the corresponding `hashkey()`
> >>function from <lj_tab.c>.
> >>
> >>Hash calculating via passing two arguments `lo`, and `hi` to `hashrot()`
> >Typo: s/calculating via/is calculated by
> >>routine. For non-string GC objects the first `lo` argument is the same
> >>for GC64 and not GC64 mode -- lower 32 bits of the object address. For
> >>GC64 mode `hi` argument is upper 32 bits of the object address,
> >>including specific type NaN-tag. This `hi` argument in `ir_khash()`
> >>function is miscalculated in GC64 using non-GC64 value (`lo` +
> >>`HASH_BIAS`). As a result, the hash for the GC object is miscalculated
> >>on trace and we exit from trace due to assertion guard on the type or
> >>value check.
> >>
> >>This patch fixes calculation of hash value on trace for GC64 mode by
> >>making it consistent with `hashkey()`.
> >Typo: s/of hash/of the hash

Fixed. New commit message is:

| LJ_GC64: Fix ir_khash for non-string GCobj.
|
| Contributed by Peter Cawley.
|
| (cherry picked from commit b4ed3219a1a98dd9fe7d1e3eeea3b82f5a780948)
|
| When emitting the `IR_HREF` for a constant value lookup the `ir_khash()`
| function is used to calculate the hash for the corresponding object.
| This calculation must be the same as in the corresponding `hashkey()`
| function from <lj_tab.c>.
|
| Hash is calculated by passing two arguments `lo`, and `hi` to the
| `hashrot()` routine. For non-string GC objects the first `lo` argument
| is the same for GC64 and not GC64 mode -- lower 32 bits of the object
| address. For GC64 mode `hi` argument is upper 32 bits of the object
| address, including a specific type NaN-tag. This `hi` argument in
| `ir_khash()` function is miscalculated in GC64 mode using a non-GC64
| value (`lo` + `HASH_BIAS`). As a result, the hash for the GC object is
| miscalculated on trace and we exit from the trace due to an assertion
| guard on the type or value check.
|
| This patch fixes calculation of the hash value on trace for GC64 mode by
| making it consistent with the `hashkey()`.
|
| Sergey Kaplun:
| * added the description and the test for the problem
|
| Part of tarantool/tarantool#7230


> >>
> >>Sergey Kaplun:
> >>* added the description and the test for the problem
> >>
> >>Part of tarantool/tarantool#7230
> >>---
> >>
> >>Branch:  https://github.com/tarantool/luajit/tree/skaplun/lj-356-ir-khash-non-string-obj-full-ci
> >>Issue/PR:
> >>*  https://github.com/tarantool/tarantool/issues/7230
> >>*  https://github.com/LuaJIT/LuaJIT/pull/356
> >>Tarantool PR:  https://github.com/tarantool/tarantool/pull/8020
> >>
> >>Side note: Problems with red fuzzer jobs look irrelevant to the patch.

<snipped>

> >>+
> >>+-- Prevent `get_const_cdata()` become hot and be compiled before
> >>+-- the main test cycle.
> >Typo: s/become hot and be/from becoming hot and being
> >>+jit.off()
> >>+
> >>+filled_tab[get_const_cdata()] = MAGIC
> >>+
> >>+-- Speed up table filling-up.
> >>+jit.on()
> >>+
> >>+-- Filling-up the table with GC values to minimize the amount of
> >>+-- hash collisions and increases delta between the start of the
> >Typo: s/increases/increase
> >>+-- hash part of the table and currently stored node.
> >>+for i = 1, N_HASH_FI

<snipped>

> >>--

Iterative patch with the fixes:
===================================================================
diff --git a/test/tarantool-tests/lj-356-ir-khash-non-string-obj.test.lua b/test/tarantool-tests/lj-356-ir-khash-non-string-obj.test.lua
index fff0b1a5..7f304183 100644
--- a/test/tarantool-tests/lj-356-ir-khash-non-string-obj.test.lua
+++ b/test/tarantool-tests/lj-356-ir-khash-non-string-obj.test.lua
@@ -3,7 +3,7 @@ local traceinfo = require('jit.util').traceinfo
 local table_new = require('table.new')
 
 -- Test file to demonstrate the incorrect GC64 JIT behaviour
--- for `IR_HREF` for on-trace-constant key lookup.
+-- of an `IR_HREF` for the on-trace-constant key lookup.
 -- See also https://github.com/LuaJIT/LuaJIT/pull/356.
 local test = tap.test('lj-356-ir-khash-non-string-obj')
 local N_ITERATIONS = 4
@@ -14,10 +14,10 @@ test:plan(N_ITERATIONS + 1)
 
 -- To reproduce the issue we need to compile a trace with
 -- `IR_HREF`, with a lookup of constant hash key GC value. To
--- prevent `IR_HREFK` to be emitted instead, we need a table with
--- a huge hash part. Delta of address between the start of the
--- hash part of the table and the current node to lookup must be
--- more than `(1024 * 64 - 1) * sizeof(Node)`.
+-- prevent an `IR_HREFK` to be emitted instead, we need a table
+-- with a huge hash part. Delta of address between the start of
+-- the hash part of the table and the current node to lookup must
+-- be more than `(1024 * 64 - 1) * sizeof(Node)`.
 -- See <src/lj_record.c>, for details.
 -- XXX: This constant is well suited to prevent test to be flaky,
 -- because the aforementioned delta is always large enough.
@@ -36,8 +36,8 @@ end
 -- exiting the main test cycle.
 jit.opt.start('hotloop=1')
 
--- Prevent `get_const_cdata()` become hot and be compiled before
--- the main test cycle.
+-- Prevent `get_const_cdata()` from becoming hot and being
+-- compiled before the main test cycle.
 jit.off()
 
 filled_tab[get_const_cdata()] = MAGIC
@@ -46,10 +46,10 @@ filled_tab[get_const_cdata()] = MAGIC
 jit.on()
 
 -- Filling-up the table with GC values to minimize the amount of
--- hash collisions and increases delta between the start of the
+-- hash collisions and increase delta between the start of the
 -- hash part of the table and currently stored node.
-for i = 1, N_HASH_FIELDS do
-  filled_tab[1LL] = i
+for _ = 1, N_HASH_FIELDS do
+  filled_tab[1LL] = 1
 end
 
 -- Prevent JIT misbehaviour before the main test chunk.
===================================================================

> >>2.34.1
> >--
> >Best regards,
> >Maxim Kokryashkin
> > 

-- 
Best regards,
Sergey Kaplun