Skip to content

Commit 997ac19

Browse files
rwstaunerrwstauner
authored
ZJIT: Remove redundant PatchPoints within basic blocks (ruby#16231)
Add a new optimization pass that deduplicates PatchPoint instructions asserting the same invariant within a basic block when no intervening instruction could invalidate it. This is common in practice — e.g., `1 + 2 + 3` produces two identical `PatchPoint MethodRedefined(Integer, +, cme)` because each `+` call inserts one. The second is redundant since nothing between them can redefine Integer#+. The approach adds a `PatchPoint` leaf to the abstract heap DAG so the effect system can distinguish instructions that could invalidate invariants (writes to PatchPoint, e.g. Send/CCall) from those that cannot (e.g. IncrCounter, Const). The pass maintains a HashSet of seen invariants per block and skips duplicates, clearing the set when an instruction writes to the PatchPoint heap. Removed PatchPoints' orphaned Snapshot dependencies are cleaned up by the subsequent DCE pass. I added a new test with snapshots to `zjit/src/hir/tests.rs` in the first commit so that you can see how the change affects it in the second commit: ruby@88cf26b#diff-38be021d8d0035cca77d3eab5ccc530109e7832f4711a5beb4dfced7001787ae closes Shopify#779
1 parent 8e9eb69 commit 997ac19

6 files changed

Lines changed: 75 additions & 29 deletions

File tree

‎zjit/src/hir.rs‎

Lines changed: 30 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ impl std::fmt::Display for BranchEdge {
118118
}
119119

120120
/// Invalidation reasons
121-
#[derive(Debug, Clone, Copy)]
121+
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
122122
pub enum Invariant {
123123
/// Basic operation is redefined
124124
BOPRedefined {
@@ -1237,10 +1237,10 @@ impl Insn {
12371237
Insn::GuardNoBitsSet { .. } => effects::Any,
12381238
Insn::GuardGreaterEq { .. } => effects::Any,
12391239
Insn::GuardLess { .. } => effects::Any,
1240-
Insn::PatchPoint { .. } => effects::Any,
1240+
Insn::PatchPoint { .. } => Effect::read_write(abstract_heaps::PatchPoint, abstract_heaps::Control),
12411241
Insn::SideExit { .. } => effects::Any,
1242-
Insn::IncrCounter(_) => effects::Any,
1243-
Insn::IncrCounterPtr { .. } => effects::Any,
1242+
Insn::IncrCounter(_) => Effect::read_write(abstract_heaps::Empty, abstract_heaps::Other),
1243+
Insn::IncrCounterPtr { .. } => Effect::read_write(abstract_heaps::Empty, abstract_heaps::Other),
12441244
Insn::CheckInterrupts { .. } => effects::Any,
12451245
Insn::InvokeProc { .. } => effects::Any,
12461246
Insn::RefineType { .. } => effects::Empty,
@@ -5037,6 +5037,29 @@ impl Function {
50375037
}
50385038
}
50395039

5040+
/// Remove duplicate PatchPoint instructions within each basic block.
5041+
/// Two PatchPoints are redundant if they assert the same Invariant and no
5042+
/// intervening instruction could invalidate it (i.e., writes to PatchPoint).
5043+
fn remove_redundant_patch_points(&mut self) {
5044+
for block_id in self.rpo() {
5045+
let mut seen = HashSet::new();
5046+
let insns = std::mem::take(&mut self.blocks[block_id.0].insns);
5047+
let mut new_insns = Vec::with_capacity(insns.len());
5048+
for insn_id in insns {
5049+
let insn = self.find(insn_id);
5050+
if let Insn::PatchPoint { invariant, .. } = insn {
5051+
if !seen.insert(invariant) {
5052+
continue;
5053+
}
5054+
} else if insn.effects_of().write_bits().overlaps(abstract_heaps::PatchPoint) {
5055+
seen.clear();
5056+
}
5057+
new_insns.push(insn_id);
5058+
}
5059+
self.blocks[block_id.0].insns = new_insns;
5060+
}
5061+
}
5062+
50405063
/// Return a list that has entry_block and then jit_entry_blocks
50415064
fn entry_blocks(&self) -> Vec<BlockId> {
50425065
let mut entry_blocks = self.jit_entry_blocks.clone();
@@ -5260,6 +5283,8 @@ impl Function {
52605283
Counter::compile_hir_fold_constants_time_ns
52615284
} else if ident_equal!($name, clean_cfg) {
52625285
Counter::compile_hir_clean_cfg_time_ns
5286+
} else if ident_equal!($name, remove_redundant_patch_points) {
5287+
Counter::compile_hir_remove_redundant_patch_points_time_ns
52635288
} else if ident_equal!($name, eliminate_dead_code) {
52645289
Counter::compile_hir_eliminate_dead_code_time_ns
52655290
} else {
@@ -5286,6 +5311,7 @@ impl Function {
52865311
run_pass!(optimize_c_calls);
52875312
run_pass!(fold_constants);
52885313
run_pass!(clean_cfg);
5314+
run_pass!(remove_redundant_patch_points);
52895315
run_pass!(eliminate_dead_code);
52905316

52915317
if should_dump {
@@ -8473,8 +8499,6 @@ mod graphviz_tests {
84738499
bb3 [label=<<TABLE BORDER="0" CELLBORDER="1" CELLSPACING="0">
84748500
<TR><TD ALIGN="LEFT" PORT="params" BGCOLOR="gray">bb3(v10:BasicObject, v11:BasicObject, v12:BasicObject)&nbsp;</TD></TR>
84758501
<TR><TD ALIGN="left" PORT="v15">PatchPoint NoTracePoint&nbsp;</TD></TR>
8476-
<TR><TD ALIGN="left" PORT="v18">PatchPoint NoTracePoint&nbsp;</TD></TR>
8477-
<TR><TD ALIGN="left" PORT="v25">PatchPoint NoTracePoint&nbsp;</TD></TR>
84788502
<TR><TD ALIGN="left" PORT="v26">PatchPoint MethodRedefined(Integer@0x1000, |@0x1008, cme:0x1010)&nbsp;</TD></TR>
84798503
<TR><TD ALIGN="left" PORT="v27">v27:Fixnum = GuardType v11, Fixnum&nbsp;</TD></TR>
84808504
<TR><TD ALIGN="left" PORT="v28">v28:Fixnum = GuardType v12, Fixnum&nbsp;</TD></TR>
@@ -8536,7 +8560,6 @@ mod graphviz_tests {
85368560
<TR><TD ALIGN="left" PORT="v18">v18:Truthy = RefineType v9, Truthy&nbsp;</TD></TR>
85378561
<TR><TD ALIGN="left" PORT="v20">PatchPoint NoTracePoint&nbsp;</TD></TR>
85388562
<TR><TD ALIGN="left" PORT="v21">v21:Fixnum[3] = Const Value(3)&nbsp;</TD></TR>
8539-
<TR><TD ALIGN="left" PORT="v23">PatchPoint NoTracePoint&nbsp;</TD></TR>
85408563
<TR><TD ALIGN="left" PORT="v24">CheckInterrupts&nbsp;</TD></TR>
85418564
<TR><TD ALIGN="left" PORT="v25">Return v21&nbsp;</TD></TR>
85428565
</TABLE>>];
@@ -8545,7 +8568,6 @@ mod graphviz_tests {
85458568
<TR><TD ALIGN="LEFT" PORT="params" BGCOLOR="gray">bb4(v26:BasicObject, v27:Falsy)&nbsp;</TD></TR>
85468569
<TR><TD ALIGN="left" PORT="v30">PatchPoint NoTracePoint&nbsp;</TD></TR>
85478570
<TR><TD ALIGN="left" PORT="v31">v31:Fixnum[4] = Const Value(4)&nbsp;</TD></TR>
8548-
<TR><TD ALIGN="left" PORT="v33">PatchPoint NoTracePoint&nbsp;</TD></TR>
85498571
<TR><TD ALIGN="left" PORT="v34">CheckInterrupts&nbsp;</TD></TR>
85508572
<TR><TD ALIGN="left" PORT="v35">Return v31&nbsp;</TD></TR>
85518573
</TABLE>>];

‎zjit/src/hir/opt_tests.rs‎

Lines changed: 0 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -115,10 +115,7 @@ mod hir_opt_tests {
115115
v10:Fixnum[1] = Const Value(1)
116116
v12:Fixnum[2] = Const Value(2)
117117
PatchPoint MethodRedefined(Integer@0x1000, +@0x1008, cme:0x1010)
118-
v34:Fixnum[3] = Const Value(3)
119118
IncrCounter inline_cfunc_optimized_send_count
120-
v17:Fixnum[3] = Const Value(3)
121-
PatchPoint MethodRedefined(Integer@0x1000, +@0x1008, cme:0x1010)
122119
v35:Fixnum[6] = Const Value(6)
123120
IncrCounter inline_cfunc_optimized_send_count
124121
CheckInterrupts
@@ -147,10 +144,7 @@ mod hir_opt_tests {
147144
v10:Fixnum[5] = Const Value(5)
148145
v12:Fixnum[3] = Const Value(3)
149146
PatchPoint MethodRedefined(Integer@0x1000, -@0x1008, cme:0x1010)
150-
v34:Fixnum[2] = Const Value(2)
151147
IncrCounter inline_cfunc_optimized_send_count
152-
v17:Fixnum[1] = Const Value(1)
153-
PatchPoint MethodRedefined(Integer@0x1000, -@0x1008, cme:0x1010)
154148
v35:Fixnum[1] = Const Value(1)
155149
IncrCounter inline_cfunc_optimized_send_count
156150
CheckInterrupts
@@ -3063,13 +3057,10 @@ mod hir_opt_tests {
30633057
PatchPoint SingleRactorMode
30643058
PatchPoint StableConstantNames(0x1000, String)
30653059
v26:Class[String@0x1008] = Const Value(VALUE(0x1008))
3066-
PatchPoint SingleRactorMode
30673060
PatchPoint StableConstantNames(0x1010, Class)
30683061
v29:Class[Class@0x1018] = Const Value(VALUE(0x1018))
3069-
PatchPoint SingleRactorMode
30703062
PatchPoint StableConstantNames(0x1020, Module)
30713063
v32:Class[Module@0x1028] = Const Value(VALUE(0x1028))
3072-
PatchPoint SingleRactorMode
30733064
PatchPoint StableConstantNames(0x1030, BasicObject)
30743065
v35:Class[BasicObject@0x1038] = Const Value(VALUE(0x1038))
30753066
v18:ArrayExact = NewArray v26, v29, v32, v35
@@ -3098,7 +3089,6 @@ mod hir_opt_tests {
30983089
PatchPoint SingleRactorMode
30993090
PatchPoint StableConstantNames(0x1000, Enumerable)
31003091
v22:ModuleExact[VALUE(0x1008)] = Const Value(VALUE(0x1008))
3101-
PatchPoint SingleRactorMode
31023092
PatchPoint StableConstantNames(0x1010, Kernel)
31033093
v25:ModuleExact[VALUE(0x1018)] = Const Value(VALUE(0x1018))
31043094
v14:ArrayExact = NewArray v22, v25
@@ -5226,7 +5216,6 @@ mod hir_opt_tests {
52265216
bb3(v6:BasicObject):
52275217
PatchPoint BOPRedefined(HASH_REDEFINED_OP_FLAG, BOP_FREEZE)
52285218
v11:HashExact[VALUE(0x1000)] = Const Value(VALUE(0x1000))
5229-
PatchPoint BOPRedefined(HASH_REDEFINED_OP_FLAG, BOP_FREEZE)
52305219
CheckInterrupts
52315220
Return v11
52325221
");
@@ -5323,7 +5312,6 @@ mod hir_opt_tests {
53235312
bb3(v6:BasicObject):
53245313
PatchPoint BOPRedefined(ARRAY_REDEFINED_OP_FLAG, BOP_FREEZE)
53255314
v11:ArrayExact[VALUE(0x1000)] = Const Value(VALUE(0x1000))
5326-
PatchPoint BOPRedefined(ARRAY_REDEFINED_OP_FLAG, BOP_FREEZE)
53275315
CheckInterrupts
53285316
Return v11
53295317
");
@@ -5420,7 +5408,6 @@ mod hir_opt_tests {
54205408
bb3(v6:BasicObject):
54215409
PatchPoint BOPRedefined(STRING_REDEFINED_OP_FLAG, BOP_FREEZE)
54225410
v11:StringExact[VALUE(0x1000)] = Const Value(VALUE(0x1000))
5423-
PatchPoint BOPRedefined(STRING_REDEFINED_OP_FLAG, BOP_FREEZE)
54245411
CheckInterrupts
54255412
Return v11
54265413
");
@@ -7222,7 +7209,6 @@ mod hir_opt_tests {
72227209
PatchPoint SingleRactorMode
72237210
PatchPoint StableConstantNames(0x1000, A)
72247211
v35:ArrayExact[VALUE(0x1008)] = Const Value(VALUE(0x1008))
7225-
PatchPoint SingleRactorMode
72267212
PatchPoint StableConstantNames(0x1010, B)
72277213
v38:ArrayExact[VALUE(0x1018)] = Const Value(VALUE(0x1018))
72287214
PatchPoint NoSingletonClass(Array@0x1020)
@@ -11514,7 +11500,6 @@ mod hir_opt_tests {
1151411500
v15:TrueClass = Const Value(true)
1151511501
PatchPoint NoSingletonClass(Class@0x1040)
1151611502
PatchPoint MethodRedefined(Class@0x1040, respond_to?@0x1048, cme:0x1050)
11517-
PatchPoint NoSingletonClass(Class@0x1040)
1151811503
PatchPoint MethodRedefined(Class@0x1040, _lex_actions@0x1078, cme:0x1080)
1151911504
v55:TrueClass = Const Value(true)
1152011505
IncrCounter inline_cfunc_optimized_send_count
@@ -12098,10 +12083,8 @@ mod hir_opt_tests {
1209812083
PatchPoint NoSingletonClass(TestNestedAccess@0x1010)
1209912084
PatchPoint MethodRedefined(TestNestedAccess@0x1010, x@0x1018, cme:0x1020)
1210012085
v52:Fixnum[100] = Const Value(100)
12101-
PatchPoint SingleRactorMode
1210212086
PatchPoint StableConstantNames(0x1048, NESTED_FROZEN)
1210312087
v33:HeapObject[VALUE(0x1008)] = Const Value(VALUE(0x1008))
12104-
PatchPoint NoSingletonClass(TestNestedAccess@0x1010)
1210512088
PatchPoint MethodRedefined(TestNestedAccess@0x1010, y@0x1050, cme:0x1058)
1210612089
v54:Fixnum[200] = Const Value(200)
1210712090
PatchPoint MethodRedefined(Integer@0x1080, +@0x1088, cme:0x1090)

‎zjit/src/hir/tests.rs‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,41 @@ mod snapshot_tests {
2424
format!("{}", FunctionPrinter::with_snapshot(&function))
2525
}
2626

27+
#[test]
28+
fn test_remove_redundant_patch_points() {
29+
eval("
30+
def test = 1 + 2 + 3
31+
test
32+
test
33+
");
34+
assert_snapshot!(optimized_hir_string("test"), @r"
35+
fn test@<compiled>:2:
36+
bb0():
37+
Entries bb1, bb2
38+
bb1():
39+
EntryPoint interpreter
40+
v1:BasicObject = LoadSelf
41+
Jump bb3(v1)
42+
bb2():
43+
EntryPoint JIT(0)
44+
v4:BasicObject = LoadArg :self@0
45+
Jump bb3(v4)
46+
bb3(v6:BasicObject):
47+
v8:Any = Snapshot FrameState { pc: 0x1000, stack: [], locals: [] }
48+
PatchPoint NoTracePoint
49+
v10:Fixnum[1] = Const Value(1)
50+
v12:Fixnum[2] = Const Value(2)
51+
v13:Any = Snapshot FrameState { pc: 0x1008, stack: [v10, v12], locals: [] }
52+
PatchPoint MethodRedefined(Integer@0x1010, +@0x1018, cme:0x1020)
53+
IncrCounter inline_cfunc_optimized_send_count
54+
v35:Fixnum[6] = Const Value(6)
55+
IncrCounter inline_cfunc_optimized_send_count
56+
v21:Any = Snapshot FrameState { pc: 0x1048, stack: [v35], locals: [] }
57+
CheckInterrupts
58+
Return v35
59+
");
60+
}
61+
2762
#[test]
2863
fn test_new_array_with_elements() {
2964
eval("def test(a, b) = [a, b]");

‎zjit/src/hir_effect/gen_hir_effect.rb‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ def to_graphviz effect, f
4848
allocator = any.subeffect 'Allocator'
4949
control = any.subeffect 'Control'
5050
memory = any.subeffect 'Memory'
51+
patch_point = any.subeffect 'PatchPoint'
5152
other = memory.subeffect 'Other'
5253
frame = memory.subeffect 'Frame'
5354
pc = frame.subeffect 'PC'

‎zjit/src/hir_effect/hir_effect.inc.rs‎

Lines changed: 8 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎zjit/src/stats.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,7 @@ make_counters! {
169169
compile_hir_strength_reduce_time_ns,
170170
compile_hir_fold_constants_time_ns,
171171
compile_hir_clean_cfg_time_ns,
172+
compile_hir_remove_redundant_patch_points_time_ns,
172173
compile_hir_eliminate_dead_code_time_ns,
173174
compile_lir_time_ns,
174175
}

0 commit comments

Comments
 (0)