Skip to content

Commit 42f2899

Browse files
tekknolagitekknolagi
committed
ZJIT: Support negative array indices
Previously we would side-exit if the index was negative. Instead, adjust the index to be in-bounds.
1 parent fda7e67 commit 42f2899

4 files changed

Lines changed: 109 additions & 54 deletions

File tree

‎zjit/src/codegen.rs‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -561,6 +561,7 @@ fn gen_insn(cb: &mut CodeBlock, jit: &mut JITState, asm: &mut Assembler, functio
561561
Insn::NewRange { low, high, flag, state } => gen_new_range(jit, asm, opnd!(low), opnd!(high), *flag, &function.frame_state(*state)),
562562
Insn::NewRangeFixnum { low, high, flag, state } => gen_new_range_fixnum(asm, opnd!(low), opnd!(high), *flag, &function.frame_state(*state)),
563563
Insn::ArrayDup { val, state } => gen_array_dup(asm, opnd!(val), &function.frame_state(*state)),
564+
Insn::AdjustBounds { index, length } => gen_adjust_bounds(asm, opnd!(index), opnd!(length)),
564565
Insn::ArrayAref { array, index, .. } => gen_array_aref(asm, opnd!(array), opnd!(index)),
565566
Insn::ArrayAset { array, index, val } => {
566567
no_output!(gen_array_aset(asm, opnd!(array), opnd!(index), opnd!(val)))
@@ -1791,6 +1792,14 @@ fn gen_new_array(
17911792
}
17921793
}
17931794

1795+
/// Adjust potentially-negative index by the given length, returning the adjusted index. If still negative,
1796+
/// return a negative number, which indicates the index is still out-of-bounds.
1797+
fn gen_adjust_bounds(asm: &mut Assembler, index: Opnd, length: Opnd) -> lir::Opnd {
1798+
let adjusted = asm.add(index, length);
1799+
asm.test(index, index);
1800+
asm.csel_l(adjusted, index)
1801+
}
1802+
17941803
/// Compile array access (`array[index]`)
17951804
fn gen_array_aref(
17961805
asm: &mut Assembler,

‎zjit/src/cruby_methods.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -354,6 +354,7 @@ fn inline_array_aref(fun: &mut hir::Function, block: hir::BlockId, recv: hir::In
354354
let index = fun.push_insn(block, hir::Insn::UnboxFixnum { val: index });
355355
let length = fun.push_insn(block, hir::Insn::ArrayLength { array: recv });
356356
let index = fun.push_insn(block, hir::Insn::GuardLess { left: index, right: length, state });
357+
let index = fun.push_insn(block, hir::Insn::AdjustBounds { index, length });
357358
let zero = fun.push_insn(block, hir::Insn::Const { val: hir::Const::CInt64(0) });
358359
use crate::hir::SideExitReason;
359360
let index = fun.push_insn(block, hir::Insn::GuardGreaterEq { left: index, right: zero, reason: SideExitReason::GuardGreaterEq, state });

‎zjit/src/hir.rs‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -806,6 +806,9 @@ pub enum Insn {
806806
ArrayPop { array: InsnId, state: InsnId },
807807
/// Return the length of the array as a C `long` ([`types::CInt64`])
808808
ArrayLength { array: InsnId },
809+
/// Adjust potentially-negative index by the given length, returning the adjusted index. If
810+
/// still negative, return a negative number, which indicates the index is still out-of-bounds.
811+
AdjustBounds { index: InsnId, length: InsnId },
809812

810813
HashAref { hash: InsnId, key: InsnId, state: InsnId },
811814
HashAset { hash: InsnId, key: InsnId, val: InsnId, state: InsnId },
@@ -1270,6 +1273,10 @@ macro_rules! for_each_operand_impl {
12701273
Insn::ArrayLength { array } => {
12711274
$visit_one!(array);
12721275
}
1276+
Insn::AdjustBounds { index, length } => {
1277+
$visit_one!(index);
1278+
$visit_one!(length);
1279+
}
12731280
Insn::HashAref { hash, key, state } => {
12741281
$visit_one!(hash);
12751282
$visit_one!(key);
@@ -1472,6 +1479,7 @@ impl Insn {
14721479
Insn::ArrayAset { .. } => effects::Any,
14731480
Insn::ArrayPop { .. } => effects::Any,
14741481
Insn::ArrayLength { .. } => Effect::write(abstract_heaps::Empty),
1482+
Insn::AdjustBounds { .. } => effects::Empty,
14751483
Insn::HashAref { .. } => effects::Any,
14761484
Insn::HashAset { .. } => effects::Any,
14771485
Insn::HashDup { .. } => allocates,
@@ -1713,6 +1721,9 @@ impl<'a> std::fmt::Display for InsnPrinter<'a> {
17131721
Insn::ArrayLength { array } => {
17141722
write!(f, "ArrayLength {array}")
17151723
}
1724+
Insn::AdjustBounds { index, length } => {
1725+
write!(f, "AdjustBounds {index}, {length}")
1726+
}
17161727
Insn::NewHash { elements, .. } => {
17171728
write!(f, "NewHash")?;
17181729
let mut prefix = " ";
@@ -2806,6 +2817,7 @@ impl Function {
28062817
&ArrayAset { array, index, val } => ArrayAset { array: find!(array), index: find!(index), val: find!(val) },
28072818
&ArrayPop { array, state } => ArrayPop { array: find!(array), state: find!(state) },
28082819
&ArrayLength { array } => ArrayLength { array: find!(array) },
2820+
&AdjustBounds { index, length } => AdjustBounds { index: find!(index), length: find!(length) },
28092821
&ArrayMax { ref elements, state } => ArrayMax { elements: find_vec!(elements), state: find!(state) },
28102822
&ArrayInclude { ref elements, target, state } => ArrayInclude { elements: find_vec!(elements), target: find!(target), state: find!(state) },
28112823
&ArrayPackBuffer { ref elements, fmt, buffer, state } => ArrayPackBuffer { elements: find_vec!(elements), fmt: find!(fmt), buffer: find!(buffer), state: find!(state) },
@@ -2923,6 +2935,7 @@ impl Function {
29232935
Insn::ArrayAref { .. } => types::BasicObject,
29242936
Insn::ArrayPop { .. } => types::BasicObject,
29252937
Insn::ArrayLength { .. } => types::CInt64,
2938+
Insn::AdjustBounds { .. } => types::CInt64,
29262939
Insn::HashAref { .. } => types::BasicObject,
29272940
Insn::NewHash { .. } => types::HashExact,
29282941
Insn::HashDup { .. } => types::HashExact,
@@ -5281,6 +5294,16 @@ impl Function {
52815294
_ => insn_id,
52825295
}
52835296
}
5297+
Insn::AdjustBounds { index, .. } => {
5298+
// If index is known nonnegative, then we don't need to adjust bounds.
5299+
if self.type_of(index).cint64_value().filter(|&i| i >= 0).is_some() {
5300+
self.make_equal_to(insn_id, index);
5301+
// Don't bother re-inferring the type of index; we already know it.
5302+
continue;
5303+
} else {
5304+
insn_id
5305+
}
5306+
}
52845307
Insn::Test { val } if self.type_of(val).is_known_falsy() => {
52855308
self.new_insn(Insn::Const { val: Const::CBool(false) })
52865309
}
@@ -6063,6 +6086,10 @@ impl Function {
60636086
self.assert_subtype(insn_id, array, types::ArrayExact)?;
60646087
self.assert_subtype(insn_id, index, types::CInt64)
60656088
}
6089+
Insn::AdjustBounds { index, length } => {
6090+
self.assert_subtype(insn_id, index, types::CInt64)?;
6091+
self.assert_subtype(insn_id, length, types::CInt64)
6092+
}
60666093
// Instructions with Hash operands
60676094
Insn::HashAref { hash, .. }
60686095
| Insn::HashAset { hash, .. } => self.assert_subtype(insn_id, hash, types::HashExact),

‎zjit/src/hir/opt_tests.rs‎

Lines changed: 72 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -1014,12 +1014,12 @@ mod hir_opt_tests {
10141014
PatchPoint NoSingletonClass(Array@0x1008)
10151015
PatchPoint MethodRedefined(Array@0x1008, []@0x1010, cme:0x1018)
10161016
v27:ArrayExact = GuardType v10, ArrayExact
1017-
v34:CInt64[0] = Const CInt64(0)
1017+
v35:CInt64[0] = Const CInt64(0)
10181018
v29:CInt64 = ArrayLength v27
1019-
v30:CInt64[0] = GuardLess v34, v29
1020-
v33:BasicObject = ArrayAref v27, v30
1019+
v30:CInt64[0] = GuardLess v35, v29
1020+
v34:BasicObject = ArrayAref v27, v30
10211021
CheckInterrupts
1022-
Return v33
1022+
Return v34
10231023
");
10241024
}
10251025

@@ -1047,12 +1047,12 @@ mod hir_opt_tests {
10471047
PatchPoint NoSingletonClass(Array@0x1008)
10481048
PatchPoint MethodRedefined(Array@0x1008, []@0x1010, cme:0x1018)
10491049
v27:ArrayExact = GuardType v10, ArrayExact
1050-
v34:CInt64[0] = Const CInt64(0)
1050+
v35:CInt64[0] = Const CInt64(0)
10511051
v29:CInt64 = ArrayLength v27
1052-
v30:CInt64[0] = GuardLess v34, v29
1053-
v33:BasicObject = ArrayAref v27, v30
1052+
v30:CInt64[0] = GuardLess v35, v29
1053+
v34:BasicObject = ArrayAref v27, v30
10541054
CheckInterrupts
1055-
Return v33
1055+
Return v34
10561056
");
10571057
}
10581058

@@ -1077,10 +1077,15 @@ mod hir_opt_tests {
10771077
v13:Fixnum[-10] = Const Value(-10)
10781078
PatchPoint NoSingletonClass(Array@0x1008)
10791079
PatchPoint MethodRedefined(Array@0x1008, []@0x1010, cme:0x1018)
1080-
v31:CInt64[-10] = Const CInt64(-10)
1080+
v32:CInt64[-10] = Const CInt64(-10)
10811081
v26:CInt64 = ArrayLength v11
1082-
v27:CInt64[-10] = GuardLess v31, v26
1083-
SideExit GuardGreaterEq
1082+
v27:CInt64[-10] = GuardLess v32, v26
1083+
v28:CInt64 = AdjustBounds v27, v26
1084+
v29:CInt64[0] = Const CInt64(0)
1085+
v30:CInt64 = GuardGreaterEq v28, v29
1086+
v31:BasicObject = ArrayAref v11, v30
1087+
CheckInterrupts
1088+
Return v31
10841089
");
10851090
}
10861091

@@ -2343,12 +2348,12 @@ mod hir_opt_tests {
23432348
PatchPoint NoSingletonClass(Array@0x1008)
23442349
PatchPoint MethodRedefined(Array@0x1008, []@0x1010, cme:0x1018)
23452350
v27:ArrayExact = GuardType v10, ArrayExact
2346-
v34:CInt64[0] = Const CInt64(0)
2351+
v35:CInt64[0] = Const CInt64(0)
23472352
v29:CInt64 = ArrayLength v27
2348-
v30:CInt64[0] = GuardLess v34, v29
2349-
v33:BasicObject = ArrayAref v27, v30
2353+
v30:CInt64[0] = GuardLess v35, v29
2354+
v34:BasicObject = ArrayAref v27, v30
23502355
CheckInterrupts
2351-
Return v33
2356+
Return v34
23522357
");
23532358
assert_snapshot!(inspect("test [1,2,3]"), @"1");
23542359
}
@@ -6055,12 +6060,12 @@ mod hir_opt_tests {
60556060
v12:Fixnum[0] = Const Value(0)
60566061
PatchPoint NoSingletonClass(Array@0x1010)
60576062
PatchPoint MethodRedefined(Array@0x1010, []@0x1018, cme:0x1020)
6058-
v33:CInt64[0] = Const CInt64(0)
6063+
v34:CInt64[0] = Const CInt64(0)
60596064
v28:CInt64 = ArrayLength v23
6060-
v29:CInt64[0] = GuardLess v33, v28
6061-
v32:BasicObject = ArrayAref v23, v29
6065+
v29:CInt64[0] = GuardLess v34, v28
6066+
v33:BasicObject = ArrayAref v23, v29
60626067
CheckInterrupts
6063-
Return v32
6068+
Return v33
60646069
");
60656070
// TODO(max): Check the result of `S[0] = 5; test` using `inspect` to make sure that we
60666071
// actually do the load at run-time.
@@ -6087,12 +6092,12 @@ mod hir_opt_tests {
60876092
v13:Fixnum[1] = Const Value(1)
60886093
PatchPoint NoSingletonClass(Array@0x1008)
60896094
PatchPoint MethodRedefined(Array@0x1008, []@0x1010, cme:0x1018)
6090-
v31:CInt64[1] = Const CInt64(1)
6095+
v32:CInt64[1] = Const CInt64(1)
60916096
v26:CInt64 = ArrayLength v11
6092-
v27:CInt64[1] = GuardLess v31, v26
6093-
v32:Fixnum[5] = Const Value(5)
6097+
v27:CInt64[1] = GuardLess v32, v26
6098+
v33:Fixnum[5] = Const Value(5)
60946099
CheckInterrupts
6095-
Return v32
6100+
Return v33
60966101
");
60976102
}
60986103

@@ -6117,10 +6122,15 @@ mod hir_opt_tests {
61176122
v13:Fixnum[-3] = Const Value(-3)
61186123
PatchPoint NoSingletonClass(Array@0x1008)
61196124
PatchPoint MethodRedefined(Array@0x1008, []@0x1010, cme:0x1018)
6120-
v31:CInt64[-3] = Const CInt64(-3)
6125+
v32:CInt64[-3] = Const CInt64(-3)
61216126
v26:CInt64 = ArrayLength v11
6122-
v27:CInt64[-3] = GuardLess v31, v26
6123-
SideExit GuardGreaterEq
6127+
v27:CInt64[-3] = GuardLess v32, v26
6128+
v28:CInt64 = AdjustBounds v27, v26
6129+
v29:CInt64[0] = Const CInt64(0)
6130+
v30:CInt64 = GuardGreaterEq v28, v29
6131+
v31:BasicObject = ArrayAref v11, v30
6132+
CheckInterrupts
6133+
Return v31
61246134
");
61256135
}
61266136

@@ -6145,10 +6155,15 @@ mod hir_opt_tests {
61456155
v13:Fixnum[-10] = Const Value(-10)
61466156
PatchPoint NoSingletonClass(Array@0x1008)
61476157
PatchPoint MethodRedefined(Array@0x1008, []@0x1010, cme:0x1018)
6148-
v31:CInt64[-10] = Const CInt64(-10)
6158+
v32:CInt64[-10] = Const CInt64(-10)
61496159
v26:CInt64 = ArrayLength v11
6150-
v27:CInt64[-10] = GuardLess v31, v26
6151-
SideExit GuardGreaterEq
6160+
v27:CInt64[-10] = GuardLess v32, v26
6161+
v28:CInt64 = AdjustBounds v27, v26
6162+
v29:CInt64[0] = Const CInt64(0)
6163+
v30:CInt64 = GuardGreaterEq v28, v29
6164+
v31:BasicObject = ArrayAref v11, v30
6165+
CheckInterrupts
6166+
Return v31
61526167
");
61536168
}
61546169

@@ -6173,12 +6188,12 @@ mod hir_opt_tests {
61736188
v13:Fixnum[10] = Const Value(10)
61746189
PatchPoint NoSingletonClass(Array@0x1008)
61756190
PatchPoint MethodRedefined(Array@0x1008, []@0x1010, cme:0x1018)
6176-
v31:CInt64[10] = Const CInt64(10)
6191+
v32:CInt64[10] = Const CInt64(10)
61776192
v26:CInt64 = ArrayLength v11
6178-
v27:CInt64[10] = GuardLess v31, v26
6179-
v32:NilClass = Const Value(nil)
6193+
v27:CInt64[10] = GuardLess v32, v26
6194+
v33:NilClass = Const Value(nil)
61806195
CheckInterrupts
6181-
Return v32
6196+
Return v33
61826197
");
61836198
}
61846199

@@ -8665,12 +8680,12 @@ mod hir_opt_tests {
86658680
v19:Fixnum[0] = Const Value(0)
86668681
PatchPoint NoSingletonClass(Array@0x1008)
86678682
PatchPoint MethodRedefined(Array@0x1008, []@0x1010, cme:0x1018)
8668-
v37:CInt64[0] = Const CInt64(0)
8683+
v38:CInt64[0] = Const CInt64(0)
86698684
v32:CInt64 = ArrayLength v14
8670-
v33:CInt64[0] = GuardLess v37, v32
8671-
v36:BasicObject = ArrayAref v14, v33
8685+
v33:CInt64[0] = GuardLess v38, v32
8686+
v37:BasicObject = ArrayAref v14, v33
86728687
CheckInterrupts
8673-
Return v36
8688+
Return v37
86748689
");
86758690
}
86768691

@@ -8705,11 +8720,12 @@ mod hir_opt_tests {
87058720
v31:CInt64 = UnboxFixnum v30
87068721
v32:CInt64 = ArrayLength v29
87078722
v33:CInt64 = GuardLess v31, v32
8708-
v34:CInt64[0] = Const CInt64(0)
8709-
v35:CInt64 = GuardGreaterEq v33, v34
8710-
v36:BasicObject = ArrayAref v29, v35
8723+
v34:CInt64 = AdjustBounds v33, v32
8724+
v35:CInt64[0] = Const CInt64(0)
8725+
v36:CInt64 = GuardGreaterEq v34, v35
8726+
v37:BasicObject = ArrayAref v29, v36
87118727
CheckInterrupts
8712-
Return v36
8728+
Return v37
87138729
");
87148730
}
87158731

@@ -8745,11 +8761,12 @@ mod hir_opt_tests {
87458761
v31:CInt64 = UnboxFixnum v30
87468762
v32:CInt64 = ArrayLength v29
87478763
v33:CInt64 = GuardLess v31, v32
8748-
v34:CInt64[0] = Const CInt64(0)
8749-
v35:CInt64 = GuardGreaterEq v33, v34
8750-
v36:BasicObject = ArrayAref v29, v35
8764+
v34:CInt64 = AdjustBounds v33, v32
8765+
v35:CInt64[0] = Const CInt64(0)
8766+
v36:CInt64 = GuardGreaterEq v34, v35
8767+
v37:BasicObject = ArrayAref v29, v36
87518768
CheckInterrupts
8752-
Return v36
8769+
Return v37
87538770
");
87548771
}
87558772

@@ -9340,21 +9357,22 @@ mod hir_opt_tests {
93409357
v25:CallableMethodEntry[VALUE(0x1048)] = GuardBitEquals v24, Value(VALUE(0x1048))
93419358
v26:RubyValue = LoadField v23, :_ep_specval@0x1050
93429359
v27:FalseClass = GuardBitEquals v26, Value(false)
9343-
v37:CPtr = GetEP 0
9344-
v38:RubyValue = LoadField v37, :_ep_method_entry@0x1040
9345-
v39:CallableMethodEntry[VALUE(0x1048)] = GuardBitEquals v38, Value(VALUE(0x1048))
9346-
v40:RubyValue = LoadField v37, :_ep_specval@0x1050
9347-
v41:FalseClass = GuardBitEquals v40, Value(false)
9360+
v38:CPtr = GetEP 0
9361+
v39:RubyValue = LoadField v38, :_ep_method_entry@0x1040
9362+
v40:CallableMethodEntry[VALUE(0x1048)] = GuardBitEquals v39, Value(VALUE(0x1048))
9363+
v41:RubyValue = LoadField v38, :_ep_specval@0x1050
9364+
v42:FalseClass = GuardBitEquals v41, Value(false)
93489365
v28:Array = GuardType v9, Array
93499366
v29:Fixnum = GuardType v10, Fixnum
93509367
v30:CInt64 = UnboxFixnum v29
93519368
v31:CInt64 = ArrayLength v28
93529369
v32:CInt64 = GuardLess v30, v31
9353-
v33:CInt64[0] = Const CInt64(0)
9354-
v34:CInt64 = GuardGreaterEq v32, v33
9355-
v35:BasicObject = ArrayAref v28, v34
9370+
v33:CInt64 = AdjustBounds v32, v31
9371+
v34:CInt64[0] = Const CInt64(0)
9372+
v35:CInt64 = GuardGreaterEq v33, v34
9373+
v36:BasicObject = ArrayAref v28, v35
93569374
CheckInterrupts
9357-
Return v35
9375+
Return v36
93589376
");
93599377
}
93609378

0 commit comments

Comments
 (0)