Re: [Tarantool-patches] [PATCH luajit] ARM64: Fix LDP/STP fusing for unaligned accesses.

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

Hi, Sergey!
Thanks for the review!
Fixed your comments and force-pushed the branch.

On 27.06.25, Sergey Bronnikov wrote:
> Hi, Sergey,
> 
> thanks for the patch! LGTM with a minor two comments below.
> 
> Sergey
> 
> On 6/26/25 18:12, Sergey Kaplun wrote:
> > From: Mike Pall <mike>
> >
> > Thanks to Peter Cawley.
> >
> > (cherry picked from commit 0fa2f1cbcf023ad0549f1428809e506fa2c78552)
> >
> > The arm64 emitting of load/store operation works incorrectly in the
> > case when at least one offset of load/store to be fused into ldp/stp is
> > misaligned. In this case this misaligning is ignored, and instructions
> > are fused, which leads to loading/storing from/to at least one incorrect
> > address.
> >
> > For example, the following instructions:
> > | stur  w0, [x1, #17]
> > | stur  w0, [x1, #21]
> >
> > May be fused to the following:
> > | stp   w0, w0, [x1, #16]
> >
> > This patch prevents fusion in this case by testing the alignment with
> > the help of bitwise ROR by the alignment value. In case of misaligned
> > offset, the value overflows the 7-bit length mask in the check.
> >
> > The negative immediate (7-bit width including sign bit) is limited by
> > the corresponding addition of `64 << sc` (it is harmless in the case of
> > positive values).
> >
> > Sergey Kaplun:
> > * added the description and the test for the problem
> >
> > Part of tarantool/tarantool#11278
> > ---
> >
> > Related issues:
> > *https://github.com/LuaJIT/LuaJIT/issues/1056
> > *https://github.com/tarantool/tarantool/issues/11278
> > Branch:https://github.com/tarantool/luajit/tree/skaplun/lj-1056-arm64-ldp-sdp-misaligned-fusing
> >
> >   src/lj_emit_arm64.h                           |  2 +-
> >   ...6-arm64-ldp-sdp-misaligned-fusing.test.lua | 98 +++++++++++++++++++
> >   2 files changed, 99 insertions(+), 1 deletion(-)
> >   create mode 100644 test/tarantool-tests/lj-1056-arm64-ldp-sdp-misaligned-fusing.test.lua
> >
> > diff --git a/src/lj_emit_arm64.h b/src/lj_emit_arm64.h
> > index 30cd3505..5c1bc372 100644
> > --- a/src/lj_emit_arm64.h
> > +++ b/src/lj_emit_arm64.h

<snipped>

> > diff --git a/test/tarantool-tests/lj-1056-arm64-ldp-sdp-misaligned-fusing.test.lua b/test/tarantool-tests/lj-1056-arm64-ldp-sdp-misaligned-fusing.test.lua
> > new file mode 100644
> > index 00000000..5d03097e
> > --- /dev/null
> > +++ b/test/tarantool-tests/lj-1056-arm64-ldp-sdp-misaligned-fusing.test.lua
> > @@ -0,0 +1,98 @@
> > +local tap = require('tap')
> > +local ffi = require('ffi')
> > +
> > +-- This test demonstrates LuaJIT's incorrect emitting of LDP/STP
> > +-- instructions from LDUR/STUR instructions with misaligned offset
> > +-- on arm64.
> > +-- See alsohttps://github.com/LuaJIT/LuaJIT/issue/1056.
> s/issue/issues/

Fixed, thanks!

===================================================================
diff --git a/test/tarantool-tests/lj-1056-arm64-ldp-sdp-misaligned-fusing.test.lua b/test/tarantool-tests/lj-1056-arm64-ldp-sdp-misaligned-fusing.test.lua
index 815da15d..5ff040e7 100644
--- a/test/tarantool-tests/lj-1056-arm64-ldp-sdp-misaligned-fusing.test.lua
+++ b/test/tarantool-tests/lj-1056-arm64-ldp-sdp-misaligned-fusing.test.lua
@@ -4,7 +4,7 @@ local ffi = require('ffi')
 -- This test demonstrates LuaJIT's incorrect emitting of LDP/STP
 -- instructions from LDUR/STUR instructions with misaligned offset
 -- on arm64.
--- See also https://github.com/LuaJIT/LuaJIT/issue/1056.
+-- See also https://github.com/LuaJIT/LuaJIT/issues/1056.
 local test = tap.test('lj-1056-arm64-ldp-sdp-misaligned-fusing'):skipcond({
   ['Test requires JIT enabled'] = not jit.status(),
 })
===================================================================

> > +local test = tap.test('lj-1056-arm64-ldp-sdp-misaligned-fusing'):skipcond({
> > +  ['Test requires JIT enabled'] = not jit.status(),
> > +})
> > +

<snipped>

> > +
> > +test:is(resl, 0x4030201, 'pair of misaligned loads, left')
> > +test:is(resr, 0x8070605, 'pair of misaligned loads, right')
> 
> What does mean these magic numbers? Please add a comment or
> 
> use a variable with self-explained name. Here and below.

Added the comments nearby, see the corresponding patch below:

===================================================================
diff --git a/test/tarantool-tests/lj-1056-arm64-ldp-sdp-misaligned-fusing.test.lua b/test/tarantool-tests/lj-1056-arm64-ldp-sdp-misaligned-fusing.test.lua
index 5d03097e..815da15d 100644
--- a/test/tarantool-tests/lj-1056-arm64-ldp-sdp-misaligned-fusing.test.lua
+++ b/test/tarantool-tests/lj-1056-arm64-ldp-sdp-misaligned-fusing.test.lua
@@ -81,6 +81,8 @@ for _ = 1, N_ITERATIONS do
   resr = ffi.cast('int32_t *', ptr + 5)[0]
 end
 
+-- Values are resulted from the `init_buf()` function with the
+-- corresponding offset.
 test:is(resl, 0x4030201, 'pair of misaligned loads, left')
 test:is(resr, 0x8070605, 'pair of misaligned loads, right')
 
@@ -92,6 +94,8 @@ for _ = 1, N_ITERATIONS do
   resl = ffi.cast('int32_t *', ptr)[0]
 end
 
+-- Values are resulted from the `init_buf()` function with the
+-- corresponding offset.
 test:is(resl, 0x3020100, 'aligned / misaligned load, aligned')
 test:is(resr, 0x8070605, 'aligned / misaligned load, misaligned')
 
===================================================================

<snipped>

-- 
Best regards,
Sergey Kaplun