Skip to content

Commit 07ddb0e

Browse files
committed
ZJIT: Read iseq->body->param directly instead of through FFI
Going through a call to a C function just to read a bitfield was a little extreme. We did it to be super conservative since bitfields have historically been the trigger of many bugs and surprises. Let's try directly accessing them with code from rust-bindgen. If this ends up causing issues, we can use the FFI approach behind nicer wrappers. In any case, directly access regular struct fields such as `lead_num` and `opt_num` to remove boilerplate.
1 parent 7a09df4 commit 07ddb0e

File tree

3 files changed

+45
-40
lines changed

3 files changed

+45
-40
lines changed

zjit/src/codegen.rs

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1293,10 +1293,11 @@ fn gen_send_without_block_direct(
12931293
let mut c_args = vec![recv];
12941294
c_args.extend(&args);
12951295

1296-
let num_optionals_passed = if unsafe { get_iseq_flags_has_opt(iseq) } {
1296+
let params = unsafe { iseq.params() };
1297+
let num_optionals_passed = if params.flags.has_opt() != 0 {
12971298
// See vm_call_iseq_setup_normal_opt_start in vm_inshelper.c
1298-
let lead_num = unsafe { get_iseq_body_param_lead_num(iseq) } as u32;
1299-
let opt_num = unsafe { get_iseq_body_param_opt_num(iseq) } as u32;
1299+
let lead_num = params.lead_num as u32;
1300+
let opt_num = params.opt_num as u32;
13001301
assert!(args.len() as u32 <= lead_num + opt_num);
13011302
let num_optionals_passed = args.len() as u32 - lead_num;
13021303
num_optionals_passed
@@ -2212,7 +2213,7 @@ c_callable! {
22122213

22132214
// Fill nils to uninitialized (non-argument) locals
22142215
let local_size = get_iseq_body_local_table_size(iseq).to_usize();
2215-
let num_params = get_iseq_body_param_size(iseq).to_usize();
2216+
let num_params = iseq.params().size.to_usize();
22162217
let base = sp.offset(-local_size_and_idx_to_bp_offset(local_size, num_params) as isize);
22172218
slice::from_raw_parts_mut(base, local_size - num_params).fill(Qnil);
22182219
}

zjit/src/cruby.rs

Lines changed: 18 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -192,21 +192,7 @@ pub use rb_get_iseq_body_local_iseq as get_iseq_body_local_iseq;
192192
pub use rb_get_iseq_body_iseq_encoded as get_iseq_body_iseq_encoded;
193193
pub use rb_get_iseq_body_stack_max as get_iseq_body_stack_max;
194194
pub use rb_get_iseq_body_type as get_iseq_body_type;
195-
pub use rb_get_iseq_flags_has_lead as get_iseq_flags_has_lead;
196-
pub use rb_get_iseq_flags_has_opt as get_iseq_flags_has_opt;
197-
pub use rb_get_iseq_flags_has_kw as get_iseq_flags_has_kw;
198-
pub use rb_get_iseq_flags_has_rest as get_iseq_flags_has_rest;
199-
pub use rb_get_iseq_flags_has_post as get_iseq_flags_has_post;
200-
pub use rb_get_iseq_flags_has_kwrest as get_iseq_flags_has_kwrest;
201-
pub use rb_get_iseq_flags_has_block as get_iseq_flags_has_block;
202-
pub use rb_get_iseq_flags_ambiguous_param0 as get_iseq_flags_ambiguous_param0;
203-
pub use rb_get_iseq_flags_accepts_no_kwarg as get_iseq_flags_accepts_no_kwarg;
204195
pub use rb_get_iseq_body_local_table_size as get_iseq_body_local_table_size;
205-
pub use rb_get_iseq_body_param_keyword as get_iseq_body_param_keyword;
206-
pub use rb_get_iseq_body_param_size as get_iseq_body_param_size;
207-
pub use rb_get_iseq_body_param_lead_num as get_iseq_body_param_lead_num;
208-
pub use rb_get_iseq_body_param_opt_num as get_iseq_body_param_opt_num;
209-
pub use rb_get_iseq_body_param_opt_table as get_iseq_body_param_opt_table;
210196
pub use rb_get_cikw_keyword_len as get_cikw_keyword_len;
211197
pub use rb_get_cikw_keywords_idx as get_cikw_keywords_idx;
212198
pub use rb_get_call_data_ci as get_call_data_ci;
@@ -306,11 +292,10 @@ pub fn iseq_escapes_ep(iseq: IseqPtr) -> bool {
306292
}
307293

308294
/// Index of the local variable that has a rest parameter if any
309-
pub fn iseq_rest_param_idx(iseq: IseqPtr) -> Option<i32> {
310-
if !iseq.is_null() && unsafe { get_iseq_flags_has_rest(iseq) } {
311-
let opt_num = unsafe { get_iseq_body_param_opt_num(iseq) };
312-
let lead_num = unsafe { get_iseq_body_param_lead_num(iseq) };
313-
Some(opt_num + lead_num)
295+
pub fn iseq_rest_param_idx(params: &IseqParameters) -> Option<i32> {
296+
// TODO(alan): replace with `params.rest_start`
297+
if params.flags.has_rest() != 0 {
298+
Some(params.opt_num + params.lead_num)
314299
} else {
315300
None
316301
}
@@ -686,6 +671,20 @@ impl VALUE {
686671
}
687672
}
688673

674+
pub type IseqParameters = rb_iseq_constant_body_rb_iseq_parameters;
675+
676+
/// Extension trait to enable method calls on [`IseqPtr`]
677+
pub trait IseqAccess {
678+
unsafe fn params<'a>(self) -> &'a IseqParameters;
679+
}
680+
681+
impl IseqAccess for IseqPtr {
682+
/// Get a description of the ISEQ's signature. Analogous to `ISEQ_BODY(iseq)->param` in C.
683+
unsafe fn params<'a>(self) -> &'a IseqParameters {
684+
unsafe { &(*(*self).body).param }
685+
}
686+
}
687+
689688
impl From<IseqPtr> for VALUE {
690689
/// For `.into()` convenience
691690
fn from(iseq: IseqPtr) -> Self {

zjit/src/hir.rs

Lines changed: 22 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1497,23 +1497,24 @@ fn can_direct_send(function: &mut Function, block: BlockId, iseq: *const rb_iseq
14971497
can_send = false;
14981498
function.push_insn(block, Insn::IncrCounter(counter));
14991499
};
1500+
let params = unsafe { iseq.params() };
15001501

15011502
use Counter::*;
1502-
if unsafe { rb_get_iseq_flags_has_rest(iseq) } { count_failure(complex_arg_pass_param_rest) }
1503-
if unsafe { rb_get_iseq_flags_has_post(iseq) } { count_failure(complex_arg_pass_param_post) }
1504-
if unsafe { rb_get_iseq_flags_has_kw(iseq) } { count_failure(complex_arg_pass_param_kw) }
1505-
if unsafe { rb_get_iseq_flags_has_kwrest(iseq) } { count_failure(complex_arg_pass_param_kwrest) }
1506-
if unsafe { rb_get_iseq_flags_has_block(iseq) } { count_failure(complex_arg_pass_param_block) }
1507-
if unsafe { rb_get_iseq_flags_forwardable(iseq) } { count_failure(complex_arg_pass_param_forwardable) }
1503+
if 0 != params.flags.has_rest() { count_failure(complex_arg_pass_param_rest) }
1504+
if 0 != params.flags.has_post() { count_failure(complex_arg_pass_param_post) }
1505+
if 0 != params.flags.has_kw() { count_failure(complex_arg_pass_param_kw) }
1506+
if 0 != params.flags.has_kwrest() { count_failure(complex_arg_pass_param_kwrest) }
1507+
if 0 != params.flags.has_block() { count_failure(complex_arg_pass_param_block) }
1508+
if 0 != params.flags.forwardable() { count_failure(complex_arg_pass_param_forwardable) }
15081509

15091510
if !can_send {
15101511
function.set_dynamic_send_reason(send_insn, ComplexArgPass);
15111512
return false;
15121513
}
15131514

15141515
// Because we exclude e.g. post parameters above, they are also excluded from the sum below.
1515-
let lead_num = unsafe { get_iseq_body_param_lead_num(iseq) };
1516-
let opt_num = unsafe { get_iseq_body_param_opt_num(iseq) };
1516+
let lead_num = params.lead_num;
1517+
let opt_num = params.opt_num;
15171518
can_send = c_int::try_from(args.len())
15181519
.as_ref()
15191520
.map(|argc| (lead_num..=lead_num + opt_num).contains(argc))
@@ -2086,8 +2087,9 @@ impl Function {
20862087
/// Set self.param_types. They are copied to the param types of jit_entry_blocks.
20872088
fn set_param_types(&mut self) {
20882089
let iseq = self.iseq;
2089-
let param_size = unsafe { get_iseq_body_param_size(iseq) }.to_usize();
2090-
let rest_param_idx = iseq_rest_param_idx(iseq);
2090+
let params = unsafe { iseq.params() };
2091+
let param_size = params.size.to_usize();
2092+
let rest_param_idx = iseq_rest_param_idx(params);
20912093

20922094
self.param_types.push(types::BasicObject); // self
20932095
for local_idx in 0..param_size {
@@ -4596,11 +4598,12 @@ fn insn_idx_at_offset(idx: u32, offset: i64) -> u32 {
45964598
/// List of insn_idx that starts a JIT entry block
45974599
pub fn jit_entry_insns(iseq: IseqPtr) -> Vec<u32> {
45984600
// TODO(alan): Make an iterator type for this instead of copying all of the opt_table each call
4599-
let opt_num = unsafe { get_iseq_body_param_opt_num(iseq) };
4601+
let params = unsafe { iseq.params() };
4602+
let opt_num = params.opt_num;
46004603
if opt_num > 0 {
46014604
let mut result = vec![];
46024605

4603-
let opt_table = unsafe { get_iseq_body_param_opt_table(iseq) }; // `opt_num + 1` entries
4606+
let opt_table = params.opt_table; // `opt_num + 1` entries
46044607
for opt_idx in 0..=opt_num as isize {
46054608
let insn_idx = unsafe { opt_table.offset(opt_idx).read().as_u32() };
46064609
result.push(insn_idx);
@@ -5715,8 +5718,9 @@ fn compile_entry_state(fun: &mut Function) -> (InsnId, FrameState) {
57155718
fun.push_insn(entry_block, Insn::EntryPoint { jit_entry_idx: None });
57165719

57175720
let iseq = fun.iseq;
5718-
let param_size = unsafe { get_iseq_body_param_size(iseq) }.to_usize();
5719-
let rest_param_idx = iseq_rest_param_idx(iseq);
5721+
let params = unsafe { iseq.params() };
5722+
let param_size = params.size.to_usize();
5723+
let rest_param_idx = iseq_rest_param_idx(params);
57205724

57215725
let self_param = fun.push_insn(entry_block, Insn::LoadSelf);
57225726
let mut entry_state = FrameState::new(iseq);
@@ -5748,9 +5752,10 @@ fn compile_jit_entry_block(fun: &mut Function, jit_entry_idx: usize, target_bloc
57485752
/// Compile params and initial locals for a jit_entry_block
57495753
fn compile_jit_entry_state(fun: &mut Function, jit_entry_block: BlockId, jit_entry_idx: usize) -> (InsnId, FrameState) {
57505754
let iseq = fun.iseq;
5751-
let param_size = unsafe { get_iseq_body_param_size(iseq) }.to_usize();
5752-
let opt_num: usize = unsafe { get_iseq_body_param_opt_num(iseq) }.try_into().expect("iseq param opt_num >= 0");
5753-
let lead_num: usize = unsafe { get_iseq_body_param_lead_num(iseq) }.try_into().expect("iseq param lead_num >= 0");
5755+
let params = unsafe { iseq.params() };
5756+
let param_size = params.size.to_usize();
5757+
let opt_num: usize = params.opt_num.try_into().expect("iseq param opt_num >= 0");
5758+
let lead_num: usize = params.lead_num.try_into().expect("iseq param lead_num >= 0");
57545759
let passed_opt_num = jit_entry_idx;
57555760

57565761
let self_param = fun.push_insn(jit_entry_block, Insn::Param);

0 commit comments

Comments
 (0)