Skip to content

Commit 6a4eb74

Browse files
Mike PallBuristan
authored andcommitted
ARM64: Fix code generation for IR_SLOAD with typecheck + conversion.
Reported by memcorrupt. (cherry picked from commit 564147f) The assembling of the SLOAD with typecheck and conversion from number to int misses the corresponding move for emitting conversion to the FPR during assembling. Consider the following SLOAD: | 0006 x28 > int SLOAD #4 TCI Which results in the following mcode before the patch: | ldr x28, [x3, LuaJIT#16] | cmp x2, x28, lsr LuaJIT#32 | bls 0x62d2fda0 ->0 | ; here missing the move to d31 | fcvtzs w28, d31 | scvtf d30, w28 | fcmp d30, d31 | bne 0x62d2fda0 ->0 Instead of the expected: | ldr x28, [x3, LuaJIT#16] | cmp x2, x28, lsr LuaJIT#32 | bls 0x7bacfda0 ->0 | fmov d31, x28 | fcvtzs w28, d31 | scvtf d30, w28 | fcmp d30, d31 | bne 0x7bacfda0 ->0 Due to the incorrect check of the condition inside the `asm_sload()`, which excluded the `IRSLOAD_CONVERT` flag. It may lead to inconsistent behaviour on the trace. This patch fixes the check by comparing the source and destination registers instead. Sergey Kaplun: * added the description and the test for the problem Part of tarantool/tarantool#11278 Reviewed-by: Sergey Bronnikov <[email protected]> Signed-off-by: Sergey Kaplun <[email protected]> (cherry picked from commit 2a13d73)
1 parent e182c94 commit 6a4eb74

File tree

2 files changed

+59
-1
lines changed

2 files changed

+59
-1
lines changed

src/lj_asm_arm64.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1177,7 +1177,7 @@ static void asm_sload(ASMState *as, IRIns *ir)
11771177
tmp = ra_scratch(as, allow);
11781178
rset_clear(allow, tmp);
11791179
}
1180-
if (ra_hasreg(dest) && irt_isnum(t) && !(ir->op2 & IRSLOAD_CONVERT))
1180+
if (ra_hasreg(dest) && tmp != dest)
11811181
emit_dn(as, A64I_FMOV_D_R, (dest & 31), tmp);
11821182
/* Need type check, even if the load result is unused. */
11831183
asm_guardcc(as, irt_isnum(t) ? CC_LS : CC_NE);
Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
local tap = require('tap')
2+
-- Test file to demonstrate the incorrect JIT assembling of
3+
-- `IR_SLOAD` with typecheck and conversion to integer from
4+
-- number.
5+
-- See also https://github.com/LuaJIT/LuaJIT/issues/917.
6+
local test = tap.test('lj-917-arm64-sload-typecheck-conversion'):skipcond({
7+
['Test requires JIT enabled'] = not jit.status(),
8+
})
9+
10+
test:plan(1)
11+
12+
jit.opt.start('hotloop=1')
13+
14+
local results = {}
15+
16+
-- Use the following mathematics on a huge number not fitting into
17+
-- an int to be sure that all 3 control numbers (start, stop,
18+
-- step) of the loop should be non-integers to avoid fallback to
19+
-- the `lj_vmeta_for()` and narrowing in the `lj_meta_for()`
20+
-- (see <src/vm_arm64.dasm> for details).
21+
local NOT_INT = 2 ^ 32
22+
23+
-- The interesting for us SLOAD is the loading of the start index:
24+
-- | 0006 x28 > int SLOAD #4 TCI
25+
--
26+
-- Which results in the following mcode before the patch:
27+
-- | ldr x28, [x3, #16]
28+
-- | cmp x2, x28, lsr #32
29+
-- | bls 0x62d2fda0 ->0
30+
-- | ; here missing the move to d31
31+
-- | fcvtzs w28, d31
32+
-- | scvtf d30, w28
33+
-- | fcmp d30, d31
34+
-- | bne 0x62d2fda0 ->0
35+
--
36+
-- Instead of the expected:
37+
-- | ldr x28, [x3, #16]
38+
-- | cmp x2, x28, lsr #32
39+
-- | bls 0x7bacfda0 ->0
40+
-- | fmov d31, x28
41+
-- | fcvtzs w28, d31
42+
-- | scvtf d30, w28
43+
-- | fcmp d30, d31
44+
-- | bne 0x7bacfda0 ->0
45+
46+
-- At this moment d31 contains the value of the `step`, so `step`
47+
-- should be >= `stop` to obtain inconsistency (the too early loop
48+
-- end with the last `i` value equals to `step`).
49+
-- The resulting loop is:
50+
-- | for i = -4, -1, 1 do
51+
for i = -4 + NOT_INT * 0, -1 + NOT_INT * 0, 1 + NOT_INT * 0 do
52+
results[-i] = true
53+
end
54+
55+
-- Expected {true, true, true, true}, since -4 is a start.
56+
test:samevalues(results, 'correct SLOAD TC assembling')
57+
58+
test:done(true)

0 commit comments

Comments
 (0)