From 72c9ee51599f6ed29aa50f096703d3d9dd990afe Mon Sep 17 00:00:00 2001 From: Alan Wu Date: Fri, 11 Aug 2023 11:58:46 -0400 Subject: [PATCH] nil fills --- yjit/src/codegen.rs | 105 +++++++++++++++++++++++--------------------- 1 file changed, 54 insertions(+), 51 deletions(-) diff --git a/yjit/src/codegen.rs b/yjit/src/codegen.rs index 2213da39e9a51f..2fc38b2f668aa3 100644 --- a/yjit/src/codegen.rs +++ b/yjit/src/codegen.rs @@ -4952,7 +4952,6 @@ struct ControlFrame { frame_type: u32, specval: SpecVal, cme: *const rb_callable_method_entry_t, - local_size: i32 } // Codegen performing a similar (but not identical) function to vm_push_frame @@ -4976,8 +4975,6 @@ fn gen_push_frame( set_sp_cfp: bool, // if true CFP and SP will be switched to the callee frame: ControlFrame, ) { - assert!(frame.local_size >= 0); - let sp = frame.sp; asm.comment("push cme, specval, frame type"); @@ -5059,20 +5056,6 @@ fn gen_push_frame( asm.mov(cfp_opnd(RUBY_OFFSET_CFP_SELF), frame.recv); asm.mov(cfp_opnd(RUBY_OFFSET_CFP_BLOCK_CODE), 0.into()); - // This Qnil fill snippet potentially requires 2 more registers on Arm, one for Qnil and - // another for calculating the address in case there are a lot of local variables. So doing - // this after releasing the register for specval and the receiver to avoid register spill. - let num_locals = frame.local_size; - if num_locals > 0 { - asm.comment("initialize locals"); - - // Initialize local variables to Qnil - for i in 0..num_locals { - let offs = SIZEOF_VALUE_I32 * (i - num_locals - 3); - asm.store(Opnd::mem(64, sp, offs), Qnil.into()); - } - } - // Spill stack temps to let the callee use them (must be done before changing SP) asm.spill_temps(); @@ -5304,7 +5287,6 @@ fn gen_send_cfunc( None // Leave PC uninitialized as cfuncs shouldn't read it }, iseq: None, - local_size: 0, }); if !kw_arg.is_null() { @@ -5623,6 +5605,8 @@ fn gen_send_iseq( argc: i32, captured_opnd: Option, ) -> Option { + // XXX: expand on this with the various identities you use + // The argument deepest in the stack will become the 0th local in the callee. let mut argc = argc; // When you have keyword arguments, there is an extra object that gets @@ -6041,7 +6025,7 @@ fn gen_send_iseq( new_ary }; - // Find where the rest parameter array should go + // Find where to put the rest parameter array let rest_param = if opts_missing == 0 { // All optionals are filled, the rest param goes at the top of the stack argc += 1; @@ -6064,37 +6048,6 @@ fn gen_send_iseq( asm.store(rest_param, rest_param_array); } - // Nil-initialize missing optional parameters in case they are not - // contiguous with non-parameter locals. When there is no gap between - // the two they'll be nil-initilaized with non-parameter locals later. - if opts_missing > 0 && - required_num + opt_num != unsafe { get_iseq_body_param_size(iseq) as c_int } { - asm.comment("nil fill missing opt params"); - - // rest + unfilled opts? - - // because kw unspecificed bits comes after rest - // if opts_missing == 0 -> push - // else opts_missing >0 -> don't? nah - // ^ wrong because either way kw uses -1. - - // add to stack size, if appropriate - // set opts_missing to all unknown - or nil? - // actually, no, because only argc get type transferred in the context - // and the unspecified bits below use stack_opnd(-1) - - // XXX: ^ contradiction between the two. - // ✅seems fine. because of exit_if_doing_kw_and_opts_missing - // and exit_if_doing_kw_and_splat. So we never have the clobbering case of - // negative idx in the rest array movement above and the storing of - // unspecified_bit at stack_opnd(-1) below. - // - // XXX: wanting to place rest param at just the right spot without movement - - // The argument deepest in the stack will become the first local in the callee. - let callee_locals_base = argc - 1; - } - if doing_kw_call { // Here we're calling a method with keyword arguments and specifying // keyword arguments at this call site. @@ -6262,6 +6215,57 @@ fn gen_send_iseq( argc = lead_num; } + // because kw unspecificed bits comes after rest + // if opts_missing == 0 -> push + // else opts_missing >0 -> don't? nah + // ^ wrong because either way kw uses -1. + + // add to stack size, if appropriate + // set opts_missing to all unknown - or nil? + // actually, no, because only argc get type transferred in the context + // and the unspecified bits below use stack_opnd(-1) + + // XXX: ^ contradiction between the two. + // ✅seems fine. because of `exit_if_doing_kw_and_opts_missing` + // and `exit_if_doing_kw_and_splat`. So we never have the clobbering case of + // negative idx in the rest array movement above and the storing of + // unspecified_bit at stack_opnd(-1) below. + + fn nil_fill(comment: &'static str, fill_range: std::ops::Range, asm: &mut Assembler) { + if fill_range.is_empty() { + return; + } + + asm.comment(comment); + for i in fill_range { + let value_slot = asm.ctx.sp_opnd(i * SIZEOF_VALUE as isize); + asm.store(value_slot, Qnil.into()); + } + } + + // Nil-initialize missing optional parameters + nil_fill( + "nil-initialize missing optionals", + { + let begin = -(argc as isize) + required_num as isize + opts_filled as isize; + let end = -(argc as isize) + required_num as isize + opt_num as isize; + + begin..end + }, + asm + ); + // Nil-initialize non-parameter local variables + nil_fill( + "nil-initialize locals", + { + let begin = -(argc as isize) + num_params as isize; + let end = -(argc as isize) + num_locals_total as isize; + + begin..end + }, + asm + ); + // Points to the receiver operand on the stack unless a captured environment is used let recv = match captured_opnd { Some(captured_opnd) => asm.load(Opnd::mem(64, captured_opnd, 0)), // captured->self @@ -6308,7 +6312,6 @@ fn gen_send_iseq( sp: callee_sp, iseq: Some(iseq), pc: None, // We are calling into jitted code, which will set the PC as necessary - local_size: num_locals }); // No need to set cfp->pc since the callee sets it whenever calling into routines