diff --git a/.github/workflows/zjit-macos.yml b/.github/workflows/zjit-macos.yml index b6da6db9a6035e..f82baa081ab6cc 100644 --- a/.github/workflows/zjit-macos.yml +++ b/.github/workflows/zjit-macos.yml @@ -58,7 +58,7 @@ jobs: env: GITPULLOPTIONS: --no-tags origin ${{ github.ref }} RUN_OPTS: ${{ matrix.run_opts }} - SPECOPTS: ${{ matrix.specopts }} + SPECOPTS: ${{ matrix.specopts }} -B../src/spec/zjit.mspec TESTOPTS: ${{ matrix.testopts }} RUST_BACKTRACE: 1 ZJIT_RB_BUG: 1 diff --git a/.github/workflows/zjit-ubuntu.yml b/.github/workflows/zjit-ubuntu.yml index 0bf8930c5f603a..396397052e0758 100644 --- a/.github/workflows/zjit-ubuntu.yml +++ b/.github/workflows/zjit-ubuntu.yml @@ -110,7 +110,7 @@ jobs: GITPULLOPTIONS: --no-tags origin ${{ github.ref }} RUN_OPTS: ${{ matrix.run_opts }} YJIT_BENCH_OPTS: ${{ matrix.yjit_bench_opts }} - SPECOPTS: ${{ matrix.specopts }} + SPECOPTS: ${{ matrix.specopts }} -B../src/spec/zjit.mspec TESTOPTS: ${{ matrix.testopts }} RUBY_DEBUG: ci BUNDLE_JOBS: 8 # for yjit-bench diff --git a/set.c b/set.c index 0ea48a17e19ce7..3d88d8d2ca10ae 100644 --- a/set.c +++ b/set.c @@ -776,7 +776,7 @@ set_i_add(VALUE set, VALUE item) * call-seq: * add?(object) -> self or nil * - * Like #add, but returns +nil+ if +object+ is already in +self+: + * Like #add, but returns +nil+ if the given +object+ is already in +self+: * * set = Set[0, 1, 2] * set.add?(:foo) # => Set[0, 1, 2, :foo] @@ -2432,41 +2432,39 @@ rb_set_size(VALUE set) * or greater than a given object. * - #==: Returns whether +self+ and a given enumerable are equal, * as determined by Object#eql?. - * - * === Methods for Querying - * * - #compare_by_identity?: - * Returns whether the set considers only identity + * Returns whether +self+ considers only identity * when comparing elements. - * - #size (aliased as #length): - * Returns the count of elements. - * - #empty?: - * Returns whether the set has no elements. - * - #include? (aliased as #member? and #===): - * Returns whether a given object is an element in the set. - * - #subset? (aliased as #<=): - * Returns whether a given object is a subset of the set. * - #proper_subset? (aliased as #<): - * Returns whether a given enumerable is a proper subset of the set. - * - #superset? (aliased as #>=): - * Returns whether a given enumerable is a superset of the set. + * Returns whether the given enumerable is a proper subset of +self+. * - #proper_superset? (aliased as #>): - * Returns whether a given enumerable is a proper superset of the set. + * Returns whether the given enumerable is a proper superset of +self+. + * - #subset? (aliased as #<=): + * Returns whether the given object is a subset of +self+. + * - #superset? (aliased as #>=): + * Returns whether the given enumerable is a superset of +self+. + * + * === Methods for Querying + * * - #disjoint?: - * Returns +true+ if the set and a given enumerable - * have no common elements, +false+ otherwise. + * Returns whether no element of the given enumerable is present in +self+. + * - #empty?: + * Returns whether +self+ contains no elements. + * - #include? (aliased as #member? and #===): + * Returns whether the given object is an element of +self+. * - #intersect?: - * Returns +true+ if the set and a given enumerable: - * have any common elements, +false+ otherwise. + * Returns whether +self+ and the given enumerable have any elements in common. + * - #size (aliased as #length): + * Returns the number of elements in +self+. * * === Methods for Assigning * * - #add (aliased as #<<): - * Adds the given object to +self+, returns +self+. + * Adds the given object to +self+; returns +self+. * - #add?: * Like #add, but returns +nil+ if the given object is already in +self+. * - #merge: - * Adds the elements of the given enumerables to +self+; returns +self+. + * Adds each element of each of the given enumerables to +self+; returns +self+. * - #replace: * Replaces the contents of +self+ with the contents of the given enumerable; * returns +self+. diff --git a/spec/zjit.mspec b/spec/zjit.mspec new file mode 100644 index 00000000000000..570b17fcf08881 --- /dev/null +++ b/spec/zjit.mspec @@ -0,0 +1,2 @@ +# Fails with --zjit-disable-hir-opt. See https://github.com/Shopify/ruby/issues/970 +MSpec.register(:exclude, "Kernel#eval updates a local in a scope above a surrounding block scope") diff --git a/test/.excludes-zjit/TestKeywordArguments.rb b/test/.excludes-zjit/TestKeywordArguments.rb new file mode 100644 index 00000000000000..93e137669232e3 --- /dev/null +++ b/test/.excludes-zjit/TestKeywordArguments.rb @@ -0,0 +1,3 @@ +# See . +# This tests fail with --zjit-disable-hir-opt +exclude(:test_required_keyword_with_newline, 'local assignment within eval') diff --git a/test/.excludes-zjit/TestParse.rb b/test/.excludes-zjit/TestParse.rb new file mode 100644 index 00000000000000..b1b622d5b7b6be --- /dev/null +++ b/test/.excludes-zjit/TestParse.rb @@ -0,0 +1,8 @@ +# See . +# These tests fail with --zjit-disable-hir-opt +exclude(:test_utf8_bom, 'local assignment within eval') +exclude(:test_pow_asgn, 'local assignment within eval') +exclude(:test_backquote, 'local assignment within eval') +exclude(:test_dot_in_next_line, 'local assignment within eval') +exclude(:test_here_document, 'local assignment within eval') +exclude(:test_magic_comment, 'local assignment within eval') diff --git a/test/.excludes-zjit/TestString.rb b/test/.excludes-zjit/TestString.rb new file mode 100644 index 00000000000000..4fb2634534f8f0 --- /dev/null +++ b/test/.excludes-zjit/TestString.rb @@ -0,0 +1,3 @@ +# See . +# This tests fail with --zjit-disable-hir-opt +exclude(:test_unknown_string_option, 'local assignment within eval') diff --git a/test/.excludes-zjit/TestString2.rb b/test/.excludes-zjit/TestString2.rb new file mode 100644 index 00000000000000..4fb2634534f8f0 --- /dev/null +++ b/test/.excludes-zjit/TestString2.rb @@ -0,0 +1,3 @@ +# See . +# This tests fail with --zjit-disable-hir-opt +exclude(:test_unknown_string_option, 'local assignment within eval') diff --git a/test/digest/test_digest.rb b/test/digest/test_digest.rb index 95cce939401e07..8a39a7bc27820a 100644 --- a/test/digest/test_digest.rb +++ b/test/digest/test_digest.rb @@ -204,7 +204,80 @@ class TestCRC32 < Test::Unit::TestCase Data1 => "352441c2", Data2 => "171a3f5f", } - end if defined?(Digest::CRC32) + + def test_digest_byte_order + # CRC32("abc") = 0x352441C2; raw digest bytes must be big-endian (MSB first) + assert_equal [0x35, 0x24, 0x41, 0xC2], Digest::CRC32.digest("abc").bytes + end + + def test_digest_length + assert_equal 4, Digest::CRC32.new.digest_length + end + + def test_block_length + assert_equal 8, Digest::CRC32.new.block_length + end + + def test_empty_string + assert_equal "00000000", Digest::CRC32.hexdigest("") + end + + def test_single_null_byte + assert_equal "d202ef8d", Digest::CRC32.hexdigest("\x00".b) + end + + def test_high_bytes + # Exercises the unsigned byte masking (& 0xFF) in the update loop + assert_equal "08eaaf6d", Digest::CRC32.hexdigest("\xFF\xFE\xFD".b) + end + + def test_all_byte_values + # Exercises every entry in the lookup table + assert_equal "29058c73", Digest::CRC32.hexdigest((0..255).map(&:chr).join.b) + end + + def test_incremental_equals_one_shot + inc = Digest::CRC32.new + inc << "a" << "b" << "c" + assert_equal "352441c2", inc.hexdigest + end + + def test_clone_mid_stream_independence + d = Digest::CRC32.new + d << "ab" + copy = d.clone + d << "c" + copy << "c" + assert_equal d.hexdigest, copy.hexdigest + end + + def test_reset + d = Digest::CRC32.new + d << "abc" + d.reset + d << "abc" + assert_equal "352441c2", d.hexdigest + end + + def test_digest_is_non_destructive + d = Digest::CRC32.new + d << "abc" + assert_equal d.hexdigest, d.hexdigest + end + + def test_digest_bang_resets_state + d = Digest::CRC32.new + d << "abc" + d.hexdigest! + assert_equal "00000000", d.hexdigest + end + + def test_initialize_copy_into_frozen_raises + dest = Digest::CRC32.allocate + dest.freeze + assert_raise(FrozenError) { dest.send(:initialize_copy, Digest::CRC32.new) } + end + end class TestBase < Test::Unit::TestCase def test_base diff --git a/zjit.c b/zjit.c index bc03deafeec665..082a9038066723 100644 --- a/zjit.c +++ b/zjit.c @@ -29,7 +29,8 @@ STATIC_ASSERT(pointer_tagging_scheme, USE_FLONUM); enum zjit_struct_offsets { - ISEQ_BODY_OFFSET_PARAM = offsetof(struct rb_iseq_constant_body, param) + ISEQ_BODY_OFFSET_PARAM = offsetof(struct rb_iseq_constant_body, param), + ISEQ_BODY_OFFSET_OUTER_VARIABLES = offsetof(struct rb_iseq_constant_body, outer_variables) }; // Special JITFrame used by all C method calls. We don't control the native diff --git a/zjit/bindgen/src/main.rs b/zjit/bindgen/src/main.rs index c6f8e85b3d29ec..55f784bd03965e 100644 --- a/zjit/bindgen/src/main.rs +++ b/zjit/bindgen/src/main.rs @@ -297,6 +297,7 @@ fn main() { .allowlist_function("rb_zjit_iseq_inspect") .allowlist_function("rb_zjit_iseq_insn_set") .allowlist_function("rb_zjit_local_id") + .allowlist_function("rb_id_table_lookup") .allowlist_function("rb_set_cfp_(pc|sp)") .allowlist_function("rb_c_method_tracing_currently_enabled") .allowlist_function("rb_zjit_method_tracing_currently_enabled") @@ -449,6 +450,9 @@ fn main() { .blocklist_type("ID") .blocklist_type("rb_iseq_constant_body") + // We only need id_table as an opaque pointer to pass to its APIs + .opaque_type("rb_id_table") + // Avoid binding to stuff we don't use .blocklist_item("rb_thread_struct.*") .opaque_type("rb_thread_struct.*") diff --git a/zjit/src/backend/arm64/mod.rs b/zjit/src/backend/arm64/mod.rs index 38796e18a2026b..fdd88e5001126a 100644 --- a/zjit/src/backend/arm64/mod.rs +++ b/zjit/src/backend/arm64/mod.rs @@ -1120,6 +1120,9 @@ impl Assembler { Insn::PosMarker(..) => { pos_markers.push((insn_idx, cb.get_write_ptr())) } + Insn::PosMarkerAtBlockEnd(..) => { + unreachable!("PosMarkerAtBlockEnd should have been lowered by linearize_instructions"); + }, Insn::BakeString(text) => { for byte in text.as_bytes() { cb.write_byte(*byte); diff --git a/zjit/src/backend/lir.rs b/zjit/src/backend/lir.rs index cd9d2cfdc9df54..e5962b84dca775 100644 --- a/zjit/src/backend/lir.rs +++ b/zjit/src/backend/lir.rs @@ -894,6 +894,10 @@ pub enum Insn { // Mark a position in the generated code PosMarker(PosMarkerFn), + /// Mark a position at the end of the current LIR block. + /// This is lowered to PosMarker during linearize_instructions(). + PosMarkerAtBlockEnd(PosMarkerFn), + /// Shift a value right by a certain amount (signed). RShift { opnd: Opnd, shift: Opnd, out: Opnd }, @@ -970,7 +974,8 @@ macro_rules! for_each_operand_impl { Insn::Comment(_) | Insn::CPop { .. } | Insn::PadPatchPoint | - Insn::PosMarker(_) => {}, + Insn::PosMarker(_) | + Insn::PosMarkerAtBlockEnd(_) => {}, Insn::CPopInto(opnd) | Insn::CPush(opnd) | @@ -1149,6 +1154,7 @@ impl Insn { Insn::PatchPoint(..) => "PatchPoint", Insn::PadPatchPoint => "PadPatchPoint", Insn::PosMarker(_) => "PosMarker", + Insn::PosMarkerAtBlockEnd(_) => "PosMarkerAtBlockEnd", Insn::RShift { .. } => "RShift", Insn::Store { .. } => "Store", Insn::Sub { .. } => "Sub", @@ -1827,8 +1833,14 @@ impl Assembler } // Process each instruction, expanding branch params if needed + let mut block_end_pos_marker = None; for insn in &block.insns { - self.expand_branch_insn(insn, &mut insns); + if let Insn::PosMarkerAtBlockEnd(marker) = insn { + assert!(block_end_pos_marker.is_none(), "only one PosMarkerAtBlockEnd is supported per block"); + block_end_pos_marker = Some(marker.clone()); + } else { + self.expand_branch_insn(insn, &mut insns); + } } // Eliminate redundant jumps: if the last instruction is an @@ -1843,6 +1855,10 @@ impl Assembler } } + if let Some(marker) = block_end_pos_marker { + insns.push(Insn::PosMarker(marker)); + } + // Make sure we don't stomp on the next function if block_id.0 == num_blocks - 1 { push_insns_with_perf_symbol(&mut insns, "PadPatchPoint", |insns| { @@ -3786,6 +3802,34 @@ impl Assembler { self.push_insn(Insn::PosMarker(Rc::new(marker_fn))); } + /// Insert a marker that linearize_instructions lowers to a PosMarker at the + /// end of the current block, after its terminator instructions. + pub fn pos_marker_at_block_end(&mut self, marker_fn: impl Fn(CodePtr, &CodeBlock) + 'static) { + let block_id = self.current_block_id; + let block = &self.basic_blocks[block_id.0]; + let len = block.insns.len(); + assert!( + len > 0 && block.insns[len - 1].is_terminator(), + "pos_marker_at_block_end requires the current block to end with a terminator" + ); + + // A block may end with two terminators, such as a conditional jump + // followed by an unconditional jump. Keep the pseudo-instruction before + // the whole terminator suffix so the LIR block itself still ends with + // terminators; linearize_instructions emits the real PosMarker after the + // block's instructions. + let insert_pos = if block.insns.get(len - 2).is_some_and(Insn::is_terminator) { + len - 2 + } else { + len - 1 + }; + + self.idx += 1; + let block = &mut self.basic_blocks[block_id.0]; + block.insns.insert(insert_pos, Insn::PosMarkerAtBlockEnd(Rc::new(marker_fn))); + block.insn_ids.insert(insert_pos, None); + } + #[must_use] pub fn rshift(&mut self, opnd: Opnd, shift: Opnd) -> Opnd { let out = self.new_vreg(Opnd::match_num_bits(&[opnd, shift])); diff --git a/zjit/src/backend/x86_64/mod.rs b/zjit/src/backend/x86_64/mod.rs index 7748c2418c06f4..963a177dd8125a 100644 --- a/zjit/src/backend/x86_64/mod.rs +++ b/zjit/src/backend/x86_64/mod.rs @@ -773,6 +773,9 @@ impl Assembler { Insn::PosMarker(..) => { pos_markers.push((insn_idx, cb.get_write_ptr())); }, + Insn::PosMarkerAtBlockEnd(..) => { + unreachable!("PosMarkerAtBlockEnd should have been lowered by linearize_instructions"); + }, Insn::BakeString(text) => { for byte in text.as_bytes() { diff --git a/zjit/src/codegen.rs b/zjit/src/codegen.rs index e4175adfa79245..511fa9d34c2a13 100644 --- a/zjit/src/codegen.rs +++ b/zjit/src/codegen.rs @@ -23,12 +23,9 @@ use crate::backend::lir::{self, Assembler, C_ARG_OPNDS, C_RET_OPND, CFP, EC, NAT use crate::hir::{iseq_to_hir, BlockId, Invariant, RangeType, SideExitReason::{self, *}, SpecialBackrefSymbol, SpecialObjectType}; use crate::hir::{BlockHandler, CCallVariadicData, CCallWithFrameData, Const, FieldName, FrameState, Function, Insn, InsnId, Recompile, SendFallbackReason}; use crate::hir_type::{types, Type}; -use crate::options::{get_option, InlineDepth, PerfMap}; +use crate::options::{get_option, InlineDepth, PerfMap, DEFAULT_MAX_VERSIONS}; use crate::cast::IntoUsize; -/// Default maximum number of compiled versions per ISEQ. -const DEFAULT_MAX_VERSIONS: usize = 2; - /// Maximum number of compiled versions per ISEQ. /// Configurable via --zjit-max-versions (default: 2). pub fn max_iseq_versions() -> usize { @@ -468,10 +465,11 @@ fn gen_function(cb: &mut CodeBlock, iseq: IseqPtr, version: IseqVersionRef, func // Compile all instructions for (insn_idx, &insn_id) in block.insns().enumerate() { let insn = function.find(insn_id); + let perf_symbol = hir_perf_symbol_range_start(&mut asm, &insn); - match insn { + let result = match &insn { Insn::CondBranch { val, if_true, if_false } => { - let val_opnd = jit.get_opnd(val); + let val_opnd = jit.get_opnd(*val); let true_target = hir_to_lir[if_true.target.0].unwrap(); let false_target = hir_to_lir[if_false.target.0].unwrap(); @@ -490,6 +488,7 @@ fn gen_function(cb: &mut CodeBlock, iseq: IseqPtr, version: IseqVersionRef, func asm.jmp(Target::Block(Box::new(false_branch))); assert!(asm.current_block().insns.last().unwrap().is_terminator()); + Ok(()) } Insn::Jump(target) => { let lir_target = hir_to_lir[target.target.0].unwrap(); @@ -502,40 +501,37 @@ fn gen_function(cb: &mut CodeBlock, iseq: IseqPtr, version: IseqVersionRef, func // Jump should always be the last instruction in an HIR block assert!(insn_idx == block.insns().len() - 1, "Jump must be the last instruction in HIR block"); + Ok(()) }, _ => { - // Start a new perf range for the HIR instruction. For now, we do this only for - // non-terminator instructions because LIR blocks must end with a terminator instruction. - let perf_symbol = if get_option!(perf) == Some(PerfMap::HIR) && !insn.is_terminator() { - let insn_name = format!("{insn}").split_whitespace().next().unwrap().to_string(); - Some(perf_symbol_range_start(&mut asm, &insn_name)) - } else { - None - }; - - let result = gen_insn(cb, &mut jit, &mut asm, function, insn_id, &insn); - - // Close the current perf range for the HIR instruction. - if let Some(perf_symbol) = &perf_symbol { - perf_symbol_range_end(&mut asm, perf_symbol); - } - - if let Err(last_snapshot) = result { - debug!("ZJIT: gen_function: Failed to compile insn: {insn_id} {insn}. Generating side-exit."); - gen_incr_counter(&mut asm, exit_counter_for_unhandled_hir_insn(&insn)); - let reason = match insn { - Insn::Throw { .. } => SideExitReason::UnhandledHIRThrow, - Insn::InvokeBuiltin { .. } => SideExitReason::UnhandledHIRInvokeBuiltin, - _ => SideExitReason::UnhandledHIRUnknown(insn_id), - }; - gen_side_exit(&mut jit, &mut asm, &reason, None, &function.frame_state(last_snapshot)); - // Don't bother generating code after a side-exit. We won't run it. - // TODO(max): Generate ud2 or equivalent. - break; - }; - // It's fine; we generated the instruction + gen_insn(cb, &mut jit, &mut asm, function, insn_id, &insn) + } + }; + + // Close the current perf range for the HIR instruction. + if let Some(perf_symbol) = &perf_symbol { + if result.is_ok() && insn.is_terminator() { + assert!(asm.current_block().insns.last().is_some_and(|insn| insn.is_terminator())); + perf_symbol_range_end_at_block_end(&mut asm, perf_symbol); + } else { + perf_symbol_range_end(&mut asm, perf_symbol); } } + + if let Err(last_snapshot) = result { + debug!("ZJIT: gen_function: Failed to compile insn: {insn_id} {insn}. Generating side-exit."); + gen_incr_counter(&mut asm, exit_counter_for_unhandled_hir_insn(&insn)); + let reason = match insn { + Insn::Throw { .. } => SideExitReason::UnhandledHIRThrow, + Insn::InvokeBuiltin { .. } => SideExitReason::UnhandledHIRInvokeBuiltin, + _ => SideExitReason::UnhandledHIRUnknown(insn_id), + }; + gen_side_exit(&mut jit, &mut asm, &reason, None, &function.frame_state(last_snapshot)); + // Don't bother generating code after a side-exit. We won't run it. + // TODO(max): Generate ud2 or equivalent. + break; + }; + // It's fine; we generated the instruction } // Blocks should always end with control flow assert!(asm.current_block().insns.last().unwrap().is_terminator()); @@ -3806,6 +3802,16 @@ impl IseqCall { type PerfSymbol = Rc>>; +/// Start a HIR perf symbol range when --zjit-perf=hir is enabled. +fn hir_perf_symbol_range_start(asm: &mut Assembler, insn: &Insn) -> Option { + if get_option!(perf) == Some(PerfMap::HIR) { + let insn_name = format!("{insn}").split_whitespace().next().unwrap().to_string(); + Some(perf_symbol_range_start(asm, &insn_name)) + } else { + None + } +} + /// Mark the start of a perf symbol range via pos_marker. /// Returns a handle to pass to perf_symbol_range_end. pub fn perf_symbol_range_start(asm: &mut Assembler, symbol_name: &str) -> PerfSymbol { @@ -3832,6 +3838,23 @@ pub fn perf_symbol_range_end(asm: &mut Assembler, perf_symbol: &PerfSymbol) { }); } +/// Mark the end of a perf symbol range at the end of the current LIR block. +pub fn perf_symbol_range_end_at_block_end(asm: &mut Assembler, perf_symbol: &PerfSymbol) { + let current = perf_symbol.clone(); + asm.pos_marker_at_block_end(move |end, cb| { + if let Some((start, name)) = current.borrow_mut().take() { + let start_addr = start.raw_addr(cb); + let end_addr = end.raw_addr(cb); + // A terminator's jump can be removed when it targets the next + // linear block, leaving no code between the range start and the + // block-end marker. Skip zero-sized perf map entries. + if start_addr < end_addr { + register_with_perf(name, start_addr, end_addr - start_addr); + } + } + }); +} + #[cfg(test)] #[path = "codegen_tests.rs"] mod tests; diff --git a/zjit/src/codegen_tests.rs b/zjit/src/codegen_tests.rs index 7bc6282a3443e3..53cd1061375fc7 100644 --- a/zjit/src/codegen_tests.rs +++ b/zjit/src/codegen_tests.rs @@ -735,6 +735,26 @@ fn test_send_with_local_written_by_blockiseq() { "), @"[1, 2]"); } +#[test] +fn test_send_does_not_reload_local_untouched_by_blockiseq() { + // https://github.com/Shopify/ruby/issues/976: a call with a block must not + // reload locals the block never assigns, otherwise it reads a stale stack + // slot and clobbers the correct SSA value (here, `a`). + eval(" + def foo(&block) = 1 + + def test + a = 1 + foo {} + a + end + + test + "); + assert_contains_opcode("test", YARVINSN_send); + assert_snapshot!(assert_compiles("test"), @"1"); +} + #[test] fn test_no_ep_escape_patch_point_after_send_does_not_repeat_send() { eval(r#" diff --git a/zjit/src/cruby.rs b/zjit/src/cruby.rs index e68890fa9f368c..b798fe45d8bbf9 100644 --- a/zjit/src/cruby.rs +++ b/zjit/src/cruby.rs @@ -88,6 +88,7 @@ use std::ffi::{c_void, CString, CStr}; use std::fmt::{Debug, Display, Formatter}; use std::os::raw::{c_char, c_int, c_long, c_uint}; use std::panic::{catch_unwind, UnwindSafe}; +use std::ptr::NonNull; use crate::cast::IntoUsize as _; @@ -776,9 +777,42 @@ impl VALUE { pub type IseqParameters = rb_iseq_constant_body_rb_iseq_parameters; +/// How a block iseq refers to a variable in an enclosing scope, as recorded in +/// `ISEQ_BODY(blockiseq)->outer_variables`. `compile.c` aggregates accesses from +/// nested blocks up the chain, and the same table backs `Ractor.shareable_proc`'s +/// isolation checks. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum OuterLocalAccess { + /// The variable is read but never assigned to. + ReadOnly, + /// The variable is assigned to and maybe also read. + ReadWrite, +} + +/// Wrapper over an iseq's `outer_variables` table, which describes +/// how a block iseq refers to a variable in an enclosing scope. +#[derive(Clone, Copy)] +pub struct OuterVariables(Option>); + +impl OuterVariables { + /// Look up how the enclosing-scope local `id` is accessed by the iseq (or any + /// iseq nested within it). Returns `None` when the variable isn't referenced. + pub fn local_access(self, id: ID) -> Option { + let table = self.0?; + let mut write = Qfalse; + // Non-zero return means there's a table entry, i.e. the variable is referenced. + if unsafe { rb_id_table_lookup(table.as_ptr(), id, &mut write) } == 0 { + return None; + } + // Truthy means write + Some(if write.test() { OuterLocalAccess::ReadWrite } else { OuterLocalAccess::ReadOnly }) + } +} + /// Extension trait to enable method calls on [`IseqPtr`] pub trait IseqAccess { unsafe fn params<'a>(self) -> &'a IseqParameters; + unsafe fn outer_variables(self) -> OuterVariables; } impl IseqAccess for IseqPtr { @@ -786,6 +820,13 @@ impl IseqAccess for IseqPtr { unsafe fn params<'a>(self) -> &'a IseqParameters { unsafe { &*((*self).body.byte_add(ISEQ_BODY_OFFSET_PARAM.to_usize()) as *const IseqParameters) } } + + /// The iseq's `outer_variables` table. See [`OuterVariables`]. + unsafe fn outer_variables(self) -> OuterVariables { + use crate::cast::IntoUsize; + let field = unsafe { (*self).body.byte_add(ISEQ_BODY_OFFSET_OUTER_VARIABLES.to_usize()) } as *const *mut rb_id_table; + OuterVariables(NonNull::new(unsafe { *field })) + } } impl IseqParameters { diff --git a/zjit/src/cruby_bindings.inc.rs b/zjit/src/cruby_bindings.inc.rs index cd945e0b2bcb18..a2f71ef8fb6f6f 100644 --- a/zjit/src/cruby_bindings.inc.rs +++ b/zjit/src/cruby_bindings.inc.rs @@ -419,17 +419,10 @@ pub const BOP_LAST_: ruby_basic_operators = 35; pub type ruby_basic_operators = u32; pub type rb_serial_t = ::std::os::raw::c_ulonglong; #[repr(C)] -#[derive(Debug, Copy, Clone)] -pub struct rb_id_item { - _unused: [u8; 0], -} -#[repr(C)] +#[repr(align(8))] #[derive(Debug, Copy, Clone)] pub struct rb_id_table { - pub capa: ::std::os::raw::c_int, - pub num: ::std::os::raw::c_int, - pub used: ::std::os::raw::c_int, - pub items: *mut rb_id_item, + pub _bindgen_opaque_blob: [u64; 3usize], } pub const imemo_env: imemo_type = 0; pub const imemo_cref: imemo_type = 1; @@ -1931,6 +1924,7 @@ pub struct zjit_jit_frame { pub stack: __IncompleteArrayField, } pub const ISEQ_BODY_OFFSET_PARAM: zjit_struct_offsets = 16; +pub const ISEQ_BODY_OFFSET_OUTER_VARIABLES: zjit_struct_offsets = 288; pub type zjit_struct_offsets = u32; pub const ROBJECT_OFFSET_AS_HEAP_FIELDS: jit_bindgen_constants = 16; pub const ROBJECT_OFFSET_AS_ARY: jit_bindgen_constants = 16; @@ -2082,6 +2076,11 @@ unsafe extern "C" { ) -> VALUE; pub fn rb_vm_top_self() -> VALUE; pub static mut rb_vm_insn_count: u64; + pub fn rb_id_table_lookup( + tbl: *mut rb_id_table, + id: ID, + valp: *mut VALUE, + ) -> ::std::os::raw::c_int; pub fn rb_method_entry_at(obj: VALUE, id: ID) -> *const rb_method_entry_t; pub fn rb_callable_method_entry(klass: VALUE, id: ID) -> *const rb_callable_method_entry_t; pub fn rb_callable_method_entry_or_negative( diff --git a/zjit/src/hir.rs b/zjit/src/hir.rs index 8d6e0055089dae..de6f5e2ccccc4b 100644 --- a/zjit/src/hir.rs +++ b/zjit/src/hir.rs @@ -5225,6 +5225,62 @@ impl Function { self.push_insn(block, Insn::PatchPoint { invariant: Invariant::NoEPEscape(iseq), state: reload_exit_id }); } + /// After a call that takes a block iseq, reload the locals that the block (or any iseq nested + /// within it) may have written. This covers syntactically visible local writes where the + /// environment does not escape. Exordinary modifications through `Binding` and debug.h APIs are + /// handled via patchpoints. + fn reload_locals_modified_by_block( + &mut self, + block: BlockId, + iseq: IseqPtr, + blockiseq: IseqPtr, + state: &mut FrameState, + ep_escaped: bool, + ) { + let to_reload: &mut dyn Iterator = if ep_escaped { + // Reload everything when working with an escaped environment + &mut (0..state.locals.len()) + } else { + // When not escaped, only reload syntactically visible local modifications + let params = unsafe { iseq.params() }; + let block_param_local_idx: Option = if params.flags.has_block() != 0 { + params.block_start.try_into().ok() + } else { + None + }; + let outer_variables = unsafe { blockiseq.outer_variables() }; + &mut (0..state.locals.len()).filter(move |&local_idx| { + let id = unsafe { rb_zjit_local_id(iseq, local_idx.try_into().unwrap()) }; + let access = outer_variables.local_access(id); + if block_param_local_idx == Some(local_idx) { + // The block param slot is special: `getblockparam` come from a syntactic read, + // but operationally can write to the local slot. So, reload it whenever the + // block references it at all (read or write), not just on a setlocal. + access.is_some() + } else { + access == Some(OuterLocalAccess::ReadWrite) + } + }) + }; + + let mut base: Option = None; + for local_idx in to_reload { + let ep_offset = local_idx_to_ep_offset(iseq, local_idx); + let ep_offset_u32 = u32::try_from(ep_offset) + .unwrap_or_else(|_| panic!("Could not convert ep_offset {ep_offset} to u32")); + let recv = *base.get_or_insert_with(|| { + let base_insn = if !ep_escaped { Insn::LoadSP } else { Insn::GetEP { level: 0 } }; + self.push_insn(block, base_insn) + }); + let val = if !ep_escaped { + self.get_local_from_sp(block, iseq, recv, ep_offset_u32, types::BasicObject) + } else { + self.get_local_from_ep(block, iseq, recv, ep_offset_u32, 0, types::BasicObject) + }; + state.setlocal(ep_offset_u32, val); + } + } + fn count_not_inlined_cfunc(&mut self, block: BlockId, cme: *const rb_callable_method_entry_t) { let owner = unsafe { (*cme).owner }; let called_id = unsafe { (*cme).called_id }; @@ -8606,28 +8662,12 @@ fn add_iseq_to_hir( let send = fun.push_insn(block, Insn::Send { recv, cd, block: block_handler, args, state: exit_id, reason: Uncategorized(opcode) }); state.stack_push(send); - if let Some(BlockHandler::BlockIseq(_)) = block_handler { + if let Some(BlockHandler::BlockIseq(blockiseq)) = block_handler { // Reload locals that may have been modified by the blockiseq. - // TODO: Avoid reloading locals that are not referenced by the blockiseq - // or not used after this. Max thinks we could eventually DCE them. if !ep_escaped && !state.locals.is_empty() { fun.gen_post_send_no_ep_escape_patch_point(block, &state, insn_idx); } - let mut base: Option = None; - for local_idx in 0..state.locals.len() { - let ep_offset = local_idx_to_ep_offset(iseq, local_idx); - let ep_offset_u32 = u32::try_from(ep_offset) - .unwrap_or_else(|_| panic!("Could not convert ep_offset {ep_offset} to u32")); - let recv = *base.get_or_insert_with(|| { - if !ep_escaped { fun.load_sp(block) } else { fun.get_ep(block, 0) } - }); - let val = if !ep_escaped { - fun.get_local_from_sp(block, iseq, recv, ep_offset_u32, types::BasicObject) - } else { - fun.get_local_from_ep(block, iseq, recv, ep_offset_u32, 0, types::BasicObject) - }; - state.setlocal(ep_offset_u32, val); - } + fun.reload_locals_modified_by_block(block, iseq, blockiseq, &mut state, ep_escaped); } } YARVINSN_sendforward => { @@ -8658,21 +8698,7 @@ fn add_iseq_to_hir( if !ep_escaped && !state.locals.is_empty() { fun.gen_post_send_no_ep_escape_patch_point(block, &state, insn_idx); } - let mut base: Option = None; - for local_idx in 0..state.locals.len() { - let ep_offset = local_idx_to_ep_offset(iseq, local_idx); - let ep_offset_u32 = u32::try_from(ep_offset) - .unwrap_or_else(|_| panic!("Could not convert ep_offset {ep_offset} to u32")); - let recv = *base.get_or_insert_with(|| { - if !ep_escaped { fun.load_sp(block) } else { fun.get_ep(block, 0) } - }); - let val = if !ep_escaped { - fun.get_local_from_sp(block, iseq, recv, ep_offset_u32, types::BasicObject) - } else { - fun.get_local_from_ep(block, iseq, recv, ep_offset_u32, 0, types::BasicObject) - }; - state.setlocal(ep_offset_u32, val); - } + fun.reload_locals_modified_by_block(block, iseq, blockiseq, &mut state, ep_escaped); } } YARVINSN_invokesuper => { @@ -8697,26 +8723,10 @@ fn add_iseq_to_hir( if !blockiseq.is_null() { // Reload locals that may have been modified by the blockiseq. - // TODO: Avoid reloading locals that are not referenced by the blockiseq - // or not used after this. Max thinks we could eventually DCE them. if !ep_escaped && !state.locals.is_empty() { fun.gen_post_send_no_ep_escape_patch_point(block, &state, insn_idx); } - let mut base: Option = None; - for local_idx in 0..state.locals.len() { - let ep_offset = local_idx_to_ep_offset(iseq, local_idx); - let ep_offset_u32 = u32::try_from(ep_offset) - .unwrap_or_else(|_| panic!("Could not convert ep_offset {ep_offset} to u32")); - let recv = *base.get_or_insert_with(|| { - if !ep_escaped { fun.load_sp(block) } else { fun.get_ep(block, 0) } - }); - let val = if !ep_escaped { - fun.get_local_from_sp(block, iseq, recv, ep_offset_u32, types::BasicObject) - } else { - fun.get_local_from_ep(block, iseq, recv, ep_offset_u32, 0, types::BasicObject) - }; - state.setlocal(ep_offset_u32, val); - } + fun.reload_locals_modified_by_block(block, iseq, blockiseq, &mut state, ep_escaped); } } YARVINSN_invokesuperforward => { @@ -8743,26 +8753,10 @@ fn add_iseq_to_hir( if !blockiseq.is_null() { // Reload locals that may have been modified by the blockiseq. - // TODO: Avoid reloading locals that are not referenced by the blockiseq - // or not used after this. Max thinks we could eventually DCE them. if !ep_escaped && !state.locals.is_empty() { fun.gen_post_send_no_ep_escape_patch_point(block, &state, insn_idx); } - let mut base: Option = None; - for local_idx in 0..state.locals.len() { - let ep_offset = local_idx_to_ep_offset(iseq, local_idx); - let ep_offset_u32 = u32::try_from(ep_offset) - .unwrap_or_else(|_| panic!("Could not convert ep_offset {ep_offset} to u32")); - let recv = *base.get_or_insert_with(|| { - if !ep_escaped { fun.load_sp(block) } else { fun.get_ep(block, 0) } - }); - let val = if !ep_escaped { - fun.get_local_from_sp(block, iseq, recv, ep_offset_u32, types::BasicObject) - } else { - fun.get_local_from_ep(block, iseq, recv, ep_offset_u32, 0, types::BasicObject) - }; - state.setlocal(ep_offset_u32, val); - } + fun.reload_locals_modified_by_block(block, iseq, blockiseq, &mut state, ep_escaped); } } YARVINSN_invokeblock => { diff --git a/zjit/src/hir/opt_tests.rs b/zjit/src/hir/opt_tests.rs index 40fba2aa2ffd8c..bebf7d693dceb0 100644 --- a/zjit/src/hir/opt_tests.rs +++ b/zjit/src/hir/opt_tests.rs @@ -1579,13 +1579,11 @@ mod hir_opt_tests { bb3(v9:BasicObject, v10:BasicObject): PatchPoint NoSingletonClass(C@0x1008) PatchPoint MethodRedefined(C@0x1008, fun_new_map@0x1010, cme:0x1018) - v27:ArraySubclass[class_exact:C] = GuardType v10, ArraySubclass[class_exact:C] recompile - v28:BasicObject = SendDirect v27, 0x1040, :fun_new_map (0x1050) + v25:ArraySubclass[class_exact:C] = GuardType v10, ArraySubclass[class_exact:C] recompile + v26:BasicObject = SendDirect v25, 0x1040, :fun_new_map (0x1050) PatchPoint NoEPEscape(test) - v18:CPtr = LoadSP - v19:BasicObject = LoadField v18, :o@0x1000 CheckInterrupts - Return v28 + Return v26 "); } @@ -1616,13 +1614,11 @@ mod hir_opt_tests { bb3(v9:BasicObject, v10:BasicObject): PatchPoint NoSingletonClass(C@0x1008) PatchPoint MethodRedefined(C@0x1008, bar@0x1010, cme:0x1018) - v28:ObjectSubclass[class_exact:C] = GuardType v10, ObjectSubclass[class_exact:C] recompile - v29:BasicObject = CCallWithFrame v28, :Enumerable#bar@0x1040, block=0x1048 + v26:ObjectSubclass[class_exact:C] = GuardType v10, ObjectSubclass[class_exact:C] recompile + v27:BasicObject = CCallWithFrame v26, :Enumerable#bar@0x1040, block=0x1048 PatchPoint NoEPEscape(test) - v18:CPtr = LoadSP - v19:BasicObject = LoadField v18, :o@0x1000 CheckInterrupts - Return v29 + Return v27 "); } @@ -3860,7 +3856,7 @@ mod hir_opt_tests { def foo(&block) = 1 def test a = 1 - foo {|| } + foo {|| a = 2 } a end test @@ -3898,7 +3894,7 @@ mod hir_opt_tests { def test a = 1 lambda { a } - foo {|| } + foo {|| a = 2 } a end test @@ -4562,11 +4558,8 @@ mod hir_opt_tests { v22:TrueClass = Const Value(true) v24:BasicObject = Send v12, 0x1008, :each_line, v22 # SendFallbackReason: Complex argument passing PatchPoint NoEPEscape(test) - v27:CPtr = LoadSP - v28:BasicObject = LoadField v27, :s@0x1000 - v29:BasicObject = LoadField v27, :a@0x1030 CheckInterrupts - Return v29 + Return v17 "); } @@ -9476,12 +9469,10 @@ mod hir_opt_tests { v23:ArrayExact[VALUE(0x1018)] = Const Value(VALUE(0x1018)) PatchPoint NoSingletonClass(Array@0x1020) PatchPoint MethodRedefined(Array@0x1020, zip@0x1028, cme:0x1030) - v44:BasicObject = CCallVariadic v19, :Array#zip@0x1058, v23 + v42:BasicObject = CCallVariadic v19, :Array#zip@0x1058, v23 PatchPoint NoEPEscape(test) - v28:CPtr = LoadSP - v29:BasicObject = LoadField v28, :result@0x1060 CheckInterrupts - Return v29 + Return v13 "); } @@ -9641,6 +9632,7 @@ mod hir_opt_tests { #[test] fn test_send_iseq_with_block_param_no_block() { + set_max_versions(2); let result = eval(" def foo(&blk) blk ? blk.call : 42 @@ -17036,6 +17028,7 @@ mod hir_opt_tests { // the HIR. The auto-compile creates version 1, and hir_string() creates version 2 // (= MAX_ISEQ_VERSIONS), so this is the final version. set_call_threshold(3); + set_max_versions(2); set_inline_threshold(0); eval(" @@ -17903,6 +17896,7 @@ mod hir_opt_tests { #[test] fn test_trigger_guard_type_recompilation() { + set_max_versions(2); set_inline_threshold(0); eval(" class C @@ -19091,13 +19085,13 @@ mod hir_opt_tests { Jump bb3(v6, v7) bb3(v9:BasicObject, v10:BasicObject): PatchPoint MethodRedefined(Object@0x1008, with_yield@0x1010, cme:0x1018) - v27:ObjectSubclass[class_exact*:Object@VALUE(0x1008)] = GuardType v9, ObjectSubclass[class_exact*:Object@VALUE(0x1008)] recompile - PushInlineFrame v27 (0x1040), v10 - v35:BasicObject = InvokeBlock v10 # SendFallbackReason: InvokeBlock: not yet specialized + v25:ObjectSubclass[class_exact*:Object@VALUE(0x1008)] = GuardType v9, ObjectSubclass[class_exact*:Object@VALUE(0x1008)] recompile + PushInlineFrame v25 (0x1040), v10 + v33:BasicObject = InvokeBlock v10 # SendFallbackReason: InvokeBlock: not yet specialized CheckInterrupts PopInlineFrame PatchPoint NoEPEscape(test) - Return v35 + Return v33 "); } @@ -19143,27 +19137,27 @@ mod hir_opt_tests { Jump bb3(v6, v7) bb3(v9:BasicObject, v10:BasicObject): PatchPoint MethodRedefined(Object@0x1008, with_block_param@0x1010, cme:0x1018) - v27:ObjectSubclass[class_exact*:Object@VALUE(0x1008)] = GuardType v9, ObjectSubclass[class_exact*:Object@VALUE(0x1008)] recompile - v54:NilClass = Const Value(nil) - PushInlineFrame v27 (0x1040), v10 - v37:CPtr = GetEP 0 - v38:CUInt64 = LoadField v37, :VM_ENV_DATA_INDEX_FLAGS@0x1048 - v39:CBool = IsBlockParamModified v38 - CondBranch v39, bb6(), bb7() + v25:ObjectSubclass[class_exact*:Object@VALUE(0x1008)] = GuardType v9, ObjectSubclass[class_exact*:Object@VALUE(0x1008)] recompile + v52:NilClass = Const Value(nil) + PushInlineFrame v25 (0x1040), v10 + v35:CPtr = GetEP 0 + v36:CUInt64 = LoadField v35, :VM_ENV_DATA_INDEX_FLAGS@0x1048 + v37:CBool = IsBlockParamModified v36 + CondBranch v37, bb6(), bb7() bb6(): - v41:BasicObject = LoadField v37, :block@0x1049 - Jump bb8(v41, v41) + v39:BasicObject = LoadField v35, :block@0x1049 + Jump bb8(v39, v39) bb7(): - v43:CInt64 = LoadField v37, :VM_ENV_DATA_INDEX_SPECVAL@0x104a - v44:CInt64 = GuardAnyBitSet v43, CUInt64(1) recompile - v45:ObjectSubclass[BlockParamProxy] = Const Value(VALUE(0x1050)) - Jump bb8(v45, v54) - bb8(v35:BasicObject, v36:BasicObject): - v49:BasicObject = Send v35, :call, v10 # SendFallbackReason: SendWithoutBlock: unsupported optimized method type BlockCall + v41:CInt64 = LoadField v35, :VM_ENV_DATA_INDEX_SPECVAL@0x104a + v42:CInt64 = GuardAnyBitSet v41, CUInt64(1) recompile + v43:ObjectSubclass[BlockParamProxy] = Const Value(VALUE(0x1050)) + Jump bb8(v43, v52) + bb8(v33:BasicObject, v34:BasicObject): + v47:BasicObject = Send v33, :call, v10 # SendFallbackReason: SendWithoutBlock: unsupported optimized method type BlockCall CheckInterrupts PopInlineFrame PatchPoint NoEPEscape(test) - Return v49 + Return v47 "); } @@ -19207,27 +19201,27 @@ mod hir_opt_tests { Jump bb3(v6, v7) bb3(v9:BasicObject, v10:BasicObject): PatchPoint MethodRedefined(Object@0x1008, callee@0x1010, cme:0x1018) - v27:ObjectSubclass[class_exact*:Object@VALUE(0x1008)] = GuardType v9, ObjectSubclass[class_exact*:Object@VALUE(0x1008)] recompile - v55:NilClass = Const Value(nil) - PushInlineFrame v27 (0x1040), v10 - v39:CPtr = GetEP 0 - v40:CUInt64 = LoadField v39, :VM_ENV_DATA_INDEX_FLAGS@0x1048 - v41:CBool = IsBlockParamModified v40 - CondBranch v41, bb6(), bb7() + v25:ObjectSubclass[class_exact*:Object@VALUE(0x1008)] = GuardType v9, ObjectSubclass[class_exact*:Object@VALUE(0x1008)] recompile + v53:NilClass = Const Value(nil) + PushInlineFrame v25 (0x1040), v10 + v37:CPtr = GetEP 0 + v38:CUInt64 = LoadField v37, :VM_ENV_DATA_INDEX_FLAGS@0x1048 + v39:CBool = IsBlockParamModified v38 + CondBranch v39, bb6(), bb7() bb6(): - v43:BasicObject = LoadField v39, :block@0x1049 - Jump bb8(v43, v43) + v41:BasicObject = LoadField v37, :block@0x1049 + Jump bb8(v41, v41) bb7(): - v45:CInt64 = LoadField v39, :VM_ENV_DATA_INDEX_SPECVAL@0x104a - v46:CInt64 = GuardAnyBitSet v45, CUInt64(1) recompile - v47:ObjectSubclass[BlockParamProxy] = Const Value(VALUE(0x1050)) - Jump bb8(v47, v55) - bb8(v37:BasicObject, v38:BasicObject): - v50:BasicObject = Send v27, &block, :inner, v10, v37 # SendFallbackReason: Complex argument passing + v43:CInt64 = LoadField v37, :VM_ENV_DATA_INDEX_SPECVAL@0x104a + v44:CInt64 = GuardAnyBitSet v43, CUInt64(1) recompile + v45:ObjectSubclass[BlockParamProxy] = Const Value(VALUE(0x1050)) + Jump bb8(v45, v53) + bb8(v35:BasicObject, v36:BasicObject): + v48:BasicObject = Send v25, &block, :inner, v10, v35 # SendFallbackReason: Complex argument passing CheckInterrupts PopInlineFrame PatchPoint NoEPEscape(test) - Return v50 + Return v48 "); } diff --git a/zjit/src/hir/tests.rs b/zjit/src/hir/tests.rs index c90b05e5f59deb..cdb19ddb3b51e1 100644 --- a/zjit/src/hir/tests.rs +++ b/zjit/src/hir/tests.rs @@ -2030,13 +2030,264 @@ pub(crate) mod hir_build_tests { bb3(v9:BasicObject, v10:BasicObject): v15:BasicObject = Send v10, 0x1008, :each # SendFallbackReason: Uncategorized(send) PatchPoint NoEPEscape(test) - v18:CPtr = LoadSP - v19:BasicObject = LoadField v18, :a@0x1000 CheckInterrupts Return v15 "); } + #[test] + fn test_send_with_block_reloads_only_written_locals() { + eval(" + def foo = yield + def test + a = 1 + b = 2 + foo { a = 3 } + a + b + end + test + "); + // Only `a` is reloaded after the call; `b` is never written by the block, + // so it keeps its SSA value (the Fixnum constant) and is not reloaded. + assert_snapshot!(hir_string("test"), @" + fn test@:4: + bb1(): + EntryPoint interpreter + v1:BasicObject = LoadSelf + v2:NilClass = Const Value(nil) + v3:NilClass = Const Value(nil) + Jump bb3(v1, v2, v3) + bb2(): + EntryPoint JIT(0) + v6:BasicObject = LoadArg :self@0 + v7:NilClass = Const Value(nil) + v8:NilClass = Const Value(nil) + Jump bb3(v6, v7, v8) + bb3(v10:BasicObject, v11:NilClass, v12:NilClass): + v16:Fixnum[1] = Const Value(1) + v20:Fixnum[2] = Const Value(2) + v25:BasicObject = Send v10, 0x1000, :foo # SendFallbackReason: Uncategorized(send) + PatchPoint NoEPEscape(test) + v28:CPtr = LoadSP + v29:BasicObject = LoadField v28, :a@0x1028 + PatchPoint NoEPEscape(test) + v38:BasicObject = Send v29, :+, v20 # SendFallbackReason: Uncategorized(opt_plus) + CheckInterrupts + Return v38 + "); + } + + #[test] + fn test_send_with_block_does_not_reload_read_only_local() { + eval(" + def foo = yield + def test + a = 1 + foo { a } + a + end + test + "); + // The block only reads `a`; it never assigns it, so `a` keeps its SSA value + // and is not reloaded after the call. + assert_snapshot!(hir_string("test"), @" + fn test@:4: + bb1(): + EntryPoint interpreter + v1:BasicObject = LoadSelf + v2:NilClass = Const Value(nil) + Jump bb3(v1, v2) + bb2(): + EntryPoint JIT(0) + v5:BasicObject = LoadArg :self@0 + v6:NilClass = Const Value(nil) + Jump bb3(v5, v6) + bb3(v8:BasicObject, v9:NilClass): + v13:Fixnum[1] = Const Value(1) + v18:BasicObject = Send v8, 0x1000, :foo # SendFallbackReason: Uncategorized(send) + PatchPoint NoEPEscape(test) + PatchPoint NoEPEscape(test) + CheckInterrupts + Return v13 + "); + } + + #[test] + fn test_send_reloads_referenced_block_param() { + eval(" + def take(x) = x + def consume = yield + def test(&block) + consume { take(block) } + ::RubyVM::ZJIT.induce_side_exit! + end + test { 1 } + "); + // The block reads `block` (passed as a regular argument), so it references the + // block param. `getblockparam` is recorded as a read, but reading the block param + // materializes the captured block into its slot, so the block param must be + // reloaded after the call (this is the lazy_load_hooks miscompile scenario). + assert_snapshot!(hir_string("test"), @" + fn test@:5: + bb1(): + EntryPoint interpreter + v1:BasicObject = LoadSelf + v2:CPtr = LoadSP + v3:BasicObject = LoadField v2, :block@0x1000 + Jump bb3(v1, v3) + bb2(): + EntryPoint JIT(0) + v6:BasicObject = LoadArg :self@0 + v7:BasicObject = LoadArg :block@1 + Jump bb3(v6, v7) + bb3(v9:BasicObject, v10:BasicObject): + v15:BasicObject = Send v9, 0x1008, :consume # SendFallbackReason: Uncategorized(send) + PatchPoint NoEPEscape(test) + v18:CPtr = LoadSP + v19:BasicObject = LoadField v18, :block@0x1000 + PatchPoint SingleRactorMode + PatchPoint StableConstantNames(0x1030, ::RubyVM::ZJIT) + v25:ModuleSubclass[RubyVM::ZJIT@0x1038] = Const Value(VALUE(0x1038)) + SideExit DirectiveInduced + "); + } + + #[test] + fn test_send_does_not_reload_unreferenced_block_param() { + eval(" + def consume = yield + def test(&block) + a = 1 + consume { a } + ::RubyVM::ZJIT.induce_side_exit! + end + test { 1 } + "); + // The block only references `a`, never the block param, so the block param + // cannot have been materialized by the call and is not reloaded. (Before the + // reload filter was refined, the block param was reloaded after every + // send-with-block, even when the block could not have touched it.) + assert_snapshot!(hir_string("test"), @" + fn test@:4: + bb1(): + EntryPoint interpreter + v1:BasicObject = LoadSelf + v2:CPtr = LoadSP + v3:BasicObject = LoadField v2, :block@0x1000 + v4:NilClass = Const Value(nil) + Jump bb3(v1, v3, v4) + bb2(): + EntryPoint JIT(0) + v7:BasicObject = LoadArg :self@0 + v8:BasicObject = LoadArg :block@1 + v9:NilClass = Const Value(nil) + Jump bb3(v7, v8, v9) + bb3(v11:BasicObject, v12:BasicObject, v13:NilClass): + v17:Fixnum[1] = Const Value(1) + v22:BasicObject = Send v11, 0x1008, :consume # SendFallbackReason: Uncategorized(send) + PatchPoint NoEPEscape(test) + PatchPoint SingleRactorMode + PatchPoint StableConstantNames(0x1030, ::RubyVM::ZJIT) + v30:ModuleSubclass[RubyVM::ZJIT@0x1038] = Const Value(VALUE(0x1038)) + SideExit DirectiveInduced + "); + } + + #[test] + fn test_send_with_anonymous_block_param() { + eval(" + def consume = yield + def test(&) + consume { consume(&) } + consume(&) + end + test { 1 } + "); + assert_contains_opcode("test", YARVINSN_send); + // An anonymous `&` block param can only be forwarded with `&`, which compiles to + // `getblockparamproxy` and reads the block from the EP. It never materializes the + // param into its slot, so the block param is read directly from the EP after the + // call and is not reloaded -- there is nothing a reload could recover. + assert_snapshot!(hir_string("test"), @" + fn test@:4: + bb1(): + EntryPoint interpreter + v1:BasicObject = LoadSelf + v2:CPtr = LoadSP + v3:BasicObject = LoadField v2, :&@0x1000 + Jump bb3(v1, v3) + bb2(): + EntryPoint JIT(0) + v6:BasicObject = LoadArg :self@0 + v7:BasicObject = LoadArg :&@1 + Jump bb3(v6, v7) + bb3(v9:BasicObject, v10:BasicObject): + v15:BasicObject = Send v9, 0x1008, :consume # SendFallbackReason: Uncategorized(send) + PatchPoint NoEPEscape(test) + v24:CPtr = GetEP 0 + v25:CUInt64 = LoadField v24, :VM_ENV_DATA_INDEX_FLAGS@0x1030 + v26:CBool = IsBlockParamModified v25 + CondBranch v26, bb4(), bb5() + bb4(): + v28:BasicObject = LoadField v24, :&@0x1031 + Jump bb6(v28, v28) + bb5(): + v30:CInt64 = LoadField v24, :VM_ENV_DATA_INDEX_SPECVAL@0x1032 + v31:CInt64 = GuardAnyBitSet v30, CUInt64(1) recompile + v32:ObjectSubclass[BlockParamProxy] = Const Value(VALUE(0x1038)) + Jump bb6(v32, v10) + bb6(v22:BasicObject, v23:BasicObject): + v35:BasicObject = Send v9, &block, :consume, v22 # SendFallbackReason: Uncategorized(send) + CheckInterrupts + Return v35 + "); + } + + #[test] + fn test_send_reloads_local_written_by_nested_block() { + eval(" + def foo = yield + def test + a = 1 + b = 2 + foo { foo { a = 3 } } + a + b + end + test + "); + assert_contains_opcode("test", YARVINSN_send); + // `a` is assigned only from a block nested inside the block argument, but the + // outer block's outer_variables table still records that write (the compiler + // aggregates writes up the nesting chain), so `a` is reloaded while the + // untouched `b` keeps its SSA value. + assert_snapshot!(hir_string("test"), @" + fn test@:4: + bb1(): + EntryPoint interpreter + v1:BasicObject = LoadSelf + v2:NilClass = Const Value(nil) + v3:NilClass = Const Value(nil) + Jump bb3(v1, v2, v3) + bb2(): + EntryPoint JIT(0) + v6:BasicObject = LoadArg :self@0 + v7:NilClass = Const Value(nil) + v8:NilClass = Const Value(nil) + Jump bb3(v6, v7, v8) + bb3(v10:BasicObject, v11:NilClass, v12:NilClass): + v16:Fixnum[1] = Const Value(1) + v20:Fixnum[2] = Const Value(2) + v25:BasicObject = Send v10, 0x1000, :foo # SendFallbackReason: Uncategorized(send) + PatchPoint NoEPEscape(test) + v28:CPtr = LoadSP + v29:BasicObject = LoadField v28, :a@0x1028 + PatchPoint NoEPEscape(test) + v38:BasicObject = Send v29, :+, v20 # SendFallbackReason: Uncategorized(opt_plus) + CheckInterrupts + Return v38 + "); + } + #[test] fn test_intern_interpolated_symbol() { eval(r#" @@ -2323,8 +2574,6 @@ pub(crate) mod hir_build_tests { bb3(v9:BasicObject, v10:BasicObject): v16:BasicObject = InvokeSuperForward v9, 0x1008, v10 # SendFallbackReason: InvokeSuperForward: not yet specialized PatchPoint NoEPEscape(test) - v19:CPtr = LoadSP - v20:BasicObject = LoadField v19, :...@0x1000 CheckInterrupts Return v16 "); diff --git a/zjit/src/invariants.rs b/zjit/src/invariants.rs index 233ffc21a7903d..22cd3d2c73e852 100644 --- a/zjit/src/invariants.rs +++ b/zjit/src/invariants.rs @@ -12,7 +12,7 @@ use crate::gc::remove_gc_offsets; macro_rules! compile_patch_points { ($cb:expr, $patch_points:expr, $cause:ident, $($comment_args:tt)*) => { - trace_invalidation(&format!($($comment_args)*), || with_time_stat(invalidation_time_ns, || { + trace_invalidation(|| format!($($comment_args)*), || with_time_stat(invalidation_time_ns, || { for patch_point in $patch_points { let written_range = $cb.with_write_ptr(patch_point.patch_point_ptr, |cb| { let mut asm = Assembler::new(); diff --git a/zjit/src/options.rs b/zjit/src/options.rs index fecc0fc3977549..2bb65cf526128a 100644 --- a/zjit/src/options.rs +++ b/zjit/src/options.rs @@ -15,6 +15,9 @@ pub enum PerfMap { HIR, } +/// Default maximum number of compiled versions per ISEQ. +pub const DEFAULT_MAX_VERSIONS: usize = 4; + /// Default --zjit-num-profiles const DEFAULT_NUM_PROFILES: NumProfiles = 5; pub type NumProfiles = u16; @@ -216,7 +219,7 @@ impl Default for Options { perf: None, allowed_iseqs: None, log_compiled_iseqs: None, - max_versions: 2, + max_versions: DEFAULT_MAX_VERSIONS, inline_threshold: DEFAULT_INLINE_THRESHOLD, inline_budget: DEFAULT_INLINE_BUDGET as InlineBudget, inline_deny: HashSet::new(), @@ -638,6 +641,14 @@ pub fn set_call_threshold(call_threshold: CallThreshold) { update_profile_threshold(); } +/// Update --zjit-max-versions for testing +#[cfg(test)] +pub fn set_max_versions(max_versions: usize) { + rb_zjit_prepare_options(); + unsafe { OPTIONS.as_mut().unwrap().max_versions = max_versions; } +} + +/// Update --zjit-inline-threshold for testing #[cfg(test)] pub fn set_inline_threshold(inline_threshold: InlineThreshold) { rb_zjit_prepare_options(); diff --git a/zjit/src/state.rs b/zjit/src/state.rs index da09d09314756b..18b6157f91eae7 100644 --- a/zjit/src/state.rs +++ b/zjit/src/state.rs @@ -498,23 +498,26 @@ pub extern "C" fn rb_zjit_record_exit_stack(reason: *const std::ffi::c_char) { /// Wrap a closure in a Perfetto duration event with category "invalidation" /// and a Ruby backtrace captured on the begin event. -pub fn trace_invalidation(reason: &str, func: F) -> R where F: FnOnce() -> R { +pub fn trace_invalidation(reason_func: RF, func: F) -> R + where RF: FnOnce() -> String, F: FnOnce() -> R { if !get_option!(trace_invalidation) { return func(); } // Capture backtrace and emit begin event before patching let frames = capture_ruby_frames(); + let mut reason: String = "".into(); if let Some(tracer) = ZJITState::get_tracer() { let ts = tracer.elapsed_ns(); - tracer.write_duration_begin("invalidation", reason, ts, &frames); + reason = reason_func(); + tracer.write_duration_begin("invalidation", &reason, ts, &frames); } let result = func(); if let Some(tracer) = ZJITState::get_tracer() { let ts = tracer.elapsed_ns(); - tracer.write_duration_end("invalidation", reason, ts); + tracer.write_duration_end("invalidation", &reason, ts); } result }