Thanks! LGTM
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>