Skip to content

Commit 304d37f

Browse files
authored
ZJIT: Fix hanging loop (ruby#16711)
ruby#16122 (c272297) worked for maximal SSA but does not work for "normal" SSA. This is because it used information propagating across block args/params as a proxy for tracking changes in dependent blocks. To do this properly we need to move to something like SCCP. See example HIR diff from the repro in codegen test: ```diff diff --git a/before b/after index da00a9e..b1d2a58 100644 --- a/before +++ b/after @@ -64,10 +64,10 @@ bb7(v48:BasicObject): StoreField v46, :_shape_id@0x4, v105 v79:HeapBasicObject = RefineType v46, HeapBasicObject Jump bb5(v79, v41) -bb5(v81:HeapBasicObject, v82:Fixnum[0]): +bb5(v81:HeapBasicObject, v82:Fixnum): PatchPoint NoEPEscape(set_value_loop) v89:Fixnum[1] = Const Value(1) PatchPoint MethodRedefined(Integer@ADDR, +@0x2b, cme:ADDR) - v114:Fixnum[1] = Const Value(1) + v114:Fixnum = FixnumAdd v82, v89 Jump bb6(v81, v114) ``` (the `Fixnum[0]` is from type inference and the `Fixnum[1]` is from constant folding having folded the `FixnumAdd`) In the meantime, go back to looping over RPO repeatedly. Fix Shopify#974
1 parent 573b16a commit 304d37f

3 files changed

Lines changed: 166 additions & 67 deletions

File tree

zjit/src/codegen_tests.rs

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5610,3 +5610,25 @@ fn test_load_immediates_into_registers_before_masking() {
56105610
test
56115611
"#), @"true");
56125612
}
5613+
5614+
#[test]
5615+
fn test_loop_terminates() {
5616+
set_call_threshold(3);
5617+
// Previous worklist-based type inference only worked for maximal SSA. This is a regression
5618+
// test for hanging.
5619+
assert_snapshot!(inspect(r#"
5620+
class TheClass
5621+
def set_value_loop
5622+
i = 0
5623+
while i < 10
5624+
@levar ||= i
5625+
i += 1
5626+
end
5627+
end
5628+
end
5629+
5630+
3.times do |i|
5631+
TheClass.new.set_value_loop
5632+
end
5633+
"#), @"3");
5634+
}

zjit/src/hir.rs

Lines changed: 51 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -3204,82 +3204,66 @@ impl Function {
32043204
self.copy_param_types();
32053205

32063206
let mut reachable = BlockSet::with_capacity(self.blocks.len());
3207-
3208-
// Maintain both a worklist and a fast membership check to avoid linear search
3209-
let mut worklist: VecDeque<BlockId> = VecDeque::with_capacity(self.blocks.len());
3210-
let mut in_worklist = BlockSet::with_capacity(self.blocks.len());
3211-
macro_rules! worklist_add {
3212-
($block:expr) => {
3213-
if in_worklist.insert($block) {
3214-
worklist.push_back($block);
3215-
}
3216-
};
3217-
}
3218-
32193207
reachable.insert(self.entries_block);
3220-
worklist_add!(self.entries_block);
3221-
3222-
// Helper to propagate types along a branch edge and enqueue the target if anything changed
3223-
macro_rules! enqueue {
3224-
($self:ident, $target:expr) => {
3225-
let newly_reachable = reachable.insert($target.target);
3226-
let mut target_changed = newly_reachable;
3227-
for (idx, arg) in $target.args.iter().enumerate() {
3228-
let arg = self.union_find.borrow().find_const(*arg);
3229-
let param = $self.blocks[$target.target.0].params[idx];
3230-
let param = self.union_find.borrow().find_const(param);
3231-
let new = self.insn_types[param.0].union(self.insn_types[arg.0]);
3232-
if !self.insn_types[param.0].bit_equal(new) {
3233-
self.insn_types[param.0] = new;
3234-
target_changed = true;
3235-
}
3236-
}
3237-
if target_changed {
3238-
worklist_add!($target.target);
3239-
}
3240-
};
3241-
}
32423208

3243-
// Walk the graph, computing types until worklist is empty
3244-
while let Some(block) = worklist.pop_front() {
3245-
in_worklist.remove(block);
3246-
if !reachable.get(block) { continue; }
3247-
for insn_id in &self.blocks[block.0].insns {
3248-
let insn_id = self.union_find.borrow().find_const(*insn_id);
3249-
let insn_type = match &self.insns[insn_id.0] {
3250-
&Insn::IfTrue { val, ref target } => {
3251-
assert!(!self.type_of(val).bit_equal(types::Empty));
3252-
if self.type_of(val).could_be(Type::from_cbool(true)) {
3253-
enqueue!(self, target);
3209+
// Walk the graph, computing types until fixpoint
3210+
let rpo = self.rpo();
3211+
loop {
3212+
let mut changed = false;
3213+
for &block in &rpo {
3214+
if !reachable.get(block) { continue; }
3215+
for &insn_id in &self.blocks[block.0].insns {
3216+
// Instructions without output, including branch instructions, can't be targets
3217+
// of make_equal_to, so we don't need find() here.
3218+
let insn_type = match &self.insns[insn_id.0] {
3219+
&Insn::IfTrue { val, target: BranchEdge { target, ref args } } => {
3220+
assert!(!self.type_of(val).bit_equal(types::Empty));
3221+
if self.type_of(val).could_be(Type::from_cbool(true)) {
3222+
reachable.insert(target);
3223+
for (idx, arg) in args.iter().enumerate() {
3224+
let param = self.blocks[target.0].params[idx];
3225+
self.insn_types[param.0] = self.type_of(param).union(self.type_of(*arg));
3226+
}
3227+
}
3228+
continue;
32543229
}
3255-
continue;
3256-
}
3257-
&Insn::IfFalse { val, ref target } => {
3258-
assert!(!self.type_of(val).bit_equal(types::Empty));
3259-
if self.type_of(val).could_be(Type::from_cbool(false)) {
3260-
enqueue!(self, target);
3230+
&Insn::IfFalse { val, target: BranchEdge { target, ref args } } => {
3231+
assert!(!self.type_of(val).bit_equal(types::Empty));
3232+
if self.type_of(val).could_be(Type::from_cbool(false)) {
3233+
reachable.insert(target);
3234+
for (idx, arg) in args.iter().enumerate() {
3235+
let param = self.blocks[target.0].params[idx];
3236+
self.insn_types[param.0] = self.type_of(param).union(self.type_of(*arg));
3237+
}
3238+
}
3239+
continue;
32613240
}
3262-
continue;
3263-
}
3264-
&Insn::Jump(ref target) => {
3265-
enqueue!(self, target);
3266-
continue;
3267-
}
3268-
&Insn::Entries { ref targets } => {
3269-
for target in targets {
3270-
if reachable.insert(*target) {
3271-
worklist_add!(*target);
3241+
&Insn::Jump(BranchEdge { target, ref args }) => {
3242+
reachable.insert(target);
3243+
for (idx, arg) in args.iter().enumerate() {
3244+
let param = self.blocks[target.0].params[idx];
3245+
self.insn_types[param.0] = self.type_of(param).union(self.type_of(*arg));
32723246
}
3247+
continue;
32733248
}
3274-
continue;
3249+
Insn::Entries { targets } => {
3250+
for &target in targets {
3251+
reachable.insert(target);
3252+
}
3253+
continue;
3254+
}
3255+
insn if insn.has_output() => self.infer_type(insn_id),
3256+
_ => continue,
3257+
};
3258+
if !self.type_of(insn_id).bit_equal(insn_type) {
3259+
self.insn_types[insn_id.0] = insn_type;
3260+
changed = true;
32753261
}
3276-
insn if insn.has_output() => self.infer_type(insn_id),
3277-
_ => continue,
3278-
};
3279-
if !self.type_of(insn_id).bit_equal(insn_type) {
3280-
self.insn_types[insn_id.0] = insn_type;
32813262
}
32823263
}
3264+
if !changed {
3265+
break;
3266+
}
32833267
}
32843268
}
32853269

zjit/src/hir/opt_tests.rs

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15544,4 +15544,97 @@ mod hir_opt_tests {
1554415544
Return v43
1554515545
");
1554615546
}
15547+
15548+
#[test]
15549+
fn test_infer_types_across_non_maximal_basic_blocks() {
15550+
// Previous worklist-based type inference only worked for maximal SSA. This is a regression
15551+
// test for hanging.
15552+
eval("
15553+
class TheClass
15554+
def set_value_loop
15555+
i = 0
15556+
while i < 10
15557+
@levar ||= i
15558+
i += 1
15559+
end
15560+
end
15561+
end
15562+
3.times do |i|
15563+
TheClass.new.set_value_loop
15564+
end
15565+
");
15566+
assert_snapshot!(hir_string_proc("TheClass.instance_method(:set_value_loop)"), @"
15567+
fn set_value_loop@<compiled>:4:
15568+
bb1():
15569+
EntryPoint interpreter
15570+
v1:BasicObject = LoadSelf
15571+
v2:NilClass = Const Value(nil)
15572+
Jump bb3(v1, v2)
15573+
bb2():
15574+
EntryPoint JIT(0)
15575+
v5:BasicObject = LoadArg :self@0
15576+
v6:NilClass = Const Value(nil)
15577+
Jump bb3(v5, v6)
15578+
bb3(v8:BasicObject, v9:NilClass):
15579+
v13:Fixnum[0] = Const Value(0)
15580+
CheckInterrupts
15581+
Jump bb6(v8, v13)
15582+
bb6(v19:BasicObject, v20:Fixnum):
15583+
v24:Fixnum[10] = Const Value(10)
15584+
PatchPoint MethodRedefined(Integer@0x1000, <@0x1008, cme:0x1010)
15585+
v110:BoolExact = FixnumLt v20, v24
15586+
CheckInterrupts
15587+
v30:CBool = Test v110
15588+
IfTrue v30, bb4(v19, v20)
15589+
v35:NilClass = Const Value(nil)
15590+
CheckInterrupts
15591+
Return v35
15592+
bb4(v40:BasicObject, v41:Fixnum):
15593+
PatchPoint SingleRactorMode
15594+
v46:HeapBasicObject = GuardType v40, HeapBasicObject
15595+
v47:CUInt64 = LoadField v46, :_rbasic_flags@0x1038
15596+
v49:CUInt64[0xffffffff0000001f] = Const CUInt64(0xffffffff0000001f)
15597+
v50:CPtr[CPtr(0x1039)] = Const CPtr(0x1039)
15598+
v51 = RefineType v50, CUInt64
15599+
v52:CInt64 = IntAnd v47, v49
15600+
v53:CBool = IsBitEqual v52, v51
15601+
IfTrue v53, bb8()
15602+
v57:CUInt64[0xffffffff0000001f] = Const CUInt64(0xffffffff0000001f)
15603+
v58:CPtr[CPtr(0x103a)] = Const CPtr(0x103a)
15604+
v59 = RefineType v58, CUInt64
15605+
v60:CInt64 = IntAnd v47, v57
15606+
v61:CBool = IsBitEqual v60, v59
15607+
IfTrue v61, bb9()
15608+
v97:CShape = LoadField v46, :_shape_id@0x103b
15609+
v98:CShape[0x103c] = GuardBitEquals v97, CShape(0x103c)
15610+
v99:BasicObject = LoadField v46, :@levar@0x103d
15611+
Jump bb7(v99)
15612+
bb8():
15613+
v55:BasicObject = LoadField v46, :@levar@0x103d
15614+
Jump bb7(v55)
15615+
bb9():
15616+
v63:NilClass = Const Value(nil)
15617+
Jump bb7(v63)
15618+
bb7(v48:BasicObject):
15619+
CheckInterrupts
15620+
v69:CBool = Test v48
15621+
IfTrue v69, bb5(v46, v41)
15622+
PatchPoint NoEPEscape(set_value_loop)
15623+
PatchPoint SingleRactorMode
15624+
v101:CShape = LoadField v46, :_shape_id@0x103b
15625+
v102:CShape[0x103e] = GuardBitEquals v101, CShape(0x103e)
15626+
StoreField v46, :@levar@0x103d, v41
15627+
WriteBarrier v46, v41
15628+
v105:CShape[0x103c] = Const CShape(0x103c)
15629+
StoreField v46, :_shape_id@0x103b, v105
15630+
v79:HeapBasicObject = RefineType v46, HeapBasicObject
15631+
Jump bb5(v79, v41)
15632+
bb5(v81:HeapBasicObject, v82:Fixnum):
15633+
PatchPoint NoEPEscape(set_value_loop)
15634+
v89:Fixnum[1] = Const Value(1)
15635+
PatchPoint MethodRedefined(Integer@0x1000, +@0x103f, cme:0x1040)
15636+
v114:Fixnum = FixnumAdd v82, v89
15637+
Jump bb6(v81, v114)
15638+
");
15639+
}
1554715640
}

0 commit comments

Comments
 (0)