Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Maxim Kokryashkin <m.kokryashkin@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit] ARM64: Fix {AHUV}LOAD specialized to nil/false/true.
Date: Thu, 2 Mar 2023 08:52:53 +0300	[thread overview]
Message-ID: <ZAA5tXy+JvQ8hemY@root> (raw)
In-Reply-To: <1677577946.223837963@f341.i.mail.ru>

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

  reply	other threads:[~2023-03-02  5:56 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-25 18:57 Sergey Kaplun via Tarantool-patches
2023-02-28  9:52 ` Maxim Kokryashkin via Tarantool-patches
2023-03-02  5:52   ` Sergey Kaplun via Tarantool-patches [this message]
2023-03-06 15:29     ` Maxim Kokryashkin via Tarantool-patches
2023-03-10 14:49 ` Igor Munkin via Tarantool-patches
2023-03-30 17:37 ` Igor Munkin via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZAA5tXy+JvQ8hemY@root \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=m.kokryashkin@tarantool.org \
    --cc=skaplun@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH luajit] ARM64: Fix {AHUV}LOAD specialized to nil/false/true.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox