Re: [Tarantool-patches] [PATCH luajit 03/19] MIPS: Fix handling of spare long-range jump slots.

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

Hi, Maxim!
Thanks for the review!
Fixed your comments inline.

On 15.08.23, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the patch!
> LGTM, except for a few comments below.
> On Wed, Aug 09, 2023 at 06:35:52PM +0300, Sergey Kaplun via Tarantool-patches wrote:
> > From: Mike Pall <mike>
> > 
> > Contributed by Djordje Kovacevic and Stefan Pejic.
> > 
> > (cherry-picked from commit c7c3c4da432ddb543d4b0a9abbb245f11b26afd0)
> > 
> > `asm_setup_jump()` in <src/lj_asm_mips.h> presumes that `sizeof(MCLink)`
> > is 8 bytes, but for MIPS64 its size is 16 bytes. This leads to incorrect
> Typo: s/to incorrect/to an incorrect/

Fixed.

> > check in `asm_sparejump_setup()`, so mcode bottom is not updated.
> Typo: s/so mcode/so the mcode/

Fixed.

> > 
> > This patch fixes check of the MCLink offset from the mcbot.
> Typo: s/fixes check/fixes the check/

Fixed.

> > Nevertheless, the emitting of spare jump slots is still incorrect, so
> > the introduced test still fails due to incorrect iteration through the
> Typo: s/due to/due to the/

Fixed.

> > sparce table (the last slot is out of mcode range).
> > 
> > This should be fixed via backporting of the commit
> > dbb78630169a8106b355a5be8af627e98c362f1e ("MIPS: Fix handling of
> > long-range spare jumps."). But it triggers the new unconditional
> > assert, that is added in this patch, mentioning that sizemcode is too
> > bit. So some workaround should be found, when this test will be enabled
> Typo: s/bit/big/
> Typo: s/will be/is/

Fixed, thanks!

> > for MIPS.
> > 
> > Since test also validates the behaviour of long-range jumps to side
> > traces for arm64 and x64, and we have no testing for MIPS64 (yet), we
> > can leave it as is without a skipcond.
> > 
> > Sergey Kaplun:
> > * added the description and the test for the problem
> > 
> > Part of tarantool/tarantool#8825
> > ---
> >  src/lj_asm_mips.h                             |  9 +--
> >  src/lj_jit.h                                  |  6 ++
> >  src/lj_mcode.c                                |  6 --
> >  ...x-mips64-spare-side-exit-patching.test.lua | 65 +++++++++++++++++++
> >  4 files changed, 76 insertions(+), 10 deletions(-)
> >  create mode 100644 test/tarantool-tests/fix-mips64-spare-side-exit-patching.test.lua
> > 

<snipped>

> > diff --git a/test/tarantool-tests/fix-mips64-spare-side-exit-patching.test.lua b/test/tarantool-tests/fix-mips64-spare-side-exit-patching.test.lua
> > new file mode 100644
> > index 00000000..fdc826cb
> > --- /dev/null
> > +++ b/test/tarantool-tests/fix-mips64-spare-side-exit-patching.test.lua
> > @@ -0,0 +1,65 @@
> > +local tap = require('tap')
> > +local test = tap.test('fix-mips64-spare-side-exit-patching'):skipcond({
> > +  ['Test requires JIT enabled'] = not jit.status(),
> > +  ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
> > +  -- Need to fix the MIPS behaviour first.
> Typo: s/Need to/We need to/

Fixed.

> > +  ['Disabled for MIPS architectures'] = jit.arch:match('mips'),

<snipped>

> > +  -- Allow to use 2000 traces to avoid flushes.
> Typo: s/to use/compilation of up to/

Fixed.

> > +  'maxtrace=2000',
> > +  -- Allow to compile 8Mb of mcode to be sure the issue occurs.
> Typo: s/to compile/compilation of up to/

Fixed.

> > +  'maxmcode=8192',
> > +  -- Use big mcode area for traces to avoid using different
> Typo: s/using/usage of/

Fixed.

> > +  -- spare slots.
> > +  'sizemcode=256'
> > +)
> > +
> > +local MAX_SPARE_SLOT = 4
> A link to the definition in `lj_asm_mips.h` would be nice to have.

Added.

> 
> > +local function parent(marker)
> > +  -- Use several side exit to fill spare exit space (default is
> Typo: s/side exit/side exits/

Fixed, thanks!

> > +  -- 4 slots, each slot has 2 instructions -- jump and nop).
> > +  -- luacheck: ignore
> > +  if marker > MAX_SPARE_SLOT then end
> > +  if marker > 3 then end
> > +  if marker > 2 then end
> > +  if marker > 1 then end
> > +  if marker > 0 then end
> > +  -- XXX: use `fmod()` to avoid leaving the function and use
> > +  -- stitching here.
> > +  return math.fmod(1, 1)
> > +end
> > +
> > +-- Compile parent trace first.
> > +parent(0)
> > +parent(0)
> > +
> > +local parent_traceno = frontend.gettraceno(parent)
> > +local last_traceno = parent_traceno
> > +
> > +-- Now generate some mcode to forcify long jump with a spare slot.
> > +-- Each iteration provide different addresses and uses a different
> Typo: s/provide/provides/

Fixed, thanks!

> > +-- spare slot. After it compile and execute new side trace.
> Typo: s/After it compile and execute/After that, compiles and executes a/

Fixed.

See the iterative patch below.

===================================================================
diff --git a/test/tarantool-tests/fix-mips64-spare-side-exit-patching.test.lua b/test/tarantool-tests/fix-mips64-spare-side-exit-patching.test.lua
index fdc826cb..62933df9 100644
--- a/test/tarantool-tests/fix-mips64-spare-side-exit-patching.test.lua
+++ b/test/tarantool-tests/fix-mips64-spare-side-exit-patching.test.lua
@@ -2,7 +2,7 @@ local tap = require('tap')
 local test = tap.test('fix-mips64-spare-side-exit-patching'):skipcond({
   ['Test requires JIT enabled'] = not jit.status(),
   ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
-  -- Need to fix the MIPS behaviour first.
+  -- We need to fix the MIPS behaviour first.
   ['Disabled for MIPS architectures'] = jit.arch:match('mips'),
 })
 
@@ -18,18 +18,19 @@ jit.opt.start(
   -- Try to compile all compiled paths as early as JIT can.
   'hotloop=1',
   'hotexit=1',
-  -- Allow to use 2000 traces to avoid flushes.
+  -- Allow compilation of up to 2000 traces to avoid flushes.
   'maxtrace=2000',
   -- Allow to compile 8Mb of mcode to be sure the issue occurs.
   'maxmcode=8192',
-  -- Use big mcode area for traces to avoid using different
+  -- Use big mcode area for traces to avoid usage of different
   -- spare slots.
   'sizemcode=256'
 )
 
+-- See the define in the <src/lj_asm_mips.h>.
 local MAX_SPARE_SLOT = 4
 local function parent(marker)
-  -- Use several side exit to fill spare exit space (default is
+  -- Use several side exits to fill spare exit space (default is
   -- 4 slots, each slot has 2 instructions -- jump and nop).
   -- luacheck: ignore
   if marker > MAX_SPARE_SLOT then end
@@ -50,8 +51,9 @@ local parent_traceno = frontend.gettraceno(parent)
 local last_traceno = parent_traceno
 
 -- Now generate some mcode to forcify long jump with a spare slot.
--- Each iteration provide different addresses and uses a different
--- spare slot. After it compile and execute new side trace.
+-- Each iteration provides different addresses and uses a
+-- different spare slot. After that, compiles and executes a new
+-- side trace.
 for i = 1, MAX_SPARE_SLOT + 1 do
   generators.fillmcode(last_traceno, 1024 * 1024)
   parent(i)
===================================================================

<snipped>

> > 2.41.0
> > 

-- 
Best regards,
Sergey Kaplun