Re: [Tarantool-patches] [PATCH luajit] ARM64: Avoid side-effects of constant rematerialization.

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

Hi, Sergos!

Thanks for the review!

On 06.09.22, sergos wrote:
> Hi!
> 
> Thanks for the patch!
> As I can’t say much about the patch from Mike, LGTM.
> Just some nits in the comment.
> 
> Sergos
> 
> 
> > On 31 Aug 2022, at 12:52, Sergey Kaplun <skaplun@tarantool.org> wrote:
> > 
> > From: Mike Pall <mike>
> > 
> > Thanks to Patrick Galizia.
> > 
> > (cherry picked from commit b33e3f2d441590f4de0d189bd9a65661824a48f6)
> > 
> > Constant rematerialization must not use other registers that contain
> > constants, if the register is in-flight. When we have the high
>                                 ^^^^^^
>                                 in use?                          

Fixed.

> > regitster pressure we can face the following issue:
> > 
> > The assembly of an IR instruction allocates a constant into a free
> > register. Then it spills another register (due to high register
> > pressure), which is rematerialized using the same constant (which it
> > assumes is now in the allocated register). In case when the first
> > register also happens to be the destination register, the constant value
> > is modified before the rematerialization.
> > 
> > For the code in the test for this commit we get the following register
> > allocation order (read from top to bottom (DBG RA reversed)):
> > | current IR | operation | IR ref | register
> > |  0048         alloc       0038     x0
> > |  0048         remat       K038     x0
> > |  0048         alloc       K023     x4
> > 
> > Which leads to the following asembly:
> > | ...
> > | add   x4, x4, x0    # x4 modified before x0 rematerialization
> > | ldrb  w4, [x4, #24]
> > | add   x0, x4, #24   # constant x0 rematerialization
> > | ...
> > As a result, the value register x0 holding is incorrect.
> > 
> > This patch moves allocation of constants for earlier to be sure that the
>                                            ^^^ remove it

Fixed, thanks!

> 
> > rematerialization can not make use of the same constant as one of the
> > sources of the IR instruction.
> > 
> > After the patch register allocation order is the following:
> > | current IR | operation | IR ref | register
> > |  0048         alloc       K023     x4
> > |  0048         alloc       0038     x0
> > |  0048         remat       K038     x0
> > 
> > Also, this patch fixes the `asm_fusexref()` logic for the `IR_STRREF` in
> > case, when both operands don't fit in 32-bit constants (`asm_isk32()`
> > fails). We want to use the IR operand holds the referenced value in
>                                        holding

Fixed, thanks!

> 
> > `ra_alloc1()` as one having the hint set (`ra_hashint()` check passes).
> > It is set for the operand with a non constant value (`irref_isk()`
> > fails). The code assumes that this is always the `ir->op1` operand, so
>                                  it

Fixed.

> 
> > for cases when this value holds `ir->op2` operand register allocator
>     the case                                      the

Fixed, thanks!

Branch is force-pushed.

> 
> > misses the aforementioned hint in `ir->op2`. As the result the wrong
> > register is selected. This patch adds the corresponding `irref_isk()`
> > check for the `ir->op1` to detect which operand contains the value with
> > the hint.
> > 
> > After the patch the resulting assembly is the following:
> > | ...
> > | add   x4, x0, x4
> > | ldrb  w4, [x4, #24]
> > | add   x0, x1, #112
> > | ...
> > 
> > As we can see the constant is rematerialized from another, non-modified
> > register.
> > 
> > Sergey Kaplun:
> > * added the description and the test for the problem
> > 
> > Part of tarantool/tarantool#7230
> > ---
> > 
> > The test case leads to the coredump when compile with
> > -DCMAKE_BUILD_TYPE=[Release, RelWithDebInfo].
> > 
> > Issue: https://github.com/tarantool/tarantool/issues/7230
> > PRs:
> > * https://github.com/LuaJIT/LuaJIT/pull/438
> > * https://github.com/LuaJIT/LuaJIT/pull/479
> > Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-438-arm64-constant-rematerialization-full-ci
> > Tarantool PR: https://github.com/tarantool/tarantool/pull/7628
> > 
> > src/lj_asm_arm64.h                            |  46 +++++---
> > ...-arm64-constant-rematerialization.test.lua | 102 ++++++++++++++++++
> > 2 files changed, 131 insertions(+), 17 deletions(-)
> > create mode 100644 test/tarantool-tests/lj-438-arm64-constant-rematerialization.test.lua
> > 
> > diff --git a/src/lj_asm_arm64.h b/src/lj_asm_arm64.h
> > index da0ee4bb..a4de187f 100644
> > --- a/src/lj_asm_arm64.h
> > +++ b/src/lj_asm_arm64.h

<snipped>

> > diff --git a/test/tarantool-tests/lj-438-arm64-constant-rematerialization.test.lua b/test/tarantool-tests/lj-438-arm64-constant-rematerialization.test.lua
> > new file mode 100644
> > index 00000000..ffc449bc
> > --- /dev/null
> > +++ b/test/tarantool-tests/lj-438-arm64-constant-rematerialization.test.lua
> > @@ -0,0 +1,102 @@

<snipped>

> 

-- 
Best regards,
Sergey Kaplun