Skip to content

Commit d3f7d2c

Browse files
tekknolagitekknolagi
authored
ZJIT: Pull load out of IsBlockParamModified (ruby#16673)
This makes IsBlockParamModified pure and the loads more understandable to load-store forwarding.
1 parent 8886990 commit d3f7d2c

5 files changed

Lines changed: 278 additions & 246 deletions

File tree

‎zjit/src/codegen.rs‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -731,7 +731,7 @@ fn gen_insn(cb: &mut CodeBlock, jit: &mut JITState, asm: &mut Assembler, functio
731731
Insn::GetIvar { self_val, id, ic, state: _ } => gen_getivar(jit, asm, opnd!(self_val), *id, *ic),
732732
Insn::SetGlobal { id, val, state } => no_output!(gen_setglobal(jit, asm, *id, opnd!(val), &function.frame_state(*state))),
733733
Insn::GetGlobal { id, state } => gen_getglobal(jit, asm, *id, &function.frame_state(*state)),
734-
&Insn::IsBlockParamModified { ep } => gen_is_block_param_modified(asm, opnd!(ep)),
734+
&Insn::IsBlockParamModified { flags } => gen_is_block_param_modified(asm, opnd!(flags)),
735735
&Insn::GetBlockParam { ep_offset, level, state } => gen_getblockparam(jit, asm, ep_offset, level, &function.frame_state(state)),
736736
&Insn::SetLocal { val, ep_offset, level } => no_output!(gen_setlocal(asm, opnd!(val), function.type_of(val), ep_offset, level)),
737737
Insn::GetConstant { klass, id, allow_nil, state } => gen_getconstant(jit, asm, opnd!(klass), *id, opnd!(allow_nil), &function.frame_state(*state)),
@@ -896,8 +896,7 @@ fn gen_setlocal(asm: &mut Assembler, val: Opnd, val_type: Type, local_ep_offset:
896896
}
897897

898898
/// Returns 1 (as CBool) when VM_FRAME_FLAG_MODIFIED_BLOCK_PARAM is set; returns 0 otherwise.
899-
fn gen_is_block_param_modified(asm: &mut Assembler, ep: Opnd) -> Opnd {
900-
let flags = asm.load(Opnd::mem(VALUE_BITS, ep, SIZEOF_VALUE_I32 * (VM_ENV_DATA_INDEX_FLAGS as i32)));
899+
fn gen_is_block_param_modified(asm: &mut Assembler, flags: Opnd) -> Opnd {
901900
asm.test(flags, VM_FRAME_FLAG_MODIFIED_BLOCK_PARAM.into());
902901
asm.csel_nz(Opnd::Imm(1), Opnd::Imm(0))
903902
}

‎zjit/src/cruby.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1564,6 +1564,7 @@ pub(crate) mod ids {
15641564
name: _env_data_index_specval
15651565
name: _ep_method_entry
15661566
name: _ep_specval
1567+
name: _ep_flags
15671568
name: _rbasic_flags
15681569
name: RUBY_FL_FREEZE
15691570
name: RUBY_ELTS_SHARED

‎zjit/src/hir.rs‎

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -925,9 +925,9 @@ pub enum Insn {
925925
StoreField { recv: InsnId, id: ID, offset: i32, val: InsnId },
926926
WriteBarrier { recv: InsnId, val: InsnId },
927927

928-
/// Check whether VM_FRAME_FLAG_MODIFIED_BLOCK_PARAM is set in the environment flags.
928+
/// Check whether VM_FRAME_FLAG_MODIFIED_BLOCK_PARAM is set in the (already loaded) environment flags.
929929
/// Returns CBool (0/1).
930-
IsBlockParamModified { ep: InsnId },
930+
IsBlockParamModified { flags: InsnId },
931931
/// Get the block parameter as a Proc.
932932
GetBlockParam { level: u32, ep_offset: u32, state: InsnId },
933933
/// Set a local variable in a higher scope or the heap
@@ -1162,8 +1162,8 @@ macro_rules! for_each_operand_impl {
11621162
Insn::IsBlockGiven { lep } => {
11631163
$visit_one!(lep);
11641164
}
1165-
Insn::IsBlockParamModified { ep } => {
1166-
$visit_one!(ep);
1165+
Insn::IsBlockParamModified { flags } => {
1166+
$visit_one!(flags);
11671167
}
11681168
Insn::CheckMatch { target, pattern, state, .. } => {
11691169
$visit_one!(target);
@@ -1588,7 +1588,7 @@ impl Insn {
15881588
Insn::GetSpecialNumber { .. } => effects::Any,
15891589
Insn::GetClassVar { .. } => effects::Any,
15901590
Insn::SetClassVar { .. } => effects::Any,
1591-
Insn::IsBlockParamModified { .. } => effects::Any,
1591+
Insn::IsBlockParamModified { .. } => effects::Empty,
15921592
Insn::GetBlockParam { .. } => effects::Any,
15931593
Insn::Snapshot { .. } => effects::Empty,
15941594
Insn::Jump(_) => effects::Any,
@@ -2141,8 +2141,8 @@ impl<'a> std::fmt::Display for InsnPrinter<'a> {
21412141
Insn::SetIvar { self_val, id, val, .. } => write!(f, "SetIvar {self_val}, :{}, {val}", id.contents_lossy()),
21422142
Insn::GetGlobal { id, .. } => write!(f, "GetGlobal :{}", id.contents_lossy()),
21432143
Insn::SetGlobal { id, val, .. } => write!(f, "SetGlobal :{}, {val}", id.contents_lossy()),
2144-
&Insn::IsBlockParamModified { ep } => {
2145-
write!(f, "IsBlockParamModified {ep}")
2144+
&Insn::IsBlockParamModified { flags } => {
2145+
write!(f, "IsBlockParamModified {flags}")
21462146
},
21472147
&Insn::SetLocal { val, level, ep_offset } => {
21482148
let name = get_local_var_name_for_printer(self.iseq, level, ep_offset).map_or(String::new(), |x| format!("{x}, "));
@@ -2833,7 +2833,7 @@ impl Function {
28332833
&GuardGreaterEq { left, right, reason, state } => GuardGreaterEq { left: find!(left), right: find!(right), reason, state },
28342834
&GuardLess { left, right, state } => GuardLess { left: find!(left), right: find!(right), state },
28352835
&IsBlockGiven { lep } => IsBlockGiven { lep: find!(lep) },
2836-
&IsBlockParamModified { ep } => IsBlockParamModified { ep: find!(ep) },
2836+
&IsBlockParamModified { flags } => IsBlockParamModified { flags: find!(flags) },
28372837
&GetBlockParam { level, ep_offset, state } => GetBlockParam { level, ep_offset, state: find!(state) },
28382838
&FixnumAdd { left, right, state } => FixnumAdd { left: find!(left), right: find!(right), state },
28392839
&FixnumSub { left, right, state } => FixnumSub { left: find!(left), right: find!(right), state },
@@ -3620,6 +3620,10 @@ impl Function {
36203620
self.push_insn(block, Insn::LoadField { recv, id: ID!(_rbasic_flags), offset: RUBY_OFFSET_RBASIC_FLAGS, return_type: types::CUInt64 })
36213621
}
36223622

3623+
fn load_ep_flags(&mut self, block: BlockId, ep: InsnId) -> InsnId {
3624+
self.push_insn(block, Insn::LoadField { recv: ep, id: ID!(_ep_flags), offset: SIZEOF_VALUE_I32 * (VM_ENV_DATA_INDEX_FLAGS as i32), return_type: types::CUInt64 })
3625+
}
3626+
36233627
pub fn guard_not_frozen(&mut self, block: BlockId, recv: InsnId, state: InsnId) {
36243628
let flags = self.load_rbasic_flags(block, recv);
36253629
self.push_insn(block, Insn::GuardNoBitsSet { val: flags, mask: Const::CUInt64(RUBY_FL_FREEZE as u64), mask_name: Some(ID!(RUBY_FL_FREEZE)), reason: SideExitReason::GuardNotFrozen, state });
@@ -6167,7 +6171,6 @@ impl Function {
61676171
| Insn::LoadField { .. }
61686172
| Insn::GetConstantPath { .. }
61696173
| Insn::IsBlockGiven { .. }
6170-
| Insn::IsBlockParamModified { .. }
61716174
| Insn::GetGlobal { .. }
61726175
| Insn::LoadPC
61736176
| Insn::LoadSP
@@ -6456,6 +6459,7 @@ impl Function {
64566459
}
64576460
Insn::RefineType { .. } => Ok(()),
64586461
Insn::HasType { val, .. } => self.assert_subtype(insn_id, val, types::BasicObject),
6462+
Insn::IsBlockParamModified { flags } => self.assert_subtype(insn_id, flags, types::CUInt64),
64596463
}
64606464
}
64616465

@@ -7612,7 +7616,8 @@ pub fn iseq_to_hir(iseq: *const rb_iseq_t) -> Result<Function, ParseError> {
76127616
let join_local = if level == 0 { Some(fun.push_insn(join_block, Insn::Param)) } else { None };
76137617

76147618
let ep = fun.push_insn(block, Insn::GetEP { level });
7615-
let is_modified = fun.push_insn(block, Insn::IsBlockParamModified { ep });
7619+
let flags = fun.load_ep_flags(block, ep);
7620+
let is_modified = fun.push_insn(block, Insn::IsBlockParamModified { flags });
76167621

76177622
fun.push_insn(block, Insn::IfTrue { val: is_modified, target: BranchEdge { target: modified_block, args: vec![] }});
76187623
fun.push_insn(block, Insn::Jump(BranchEdge { target: unmodified_block, args: vec![] }));
@@ -7781,7 +7786,8 @@ pub fn iseq_to_hir(iseq: *const rb_iseq_t) -> Result<Function, ParseError> {
77817786
let join_param = fun.push_insn(join_block, Insn::Param);
77827787

77837788
let ep = fun.push_insn(block, Insn::GetEP { level });
7784-
let is_modified = fun.push_insn(block, Insn::IsBlockParamModified { ep });
7789+
let flags = fun.load_ep_flags(block, ep);
7790+
let is_modified = fun.push_insn(block, Insn::IsBlockParamModified { flags });
77857791

77867792
fun.push_insn(block, Insn::IfTrue {
77877793
val: is_modified,

0 commit comments

Comments
 (0)