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

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Sergey,

I've checked the patchset into all long-term branches in
tarantool/luajit and bumped a new version in master, 2.11 and 2.10.

On 31.08.22, Sergey Kaplun 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
> 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
> 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
> `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
> for cases when this value holds `ir->op2` operand register allocator
> 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
> 

<snipped>

> -- 
> 2.34.1
> 

-- 
Best regards,
IM