Re: [Tarantool-patches] [PATCH luajit] ARM64: Fix {AHUV}LOAD specialized to nil/false/true.

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

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

On 28.02.23, Maxim Kokryashkin wrote:
> 
> Hi, Sergey!
> Thanks for the patch!
> LGTM, except for a few nits below.
>  
> > 
> >>From: Mike Pall <mike>
> >>
> >>Reported by caohongqing.
> >>
> >>(cherry picked from commit 5bf0da3d7c02f9959fa3a9fb721e0565137b70c8)
> >>
> >>If there is high register pressure, and there are almost all registers
> >>in use during the aforementioned assembling, the same register is chosen
> >>as the one holding the given stack slot and the one holding the constant
> >>value for the type comparison. As the result we get the following
> >>assertion guard check in assembly:
> >>| cmp x0, x0, lsr #32
> >>| bne ->0
> >>Which is always false.
> >>
> >>This happens because the `tmp` register (used for loading constant type
> >>to compare in assertion guard) is scratched from `gpr` register set, but
> >>not from `allow` set, which is used during picking the register for slot
> >>loading (at the begging `allow` and `gpr` sets are the same).

Fixed typo: s/begging/beginning/

> >>
> >>This patch changes `allow` set to `gpr` to fix the issue.
> >>
> >>Sergey Kaplun:
> >>* added the description and the test for the problem
> >>
> >>Part of tarantool/tarantool#8069
> >>---

<snipped>

> >>+ local upvalue = true
> >>+ local function uload()
> >>+ return upvalue
> >>+ end
> >>+ -- Make upvalue muttable. Not really need to return this
> >Typo: s/muttable/mutable/

Fixed, thanks!

> >>+ -- function.
> >>+ local function _()
> >>+ upvalue = not upvalue
> >>+ end
> >>+ _G.uload = uload
> >>+end
> >>+
> >>+-- This function generate code like the following:
> >Typo: s/generate/generates/

Fixed, thanks!

> >>+-- | local test_f(...)
> >>+-- | local r
> >>+-- | local rup1
> >>+-- | --[[...]]
> >>+-- | for _ = 1, 4 do
> >>+-- | r1 = ffi.cast("int", 1)
> >>+-- | --[[...]]
> >>+-- | r = main_payload()
> >>+-- | rup1 = r1
> >>+-- | --[[...]]
> >>+-- | end
> >>+-- | end
> >>+-- | return test_f
> >>+-- Those `rn` variables before and after `main_payload` are
> >>+-- required to generate enough register pressure (for GPR). Amount
> >>+-- of repeats is empirical.
> >>+-- Additional `test_f(...)` wrapper is needed for IR_VLOAD usage,
> >>+-- when `main_payload` is just `...`.
> >>+local function generate_payload(n_fillers, main_payload)
> >>+ local code_chunk = 'local function test_f(...)\n'
> >>+ code_chunk = code_chunk .. 'local r\n'
> >>+ for i = 1, n_fillers do
> >>+ code_chunk = code_chunk .. ('local rup%d\n'):format(i)
> >>+ end
> >>+ code_chunk = code_chunk .. 'for _ = 1, 4 do\n'
> >>+ for i = 1, n_fillers do
> >>+ code_chunk = code_chunk ..
> >>+ ('local r%d = ffi.cast("int", %d)\n'):format(i, i)
> >>+ end
> >>+ code_chunk = code_chunk .. 'r = ' .. main_payload .. '\n'
> >>+ for i = 1, n_fillers do
> >>+ code_chunk = code_chunk .. ('rup%d = r%d\n'):format(i, i)
> >>+ end
> >>+ code_chunk = code_chunk .. 'end\nend\n'
> >>+ code_chunk = code_chunk .. 'return test_f'
> >>+ local f, err = loadstring(code_chunk, 'test_function')
> >>+ assert(type(f) == 'function', err)
> >>+ f = f()
> >>+ assert(type(f) == 'function', 'returned generated value is not a function')
> >>+ return f
> >>+end
> >That section is really hard to read, is there any way to make it more
> >readable? I believe even slight reformatting might help.

I've add the additional empty lines to separate cycle's body
generation.
Does it help?

| local function generate_payload(n_fillers, main_payload)
|   local code_chunk = 'local function test_f(...)\n'
|   code_chunk = code_chunk .. 'local r\n'
|   for i = 1, n_fillers do
|     code_chunk = code_chunk .. ('local rup%d\n'):format(i)
|   end
|
|   code_chunk = code_chunk .. 'for _ = 1, 4 do\n'
|   for i = 1, n_fillers do
|     code_chunk = code_chunk ..
|       ('local r%d = ffi.cast("int", %d)\n'):format(i, i)
|   end
|   code_chunk = code_chunk .. 'r = ' .. main_payload .. '\n'
|   for i = 1, n_fillers do
|     code_chunk = code_chunk .. ('rup%d = r%d\n'):format(i, i)
|   end
|
|   code_chunk = code_chunk .. 'end\nend\n'
|   code_chunk = code_chunk .. 'return test_f'
|
|   local f, err = loadstring(code_chunk, 'test_function')
|   assert(type(f) == 'function', err)
|   f = f()
|   assert(type(f) == 'function', 'returned generated value is not a function')
|   return f
| end

> >>+
> >>+-- Disable sink optimization to allocate more registers in a
> >>+-- "convenient" way. 'hotexit' option is required to be sure that
> >>+-- we will start a new trace on false-positive guard assertion.
> >Typo: s/on false-positive/on a false-positive/

Fixed.

> >>+-- The new trace contains the same IR and so the same assertion
> >>+-- guard. This trace will be executed, assertion guard failed
> >>+-- again and the new third trace will be recorded. This trace will
> >Typo: s/assertion guard failed again/the assertion guard will fail again/

Fixed. Thanks!

> >>+-- be the last one to record as far as iterations over cycle are
> >>+-- finished and we returning from the function. The report of
> >Typo: s/we returning/we are returning/

Fixed, thanks!

> >>+-- `jit.dump` before the patch is the following:

<snipped>

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

-- 
Best regards,
Sergey Kaplun