diff --git a/majit/majit-metainterp/src/blackhole.rs b/majit/majit-metainterp/src/blackhole.rs index f53427fd3ad..e2dcb3bada0 100644 --- a/majit/majit-metainterp/src/blackhole.rs +++ b/majit/majit-metainterp/src/blackhole.rs @@ -7346,13 +7346,9 @@ fn portal_jd_for(bh: &BlackholeInterpreter) -> Option { /// crate, and the split is what let a substituted runner be validated against /// the owning driver's declared kind. /// -/// ★ The substitution is the common case, not a fault. `codewriter.rs` stamps a -/// driver index on every portal jitcode (`jitdriver_sd_from_portal_graph` -/// matches `jd.portal_graph == code`, and `setup_jitdriver` appends one entry -/// per portal graph), while `eval.rs` registers exactly one -/// `handle_jitexc_from_bh` — index 0's. So most portal frames name a driver -/// that has no runner of its own, and refusing to substitute would withdraw -/// portal re-entry from paths that have it today. +/// ★ A miss still falls back to any registered runner so a test fixture +/// that stamps a driver index without installing a hook keeps working. +/// Production registers one hook per driver (`eval.rs` jd0 and jd1). /// /// What is withheld instead is the *kind*: a substituted runner's outcome is /// not validated against the owning driver's `result_type`, because that pair diff --git a/majit/majit-metainterp/src/jitdriver.rs b/majit/majit-metainterp/src/jitdriver.rs index e9ffd9c3bc2..8be97f0d521 100644 --- a/majit/majit-metainterp/src/jitdriver.rs +++ b/majit/majit-metainterp/src/jitdriver.rs @@ -1384,6 +1384,14 @@ pub struct JitDriverStaticData { /// unrelated jitcodes that decode at the same pc and silently return a /// mistyped count. A successful decode is not evidence of the right store. pub frame_value_count_fn: Option usize>, + /// warmstate.py `WarmEnterState.get_unique_id(greenkey)`. + /// + /// `newframe` records `ENTER_PORTAL_FRAME(jd_no, unique_id)` from this + /// hook when a greenkey is present. The greens are the typed key's + /// values in declaration order — `interp_jit.py get_unique_id(next_instr, + /// is_being_profiled, bytecode)`. `None` leaves the portal-frame unique + /// id as the greenkey hash, the previous fallback. + pub get_unique_id: Option i64>, } impl JitDriverStaticData { @@ -1465,6 +1473,7 @@ impl JitDriverStaticData { assembler_helper_adr: 0, vable_token_descr: None, frame_value_count_fn: None, + get_unique_id: None, }; // warmspot.py:529/538 — keep `index_of_virtualizable` in sync // with the `virtualizable_arg_index()` derived from `reds`. @@ -2773,6 +2782,19 @@ impl JitDriver { self.meta.is_tracing() } + /// warmstate.py `cell.flags & JC_TRACING` for this green key. + /// + /// `maybe_compile_and_run` skips only a cell that is already being + /// traced in an outer invocation of the same greens. A live session + /// on another key or another driver does not suppress this cell. + #[inline] + pub fn cell_is_tracing(&self, green_key: u64) -> bool { + self.meta + .warm_state_ref() + .get_cell(green_key) + .is_some_and(|cell| cell.is_tracing()) + } + /// Single-pass tracing: take the `(walk_final_pc, walk_final_reds)` /// snapshot captured at a terminal trace transition. CloseLoop and Finish /// publish it before draining the ctx; a fresh TraceAction::Abort publishes @@ -4850,7 +4872,9 @@ impl JitDriver { // (PyreMetaInterp::step_inline_frame pops the inline frame and // records the CALL_ASSEMBLER); it never reaches the jitdriver. // Defensive: treat an escaped instance as a plain Abort. - action @ (TraceAction::RecursiveCallAssembler { .. } | TraceAction::Abort) => { + action @ (TraceAction::RecursiveCallAssembler { .. } + | TraceAction::Abort + | TraceAction::SwitchToBlackhole(_)) => { if self.meta.bridge_info().is_some() { crate::debug::log_one("jit-abort", "Abort during bridge tracing"); } @@ -4901,60 +4925,42 @@ impl JitDriver { // by bridge symbolic init before the body walker runs. self.meta.record_declined_bridge_guard(&source_descr); } - // `pyjitpl.py:2491` `except SwitchToBlackhole as stb: - // self.aborted_tracing(stb.reason)` — the - // `aborted_tracing(reason)` accounting half of RPython's - // abort handling. Dispatch-side - // `finalize_standard_virtualizable_may_force` stashes - // `SwitchToBlackhole(ABORT_ESCAPE, raising_exception=True)` - // (`pyjitpl.py:3389-3390`) on - // `TraceCtx::pending_switch_to_blackhole`; drain it - // here so `stb.reason` flows into - // `aborted_tracing(reason)`. Falls back to the - // `blackhole_if_trace_too_long`-derived - // `AbortReason::Generic` for the - // `SwitchToBlackhole(ABORT_TOO_LONG)` - // path (`pyjitpl.py:2807`). - // - // The other half of `pyjitpl.py:2949 - // run_blackhole_interp_to_cancel_tracing(stb)` — - // `convert_and_run_from_pyjitpl(self, stb.raising_exception)` - // — is staged below into `MetaInterp::pending_abort_blackhole` - // and run by [`JitDriver::run_pending_abort_blackhole`], which - // is the first point that also holds native `state`. Both - // halves read the same drained `stb`, so take it whole rather - // than just its reason. - let pending_stb = self - .meta - .tracing - .as_mut() - .and_then(|t| t.pending_switch_to_blackhole.take()); + // pyjitpl.py `run_blackhole_interp_to_cancel_tracing(stb)` + // consumes both the reason and raising_exception from the + // same signal. The Python worker returns it with the action; + // legacy dispatch sites still put it on the trace context. + let pending_stb = match &action { + TraceAction::SwitchToBlackhole(stb) => Some(*stb), + _ => self + .meta + .tracing + .as_mut() + .and_then(|t| t.pending_switch_to_blackhole.take()), + }; // `history.py:37-43`: false unless the abort site raised at a // point where `last_exc_value` still has to be raised // (`pyjitpl.py:3389-3390` vable escape, `pyjitpl.py:3714-3716` - // `do_not_in_trace_call`). The `ABORT_TOO_LONG` path below has - // no `stb` at all and defaults to false, as upstream's - // `SwitchToBlackhole(ABORT_TOO_LONG)` does. + // `do_not_in_trace_call`). A too-long signal carries false, + // as upstream's `SwitchToBlackhole(ABORT_TOO_LONG)` does. let abort_raising_exception = pending_stb .as_ref() .is_some_and(|stb| stb.raising_exception); - // A walker raise site outside this crate cannot reach - // `TraceCtx::pending_switch_to_blackhole`, so its - // `Counters.ABORT_*` travels in `stage_abort_reason` instead. - // It ranks with `stb.reason` rather than below the too-long - // fallback: upstream raises at the site and never reaches the - // `blackhole_if_trace_too_long` check on that unwind. + // Explicit SwitchToBlackhole signals bypass the legacy + // length classification. In particular, the too-long raise + // has already disabled its callee and retired the log. let reason_int = match pending_stb .map(|stb| stb.reason) .or_else(|| self.meta.take_pending_abort_reason()) { Some(r) => r, + // The standalone JitCodeMachine still returns plain + // Abort for its post-step overflow. Keep its legacy + // classification here until it owns the MetaInterp + // decision. A SwitchToBlackhole always takes the Some + // arm, as pyjitpl.py's exception catch does. None => match self.meta.blackhole_if_trace_too_long() { Some(r) => r.as_int(), None => { - // Neither a staged reason nor a too-long - // verdict: unclassified, not a bridge giveup. - // Counted here because slot 41 holds both. crate::mc_diag_bump(71); AbortReason::Generic.as_int() } @@ -4980,10 +4986,12 @@ impl JitDriver { // frames are the sole record of where that left the // interpreter, so this bridge needs the handoff for the // same reason a fresh trace does. - if matches!(action, TraceAction::Abort) - && (self.meta.bridge_info().is_none() - || self.bridge_attempt_declined - || self.bridge_entered_at_guard_resume) + if matches!( + action, + TraceAction::Abort | TraceAction::SwitchToBlackhole(_) + ) && (self.meta.bridge_info().is_none() + || self.bridge_attempt_declined + || self.bridge_entered_at_guard_resume) { // This gate asks whether the session still has a bridge // artifact to resume into, which is the PHASE, not the @@ -9167,7 +9175,12 @@ impl JitDriver { .as_mut() .expect("bridge: tracing context must be live after start_retrace_from_guard"); ctx.header_pc = resume_pc; - if let Some(descriptor) = bridge_driver_descriptor { + // `ResumeGuardDescr._trace_and_compile_from_bridge` keeps the source + // loop's outermost driver. Only descriptor-less host entries need the + // state hook; it must not replace a driver recovered from the guard. + if ctx.driver_descriptor().is_none() + && let Some(descriptor) = bridge_driver_descriptor + { ctx.set_driver_descriptor((*descriptor).clone()); } // pyjitpl.py `if not self.partial_trace:` — bridge @@ -10529,6 +10542,61 @@ mod tests { ); } + #[test] + fn cell_is_tracing_is_per_green_key() { + const A: u64 = 11; + const B: u64 = 22; + let mut driver = JitDriver::::new(1); + driver.meta.finish_setup_descrs_for_jitdrivers(); + let mut state = ScalarWalkState::default(); + driver.force_start_tracing(A, A as usize, &mut state, &()); + assert!(driver.is_tracing()); + assert!(driver.cell_is_tracing(A)); + assert!(!driver.cell_is_tracing(B)); + } + + #[test] + fn too_long_unwind_preserves_the_root_spared_by_the_raise_site() { + const ROOT: u64 = 7; + const CALLEE: u64 = 0xa11; + let mut driver = JitDriver::::new(1); + driver.meta.finish_setup_descrs_for_jitdrivers(); + let mut state = ScalarWalkState::default(); + driver.force_start_tracing(ROOT, ROOT as usize, &mut state, &()); + assert!(driver.is_tracing()); + + driver.merge_point(|meta, _sym| { + let start = meta.trace_ctx().unwrap().get_trace_position(); + meta.push_portal_trace_position(0, Some((CALLEE, None)), start); + let ctx = meta.tracing.as_mut().unwrap(); + ctx.record_op(majit_ir::OpCode::PtrEq, &[]); + let end = ctx.get_trace_position(); + ctx.set_trace_limit(0); + meta.push_portal_trace_position(0, None, end); + let reason = meta.blackhole_if_trace_too_long().unwrap(); + assert_eq!(reason, AbortReason::TooLong); + assert!(meta.portal_trace_positions.is_none()); + assert!(meta.take_pending_abort_reason().is_none()); + TraceAction::SwitchToBlackhole(crate::SwitchToBlackhole { + reason: reason.as_int(), + raising_exception: false, + }) + }); + + assert!(!driver.is_tracing()); + let warmstate = driver.meta.warm_state_mut(); + assert!(!warmstate.can_inline_callable(CALLEE)); + assert!(warmstate.can_inline_callable(ROOT)); + assert!(!warmstate.should_force_finish_tracing(ROOT)); + // A fresh attempt must remain possible after the callee is disabled. + driver.force_start_tracing(ROOT, ROOT as usize, &mut state, &()); + assert!(driver.is_tracing()); + assert_eq!( + driver.meta.portal_trace_positions.as_ref().map(Vec::len), + Some(0) + ); + } + /// `TraceAction::AbortPermanent` retires the trace's green key through /// `abort_trace(true)` -> `disable_noninlinable_function`. For a PRIMARY /// trace that key names the code the walk gave up on, which is @@ -11130,6 +11198,85 @@ mod tests { assert!(!driver.meta.is_tracing()); } + #[test] + fn bridge_entry_preserves_the_source_driver_through_symbolic_setup() { + struct BridgeState; + impl JitState for BridgeState { + type Meta = (); + type Sym = (); + type Env = (); + fn build_meta(&self, _: usize, _: &()) {} + fn extract_live(&self, _: &()) -> Vec { + vec![0] + } + fn create_sym(_: &(), _: usize) {} + fn is_compatible(&self, _: &()) -> bool { + true + } + fn restore(&mut self, _: &(), _: &[i64]) {} + fn collect_jump_args(_: &()) -> Vec { + vec![] + } + fn validate_close(_: &(), _: &()) -> bool { + true + } + fn rebuild_from_resumedata( + _: &mut (), + fail_arg_types: &[Type], + storage: Option<&Arc>, + ) -> Option { + // This guard has no live state to reload. Unlike the default + // trait stub, the fixture admits the symbolic setup path. + assert!(fail_arg_types.is_empty()); + Some(crate::ResumeDataResult { + frames: vec![], + virtualizable_values: vec![], + virtualref_values: vec![], + storage: storage.cloned(), + num_failargs: 0, + fail_arg_types: vec![], + }) + } + } + let mut driver = JitDriver::::new(1); + driver.meta.finish_setup_descrs_for_jitdrivers(); + let host_driver = driver + .register_descriptor(JitDriverStaticData::new(vec![], vec![("value", Type::Int)])); + let source_driver = driver + .meta + .register_jitdriver_sd(JitDriverStaticData::new(vec![], vec![("value", Type::Int)])); + assert_ne!(host_driver, source_driver); + let descriptor = driver.meta.staticdata.jitdrivers_sd[source_driver].clone(); + let green_key = 406; + assert!(matches!( + driver + .meta + .force_start_tracing(green_key, (0, 0), Some(descriptor), &[Value::Int(0)],), + BackEdgeAction::StartedTracing + )); + { + let ctx = driver.meta.trace_ctx().unwrap(); + let guard = ctx.record_guard(OpCode::GuardTrue, &[OpRef::input_arg_int(0)], 0); + ctx.capture_snapshot_for_last_guard(&[], 0, 0); + ctx.set_fail_args(guard, &[]); + } + driver.meta.compile_loop(&[OpRef::input_arg_int(0)], ()); + let failure = driver.meta.run_compiled_detailed(green_key, &[0]).unwrap(); + let fail_values = crate::compile::raw_exit_values(&failure.typed_values); + let descr = failure.descr_arc.clone().unwrap(); + let mut state = BridgeState; + assert!(driver.start_bridge_tracing(&descr, &mut state, &(), &fail_values, 0, false)); + assert_eq!(driver.meta.active_jitdriver_sd, Some(source_driver)); + assert_eq!( + driver + .meta + .trace_ctx() + .and_then(|ctx| ctx.driver_descriptor()) + .and_then(|descriptor| descriptor.index), + Some(source_driver), + ); + } + // ── Multi-entry point lifecycle tests ── // Parity with warmspot.py multi-driver entry semantics: multiple functions // can share the same JitDriver and compiled loops via register_entry_point. diff --git a/majit/majit-metainterp/src/lib.rs b/majit/majit-metainterp/src/lib.rs index 53dba5478a3..7ee31989652 100644 --- a/majit/majit-metainterp/src/lib.rs +++ b/majit/majit-metainterp/src/lib.rs @@ -249,8 +249,8 @@ pub use pyjitpl::{ CompiledTraceLayout, DeadFrameArtifacts, DetailedDriverRunOutcome, InlineDecision, JitCodeMachine, JitCodeRuntime, JitCodeSym, JitHooks, JitStats, MIFrame, MIFrameStack, MetaInterp, MetaInterpGlobalData, MetaInterpStaticData, PortalGreenKey, RawCompileResult, - StandaloneFrameStack, SymbolicFnaddrPathResolver, build_state_field_snapshot, - call_int_function, call_ref_function, call_void_function, counters, + StandaloneFrameStack, SwitchToBlackhole, SymbolicFnaddrPathResolver, + build_state_field_snapshot, call_int_function, call_ref_function, call_void_function, counters, record_application_traceback_for_recording, record_application_traceback_hook_address, record_discarded_level_traceback_for_recording, record_discarded_level_traceback_hook_address, record_inline_application_traceback_for_recording, @@ -1293,6 +1293,10 @@ pub enum TraceAction { SegmentedBridge { exception_box: OpRef }, /// Abort the current trace (recoverable — may retry later). Abort, + /// pyjitpl.py `raise SwitchToBlackhole(reason)`: carry the decision to + /// the cancel-tracing catch without running another interpreter step or + /// another trace-length check while unwinding. + SwitchToBlackhole(pyjitpl::SwitchToBlackhole), /// Decline the current trace before compilation and return to residual /// execution without charging a trace abort. Decline, diff --git a/majit/majit-metainterp/src/pyjitpl.rs b/majit/majit-metainterp/src/pyjitpl.rs index 9ad8c956e92..6dc50cdb325 100644 --- a/majit/majit-metainterp/src/pyjitpl.rs +++ b/majit/majit-metainterp/src/pyjitpl.rs @@ -6263,7 +6263,7 @@ impl MetaInterp { // pyjitpl.py: `self.portal_trace_positions = None` marks the // abort boundary so post-abort consumers (e.g. test inspections // at pyjitpl.py:3547) can detect a terminated trace session. - self.portal_trace_positions = None; + self.retire_portal_trace_positions(); if let Some((huge_fn_jd_no, huge_fn_key)) = huge_fn { // pyjitpl.py:2821-2822 `jd_sd.warmstate.disable_noninlinable_function( // greenkey_of_huge_function)` disables through `jd_sd.warmstate`, @@ -15008,10 +15008,24 @@ impl MetaInterp { // greenkey-side JC_FORCE_FINISH (warmstate.py reads that flag at // loop entry, not bridge entry). let source_jct = majit_backend::descr_owning_jct(fail_descr); + // compile.py `ResumeGuardDescr._trace_and_compile_from_bridge`: + // jitdriver_sd is the source loop's outermost driver, including when + // this guard was recorded while inlining another recursive portal. + let source_jitdriver_index = source_jct + .as_ref() + .and_then(|token| token.outermost_jitdriver_index); self.force_finish_trace = source_jct.as_ref().is_some_and(|jct| { jct.retraced_count.get() & majit_backend::JitCellToken::FORCE_BRIDGE_SEGMENTING != 0 }); let mut ctx = crate::trace_ctx::TraceCtx::new(recorder, green_key, self.staticdata.clone()); + if let Some(index) = source_jitdriver_index { + let descriptor = self + .staticdata + .jitdrivers_sd + .get(index) + .expect("bridge source token must name a registered jitdriver"); + ctx.set_driver_descriptor(descriptor.clone()); + } ctx.set_force_finish(self.force_finish_trace); // pyjitpl.py:929-947 `self.metainterp.cpu` analog — see // `setup_tracing` for the contract on raw-pointer lifetime @@ -15029,12 +15043,13 @@ impl MetaInterp { ctx.callinfocollection = self.callinfocollection.clone(); self.tracing = Some(ctx); self.arm_portal_trace_positions(); - // pyjitpl.py `self.jitdriver_sd = jitdriver_sd`: bridges - // inherit the parent's driver. The bridge entry path does not - // thread `driver_descriptor`, so fall back to scanning for the - // vinfo-bearing slot — a no-op for single-portal pyre and the - // same shape `virtualizable_info()` already used. - self.active_jitdriver_sd = self.elect_active_jitdriver_sd(None); + // pyjitpl.py `MetaInterp.__init__` binds the driver passed by the + // bridge caller. The source guard's token retains that identity even + // after the warm cell has been redirected to a newer loop token. + // Descriptor-less host fixtures retain their implicit-driver entry; + // a registered source driver never falls back to a sibling's vinfo. + self.active_jitdriver_sd = + source_jitdriver_index.or_else(|| self.elect_active_jitdriver_sd(None)); if let Some(ref hook) = self.hooks.on_trace_start { hook(green_key); @@ -15829,11 +15844,9 @@ impl MetaInterp { /// ``` /// /// Resets `framestack` to empty, pushes the portal `mainjitcode` - /// frame, and seeds it with the original argboxes. Other branches - /// of the upstream method (`initialize_withgreenfields`, - /// `initialize_virtualizable`) live behind pyre's portal-runner - /// shim and are not yet wired through this entry — they remain - /// driven by the existing per-driver setup paths. + /// frame, and seeds it with the original argboxes. Then runs + /// `initialize_withgreenfields` and, when a trace is live, + /// `initialize_virtualizable` — the same tail as pyjitpl.py. pub fn initialize_state_from_start( &mut self, mainjitcode: std::sync::Arc, @@ -15852,6 +15865,64 @@ impl MetaInterp { // pyjitpl.py `self.virtualref_boxes = []` is implicit: the // backing vector lives on `TraceCtx`, which is fresh for every // `MetaInterp::setup_tracing` cycle. + self.initialize_withgreenfields(original_boxes); + let live: Vec = original_boxes + .iter() + .map(|(kind, _, concrete)| match kind { + crate::jitcode::JitArgKind::Int => majit_ir::Value::Int(*concrete), + crate::jitcode::JitArgKind::Ref => { + majit_ir::Value::Ref(majit_ir::GcRef(*concrete as usize)) + } + crate::jitcode::JitArgKind::Float => { + majit_ir::Value::Float(f64::from_bits(*concrete as u64)) + } + }) + .collect(); + if let Some(mut ctx) = self.tracing.take() { + self.initialize_virtualizable(&mut ctx, &live); + self.tracing = Some(ctx); + } + } + + /// pyjitpl.py `MetaInterp.initialize_withgreenfields(original_boxes)`. + pub(crate) fn initialize_withgreenfields( + &mut self, + original_boxes: &[(crate::jitcode::JitArgKind, OpRef, i64)], + ) { + let Some(idx) = self.active_jitdriver_sd else { + return; + }; + let (num_greens, red_index, has_vinfo) = { + let Some(jd) = self.staticdata.jitdrivers_sd.get(idx) else { + return; + }; + let Some(ginfo) = jd.greenfield_info.as_ref() else { + return; + }; + ( + jd.num_greens(), + ginfo.red_index, + jd.virtualizable_info.is_some(), + ) + }; + debug_assert!(!has_vinfo, "greenfield + virtualizable on the same driver"); + let index = num_greens + red_index; + let Some((kind, opref, concrete)) = original_boxes.get(index) else { + return; + }; + let Some(ctx) = self.tracing.as_mut() else { + return; + }; + let value = match kind { + crate::jitcode::JitArgKind::Int => majit_ir::Value::Int(*concrete), + crate::jitcode::JitArgKind::Ref => { + majit_ir::Value::Ref(majit_ir::GcRef(*concrete as usize)) + } + crate::jitcode::JitArgKind::Float => { + majit_ir::Value::Float(f64::from_bits(*concrete as u64)) + } + }; + ctx.set_greenfield_virtualizable_box(*opref, value); } /// pyjitpl.py `MetaInterp.rebuild_state_after_failure` — @@ -16596,39 +16667,46 @@ impl MetaInterp { /// falls through to `prepare_trace_segmenting`. pub fn find_biggest_function(&self) -> Option<(usize, PortalGreenKey)> { let positions = self.portal_trace_positions.as_ref()?; + let machine_events = self + .tracing + .as_ref() + .map(|ctx| ctx.portal_trace_events.as_slice()) + .unwrap_or(&[]); let mut start_stack: Vec<(usize, PortalGreenKey, usize)> = Vec::new(); - let mut max_size = 0usize; + let mut max_size = 0isize; let mut max_key = None; - for (jd_no, key, pos) in positions.iter().cloned() { + for (jd_no, key, pos) in positions + .iter() + .cloned() + .chain(machine_events.iter().cloned()) + { match key { // pyjitpl.py:3547-3548 `if key is not None: start_stack.append`. Some(key) => start_stack.push((jd_no, key, pos._pos)), - // pyjitpl.py:3549-3559 the closing entry sizes the frame it - // closes. An unmatched close cannot happen while `newframe` / - // `popframe` are the only writers, so it is left to `pop`'s - // `None` rather than given a recovery path. + // pyjitpl.py `MetaInterp.find_biggest_function`: an unmatched + // close is an invalid frame stack, not an empty candidate. None => { - if let Some((jd_no, green_key, start_pos)) = start_stack.pop() { - let size = pos._pos.saturating_sub(start_pos); - if size > max_size { - max_size = size; - max_key = Some((jd_no, green_key)); - } + let (start_jd_no, green_key, start_pos) = start_stack + .pop() + .expect("portal trace close without an opening frame"); + let size = pos._pos as isize - start_pos as isize; + if size > max_size { + max_size = size; + max_key = Some((start_jd_no, green_key)); } } } } - // pyjitpl.py `if start_stack:` — one frame, the outermost, - // measured against where the trace stopped. Upstream reads - // `self.history` there unconditionally; pyre's recorder is an `Option`, - // and a `?` on it would return `None` for the whole function and throw - // away a `max_key` the closed frames above already produced. Only the - // open frame is unmeasurable without a recorder, so only it is skipped. - if let Some((jd_no, green_key, start_pos)) = start_stack.first().cloned() - && let Some(tracing) = self.tracing.as_ref() - { + // pyjitpl.py `MetaInterp.find_biggest_function` measures the outermost + // open frame against the live history unconditionally. + if let Some((jd_no, green_key, start_pos)) = start_stack.first().cloned() { + let tracing = self + .tracing + .as_ref() + .expect("an open portal trace frame requires its live history"); let current = tracing.get_trace_position()._pos; - if current.saturating_sub(start_pos) > max_size { + let size = current as isize - start_pos as isize; + if size > max_size { max_key = Some((jd_no, green_key)); } } @@ -16646,6 +16724,17 @@ impl MetaInterp { /// rest of the process after the first overflow. fn arm_portal_trace_positions(&mut self) { self.portal_trace_positions = Some(Vec::new()); + if let Some(ctx) = self.tracing.as_mut() { + ctx.clear_portal_trace_events(); + } + } + + /// pyjitpl.py `self.portal_trace_positions = None`. + pub fn retire_portal_trace_positions(&mut self) { + self.portal_trace_positions = None; + if let Some(ctx) = self.tracing.as_mut() { + ctx.clear_portal_trace_events(); + } } /// pyjitpl.py `MetaInterp.is_main_jitcode(jitcode)`. @@ -16660,7 +16749,7 @@ impl MetaInterp { /// upstream's `jitcode.jitdriver_sd.jitdriver.is_recursive`. Falls /// back to `false` when the jitcode does not point at a registered /// driver slot — matches the `jitdriver_sd is not None` guard. - pub fn is_main_jitcode(&self, jitcode: &crate::jitcode::JitCode) -> bool { + pub fn is_main_jitcode(&self, jitcode: &crate::jitcode::CanonicalJitCode) -> bool { match jitcode.jitdriver_sd() { Some(idx) => self .staticdata @@ -16672,23 +16761,6 @@ impl MetaInterp { } } - /// The driver [`Self::is_main_jitcode`] would answer `true` for, named - /// without a jitcode. - /// - /// `newframe` reaches the log through a `JitCode`, which every caller - /// building an `MIFrame` has. A tracer that inlines a callee WITHOUT - /// building one — pyre's FBW walker walks the callee body directly — has - /// the same question to answer and no jitcode to answer it with, so the - /// predicate is offered here in its jitcode-free form: the index of the - /// recursive portal driver. `None` when none is registered, which is the - /// `is_main_jitcode` = false case and must equally leave the log alone. - pub fn main_jitdriver_index(&self) -> Option { - self.staticdata - .jitdrivers_sd - .iter() - .position(|jd| jd.is_recursive) - } - /// Append one `portal_trace_positions` entry, `newframe`'s /// (pyjitpl.py) and `popframe`'s (pyjitpl.py) shared /// tail, for a caller that reached the decision without an `MIFrame`. @@ -16749,9 +16821,10 @@ impl MetaInterp { self.portal_call_depth += 1; // pyjitpl.py:2435: self.call_ids.append(self.current_call_id) self.call_ids.push(self.current_call_id); - // pyjitpl.py: enter_portal_frame(jitdriver_sd.index, unique_id) - if let Some((unique_id, _)) = greenkey.as_ref() { - self.enter_portal_frame(jd_no, *unique_id); + // pyjitpl.py: unique_id = jitcode.jitdriver_sd.warmstate.get_unique_id(greenkey) + // enter_portal_frame(jitdriver_sd.index, unique_id) + if let Some(gk) = greenkey.as_ref() { + self.enter_portal_frame(jd_no, self.unique_id_for_greenkey(jd_no, gk)); } // pyjitpl.py:2442: self.current_call_id += 1 self.current_call_id += 1; @@ -16787,6 +16860,24 @@ impl MetaInterp { self.framestack.len() - 1 } + /// warmstate.py `WarmEnterState.get_unique_id(greenkey)`. + /// + /// The typed greens go to the driver's hook when both are present. + /// A missing hook or a hash-only key keeps the previous unique id, + /// the greenkey hash `newframe` used to record verbatim. + pub fn unique_id_for_greenkey(&self, jd_no: usize, greenkey: &PortalGreenKey) -> u64 { + if let Some(hook) = self + .staticdata + .jitdrivers_sd + .get(jd_no) + .and_then(|jd| jd.get_unique_id) + && let Some(typed) = greenkey.1.as_ref() + { + return hook(&typed.values) as u64; + } + greenkey.0 + } + /// pyjitpl.py `MetaInterp.enter_portal_frame(jd_no, unique_id)`. /// /// ```python @@ -20920,6 +21011,7 @@ mod metainterp_static_data_tests { assembler_helper_adr: 0, vable_token_descr: None, frame_value_count_fn: None, + get_unique_id: None, }; { let MetaInterp { @@ -21097,6 +21189,30 @@ mod metainterp_static_data_tests { assert_eq!(meta.portal_call_depth, 0); } + #[test] + fn initialize_state_from_start_seeds_greenfield_virtualizable_box() { + use crate::jitcode::JitArgKind; + let mut meta = MetaInterp::<()>::new(0); + meta.finish_setup_descrs_for_jitdrivers(); + let mut jd = + crate::jitdriver::JitDriverStaticData::new(vec![], vec![("obj", majit_ir::Type::Ref)]); + jd.greenfield_info = Some(crate::greenfield::GreenFieldInfo::new( + 0, + vec![("G".into(), "field".into())], + )); + let idx = meta.register_jitdriver_sd(jd); + meta.active_jitdriver_sd = Some(idx); + let action = meta.force_start_tracing(0, (0, 0), None, &[]); + assert!(matches!(action, crate::BackEdgeAction::StartedTracing)); + let box_ref = OpRef::ref_op(3); + meta.initialize_withgreenfields(&[(JitArgKind::Ref, box_ref, 0xabc)]); + let boxes = meta + .trace_ctx() + .and_then(|ctx| ctx.collect_virtualizable_boxes()) + .expect("greenfield box"); + assert_eq!(boxes, vec![box_ref]); + } + #[test] fn trace_jitcode_with_framestack_pushes_root_then_pops() { // pyjitpl.py self.framestack invariant: trace entry pushes the @@ -23000,49 +23116,44 @@ mod metainterp_static_data_tests { } #[test] - fn main_jitdriver_index_answers_is_main_jitcode_without_a_jitcode() { - // The walker inlines a callee without building an `MIFrame`, so it asks - // the `is_main_jitcode` question through this accessor instead. The two - // must agree, or the walker would log frames upstream would not (or - // skip ones it would). + fn portal_log_uses_the_jitcode_owner_with_two_recursive_drivers() { let mut meta = MetaInterp::<()>::new(0); meta.finish_setup_descrs_for_jitdrivers(); - assert_eq!(meta.main_jitdriver_index(), None); - - // A non-recursive driver is not the main one, exactly as - // `is_main_jitcode` reads it. - let mut plain = crate::jitdriver::JitDriverStaticData::new(vec![], vec![]); - plain.is_recursive = false; - let mut recursive = crate::jitdriver::JitDriverStaticData::new(vec![], vec![]); - recursive.is_recursive = true; - let (plain_idx, recursive_idx) = { + let mut first = crate::jitdriver::JitDriverStaticData::new(vec![], vec![]); + first.is_recursive = true; + let mut second = crate::jitdriver::JitDriverStaticData::new(vec![], vec![]); + second.is_recursive = true; + let second_index = { let MetaInterp { staticdata, backend, .. } = &mut meta; let sd = std::sync::Arc::get_mut(staticdata).unwrap(); - let plain_idx = sd.register_jitdriver_sd(plain, backend); - let recursive_idx = sd.register_jitdriver_sd(recursive, backend); - (plain_idx, recursive_idx) + sd.register_jitdriver_sd(first, backend); + sd.register_jitdriver_sd(second, backend) }; - assert_ne!(plain_idx, recursive_idx); - assert_eq!(meta.main_jitdriver_index(), Some(recursive_idx)); - - let mut jc = crate::jitcode::JitCodeBuilder::new().finish(); - jc.replace_jitdriver_sd(Some(plain_idx)); - assert!(!meta.is_main_jitcode(&jc)); - jc.replace_jitdriver_sd(Some(recursive_idx)); - assert!(meta.is_main_jitcode(&jc)); + assert_ne!(second_index, 0); + let mut jitcode = crate::jitcode::JitCodeBuilder::new().finish(); + jitcode.replace_jitdriver_sd(Some(second_index)); + assert!(meta.is_main_jitcode(&jitcode)); + start_tracing(&mut meta); + meta.newframe(std::sync::Arc::new(jitcode), Some((0xa11, None))); + record_ops(&mut meta, 5); + meta.popframe(true); + assert_eq!( + meta.find_biggest_function(), + Some((second_index, (0xa11, None))) + ); } #[test] fn push_portal_trace_position_pairs_are_sized_by_find_biggest_function() { // The walker's two calls land as one balanced start/end pair, which is // the shape `find_biggest_function` walks. A retired log drops both. - let (mut meta, _jc) = meta_with_recursive_portal(); + let (mut meta, jc) = meta_with_recursive_portal(); start_tracing(&mut meta); - let jd_no = meta.main_jitdriver_index().expect("recursive portal"); + let jd_no = jc.jitdriver_sd().expect("recursive portal"); let start = meta.trace_ctx().expect("tracing").get_trace_position(); meta.push_portal_trace_position(jd_no, Some((0xBEEF, None)), start); record_ops(&mut meta, 5); @@ -23050,6 +23161,22 @@ mod metainterp_static_data_tests { meta.push_portal_trace_position(jd_no, None, end); assert_eq!(meta.find_biggest_function(), Some((jd_no, (0xBEEF, None)))); + // JitCodeMachine records the same pairs on TraceCtx; the walker + // of find_biggest_function concatenates both logs. + meta.portal_trace_positions = Some(Vec::new()); + let start = meta.trace_ctx().expect("tracing").get_trace_position(); + meta.tracing + .as_mut() + .unwrap() + .push_portal_trace_event(jd_no, Some((0xCAFE, None)), start); + record_ops(&mut meta, 3); + let end = meta.trace_ctx().expect("tracing").get_trace_position(); + meta.tracing + .as_mut() + .unwrap() + .push_portal_trace_event(jd_no, None, end); + assert_eq!(meta.find_biggest_function(), Some((jd_no, (0xCAFE, None)))); + // pyjitpl.py `self.portal_trace_positions = None` — after the // abort boundary nothing is logged and nothing is found. meta.portal_trace_positions = None; @@ -23088,6 +23215,36 @@ mod metainterp_static_data_tests { assert_eq!(unique_id, majit_ir::Value::Int(0xfeed)); } + #[test] + fn newframe_records_unique_id_from_the_driver_hook() { + // warmstate.py get_unique_id(greenkey) unwraps the greens; the hash + // is not the unique id. + fn hook(greens: &[i64]) -> i64 { + assert_eq!(greens, &[1, 0, 0xabc]); + 0xfeed + } + let (mut meta, jc) = meta_with_recursive_portal(); + let jd_no = jc.jitdriver_sd().expect("recursive portal"); + std::sync::Arc::get_mut(&mut meta.staticdata) + .unwrap() + .jitdrivers_sd[jd_no] + .get_unique_id = Some(hook); + start_tracing(&mut meta); + let typed = majit_ir::GreenKey::new(vec![1, 0, 0xabc]); + meta.newframe(jc, Some((0xbadd, Some(typed)))); + let ctx = meta.trace_ctx().expect("tracing"); + let op = ctx + .recorder + .ops() + .iter() + .find(|op| op.opcode == OpCode::EnterPortalFrame) + .expect("EnterPortalFrame"); + let unique_id = ctx + .constants_get_value(op.arg(1).to_opref()) + .expect("unique_id"); + assert_eq!(unique_id, majit_ir::Value::Int(0xfeed)); + } + #[test] fn leave_portal_frame_records_const_int_jd_no() { // pyjitpl.py:2459 — history.record1(rop.LEAVE_PORTAL_FRAME, ConstInt(jd_no), None) @@ -23378,11 +23535,10 @@ mod metainterp_static_data_tests { } #[test] - fn find_biggest_function_keeps_a_closed_frame_when_the_recorder_is_gone() { - // pyjitpl.py:3560-3570 reads `self.history` unconditionally, so a - // closed frame's size always survives to the return. pyre's recorder is - // an `Option`: an unmatched open entry plus `tracing = None` must skip - // only the open frame's measurement, not discard `max_key`. + #[should_panic(expected = "an open portal trace frame requires its live history")] + fn find_biggest_function_requires_history_for_an_open_frame() { + // pyjitpl.py `MetaInterp.find_biggest_function` cannot measure an + // open frame after its recorder has been discarded. let (mut meta, jc) = meta_with_recursive_portal(); start_tracing(&mut meta); @@ -23396,87 +23552,27 @@ mod metainterp_static_data_tests { // Left open, and the recorder retired under it. meta.tracing = None; - assert_eq!( - meta.find_biggest_function(), - Some((0, (0xa11, None))), - "the closed frame's size survives a missing recorder" - ); + meta.find_biggest_function(); } #[test] - fn find_biggest_function_is_none_without_an_inlined_portal_frame() { - // The root frame carries no greenkey, so a trace that inlined nothing - // leaves the log empty and the caller takes the segmenting arm. - let (mut meta, _jc) = meta_with_recursive_portal(); + #[should_panic(expected = "portal trace close without an opening frame")] + fn find_biggest_function_rejects_an_unmatched_close() { + let (mut meta, _) = meta_with_recursive_portal(); start_tracing(&mut meta); - record_ops(&mut meta, 5); - assert_eq!(meta.find_biggest_function(), None); + let pos = meta.trace_ctx().unwrap().get_trace_position(); + meta.push_portal_trace_position(0, None, pos); + meta.find_biggest_function(); } - /// pyjitpl.py:2817-2831 runs the too-long bookkeeping once per abort: the - /// reason travels on the `SwitchToBlackhole` instance and the `_interpret` - /// catch never re-enters the check that raised it. This pins what a second - /// entry costs, because pyre reaches the same handler through a - /// `DispatchError` that carries no reason and so had a path back into it. - /// - /// The first run names the oversized callee and disables just that callee, - /// deliberately leaving the root un-marked so it can retrace without it. - /// It also retires the log it read, so a second run can name nothing and - /// takes `prepare_trace_segmenting` instead — which stamps the root with - /// `JC_FORCE_FINISH` + `JC_DONT_TRACE_HERE`, neither of which is ever - /// cleared. The callee's size is what overflowed the trace; the root pays - /// for it permanently. #[test] - fn a_second_too_long_run_segments_a_root_the_first_one_spared() { - // `start_tracing` opens the loop header this walk is rooted at, and - // its green key is the one the segmenting arm would mark. - const ROOT: u64 = 0; - const CALLEE: u64 = 0xa11; - - let (mut meta, jc) = meta_with_recursive_portal(); + fn find_biggest_function_is_none_without_an_inlined_portal_frame() { + // The root frame carries no greenkey, so a trace that inlined nothing + // leaves the log empty and the caller takes the segmenting arm. + let (mut meta, _jc) = meta_with_recursive_portal(); start_tracing(&mut meta); - // One inlined callee, sized by the ops recorded between its entries. - meta.perform_call(jc, &[], Some((CALLEE, None))) - .unwrap_err(); record_ops(&mut meta, 5); - meta.popframe(true); - meta.tracing - .as_mut() - .expect("tracing is Some") - .set_trace_limit(0); - - assert_eq!( - meta.blackhole_if_trace_too_long(), - Some(AbortReason::TooLong) - ); - assert!( - !meta.warm_state_mut().can_inline_callable(CALLEE), - "the named callee is the one that gets disabled" - ); - assert!( - !meta.warm_state_mut().should_force_finish_tracing(ROOT), - "the root is only asked to retrace, so it must not be force-finished" - ); - assert!( - meta.warm_state_mut().can_inline_callable(ROOT), - "the root is only asked to retrace, so it must stay inlinable" - ); - - // Exactly what a second entry sees: the same over-budget trace, and a - // log this abort already retired. assert_eq!(meta.find_biggest_function(), None); - assert_eq!( - meta.blackhole_if_trace_too_long(), - Some(AbortReason::TooLong) - ); - assert!( - meta.warm_state_mut().should_force_finish_tracing(ROOT), - "a second run has no callee to name and segments the root instead" - ); - assert!( - !meta.warm_state_mut().can_inline_callable(ROOT), - "and stamps it dont-trace-here, which nothing clears" - ); } #[test] @@ -25384,6 +25480,7 @@ mod tests { ) { meta.backend.set_constants_pool(constants_typed.clone()); let mut token = JitCellToken::new(green_key + 1000); + token.outermost_jitdriver_index = meta.active_jitdriver_sd; let trace_id = meta.alloc_trace_id(); meta.backend.set_next_trace_id(trace_id); meta.backend @@ -25437,6 +25534,7 @@ mod tests { ); let token_arc = std::sync::Arc::new(token); + compile::wire_clt_loop_token_wref(&token_arc); // Mirror production attach: warmstate.py:339-348 // `attach_procedure_to_interp` writes `cell.loop_token` so the // green_key → token canonical lookup (warmstate.py:188-202) is @@ -25663,6 +25761,12 @@ mod tests { fn test_start_retrace_from_guard_uses_previous_token_backend_resume_data() { let mut meta = MetaInterp::<()>::new(1); meta.finish_setup_descrs_for_jitdrivers(); + let first_driver = meta + .register_jitdriver_sd(JitDriverStaticData::new(vec![], vec![("value", Type::Int)])); + let source_driver = meta + .register_jitdriver_sd(JitDriverStaticData::new(vec![], vec![("value", Type::Int)])); + assert_ne!(first_driver, source_driver); + meta.active_jitdriver_sd = Some(source_driver); let green_key = 89; let inputargs = vec![InputArg::new_int(0)]; let mut guard = mk_op( @@ -25701,6 +25805,13 @@ mod tests { .get(&fail_index) .and_then(|layout| layout.descr.clone()) .expect("test fixture guard should carry a ResumeGuardDescr"); + // compile.py `record_loop_or_bridge` installs this on each + // ResumeDescr after backend compilation. + let source_token = entry.token.upgrade().expect("live source token"); + descr_arc + .as_fail_descr() + .unwrap() + .set_rd_loop_token_clt(source_token.compiled_loop_token_expect()); (trace_id, fail_index, descr_arc) }; @@ -25725,6 +25836,7 @@ mod tests { vec![], ); let mut fresh_token = JitCellToken::new(9004); + fresh_token.outermost_jitdriver_index = Some(first_driver); fresh_token.green_key = std::cell::Cell::new(green_key); let fresh_arc = std::sync::Arc::new(fresh_token); let old_token = @@ -25739,11 +25851,21 @@ mod tests { fresh_arc }; + // Neither the most recent trace nor the replacement token owns this + // guard. Its original token must supply the bridge's driver. + meta.active_jitdriver_sd = Some(first_driver); let retrace = meta .start_retrace_from_guard(descr_arc, green_key, trace_id, fail_index, &[42]) .expect("retrace should use previous token backend resume data"); assert_eq!(retrace.fail_types, vec![Type::Int]); + assert_eq!(meta.active_jitdriver_sd, Some(source_driver)); + assert_eq!( + meta.trace_ctx() + .and_then(|ctx| ctx.driver_descriptor()) + .and_then(|descriptor| descriptor.index), + Some(source_driver), + ); let storage = retrace .storage .as_ref() diff --git a/majit/majit-metainterp/src/pyjitpl/dispatch.rs b/majit/majit-metainterp/src/pyjitpl/dispatch.rs index cc5c5b8e02c..ab822199c2d 100644 --- a/majit/majit-metainterp/src/pyjitpl/dispatch.rs +++ b/majit/majit-metainterp/src/pyjitpl/dispatch.rs @@ -1060,6 +1060,13 @@ pub trait JitCodeRuntime { None } + /// pyjitpl.py `is_main_jitcode` for a recursive portal this runtime + /// is about to inline. Default false so fixture runtimes that never + /// overflow keep an empty log. + fn is_main_portal(&self, _jd_index: usize) -> bool { + false + } + /// Resolve the CALL_ASSEMBLER target for a `BC_RECURSIVE_CALL_*` /// opcode whose inline decision came back `CallAssembler` /// (pyjitpl.py → `do_recursive_call(assembler_call=True)`). The @@ -1388,6 +1395,10 @@ where ) -> Option<()> { (self.recursive_exec_void)(token, reds) } + + fn is_main_portal(&self, _jd_index: usize) -> bool { + true + } } /// JitCode bytecode interpreter for tracing. @@ -2764,6 +2775,9 @@ where if frame.portal_entered { let jd_box = ctx.const_int(frame.portal_jd as i64); ctx.record_op(OpCode::LeavePortalFrame, &[jd_box]); + if frame.portal_trace_logged { + ctx.push_portal_trace_event(frame.portal_jd, None, ctx.get_trace_position()); + } } let portal_scalar_state = frame.portal_scalar_state.take(); self.frames.recycle_frame(frame); @@ -3398,34 +3412,18 @@ where let jd_box = ctx.const_int(jd_index as i64); let uid_box = ctx.const_int(green_pc as i64); ctx.record_op(OpCode::EnterPortalFrame, &[jd_box, uid_box]); - // `newframe` records ENTER_PORTAL_FRAME and appends a - // `portal_trace_positions` entry on adjacent lines; this arm does - // only the first half. `push_portal_trace_position` lives on - // `MetaInterp`, and a `JitCodeMachine` reaches its host only - // through the `Runtime` trait, which carries no channel to it. - // Both LEAVE pops (`pop_exception_frame` and the finished-frame - // pop in `run_one_step`) omit the closing entry symmetrically, so - // the log stays BALANCED — `find_biggest_function` pairs entries - // off a stack, and dropping one half alone would mis-size every - // frame after it. What the omission costs is that a - // `TraceTooLong` taken inside an inlined portal finds no candidate - // to disable and falls to `prepare_trace_segmenting`. - // - // A defaulted `Runtime` hook would not close this: the trait's - // `begin_portal_op` / `commit_portal_op` / `abort_portal_op` seams - // already have no implementor anywhere in the workspace, so a - // fourth would leave every runtime's log as empty as it is now. - // The entry has to come from a host that owns the `MetaInterp`, - // and pyre's walker is such a host: `note_inline_subwalk_start` - // (`pyre-jit-trace/src/state.rs`, called from `inline_call.rs`) - // reaches the driver through `try_driver_pair()` and calls - // `push_portal_trace_position`, while the too-long handler beside - // it reads the result back through `find_biggest_function` before - // retiring the log. So the omission does NOT mean the log is - // empty wherever a recursive portal overflows — on the walker path - // it is filled and consumed. What this arm leaves out is confined - // to runtimes that inline through HERE, which in this workspace is - // the `dispatch.rs` fixtures alone. + // pyjitpl.py `newframe`: ENTER_PORTAL_FRAME and the + // `portal_trace_positions` append sit on adjacent lines. + // The machine cannot reach MetaInterp, so the log half + // lives on TraceCtx and `find_biggest_function` reads both. + if runtime.is_main_portal(jd_index) { + ctx.push_portal_trace_event( + jd_index, + Some((green_pc as u64, None)), + ctx.get_trace_position(), + ); + portal_frame.portal_trace_logged = true; + } portal_frame.inline_frame = true; // pyjitpl.py:2461-2492 pairing: this push recorded ENTER_PORTAL_FRAME, // so the frame's normal-return / exception-return pop records the @@ -3815,6 +3813,13 @@ where if finished_frame.portal_entered { let jd_box = ctx.const_int(finished_frame.portal_jd as i64); ctx.record_op(OpCode::LeavePortalFrame, &[jd_box]); + if finished_frame.portal_trace_logged { + ctx.push_portal_trace_event( + finished_frame.portal_jd, + None, + ctx.get_trace_position(), + ); + } } // [FR] Restore the caller's sym scalar/fixed-array state that this // inline recursive-portal frame overwrote. @@ -6660,6 +6665,15 @@ where if popped.inline_frame { ctx.pop_inline_frame(); } + // popframe still appends the log close when greenkey + // is set, even with leave_portal_frame=False. + if popped.portal_trace_logged { + ctx.push_portal_trace_event( + popped.portal_jd, + None, + ctx.get_trace_position(), + ); + } if let Some(snapshot) = popped.portal_scalar_state.take() { sym.restore_inline_scalar_state(snapshot); } @@ -12157,6 +12171,10 @@ mod tests { fn portal_jitcode(&self, _jd_index: usize) -> Option> { Some(self.portal.clone()) } + + fn is_main_portal(&self, _jd_index: usize) -> bool { + true + } } /// recursive-call SLICE 0 — a `BC_RECURSIVE_CALL_INT` whose runtime inlines the @@ -12832,6 +12850,10 @@ mod tests { Some(self.portal.clone()) } + fn is_main_portal(&self, _jd_index: usize) -> bool { + true + } + fn recursive_call_assembler_target( &self, _jd_index: usize, diff --git a/majit/majit-metainterp/src/pyjitpl/frame.rs b/majit/majit-metainterp/src/pyjitpl/frame.rs index 0ddd8a9583c..8ae5a739529 100644 --- a/majit/majit-metainterp/src/pyjitpl/frame.rs +++ b/majit/majit-metainterp/src/pyjitpl/frame.rs @@ -102,6 +102,10 @@ pub struct MIFrame { /// that record no ENTER and must record no LEAVE. The merge-point cut is the /// sole `leave_portal_frame=False` site and re-emits LEAVE itself. pub portal_entered: bool, + /// True when the matching `portal_trace_positions` open entry was + /// recorded. Leave must push a close only then, or find_biggest_function + /// sees an unmatched close. + pub portal_trace_logged: bool, /// \[FR\] The jd_index carried in this frame's `LEAVE_PORTAL_FRAME` op, set at /// the same push that set `portal_entered`. Unused when `portal_entered` is /// false. @@ -185,6 +189,7 @@ impl MIFrame { inline_frame: false, portal_scalar_state: None, portal_entered: false, + portal_trace_logged: false, portal_jd: 0, return_i: None, return_r: None, @@ -293,6 +298,7 @@ impl MIFrame { self.inline_frame = false; self.portal_scalar_state = None; self.portal_entered = false; + self.portal_trace_logged = false; self.portal_jd = 0; self.return_i = None; self.return_r = None; diff --git a/majit/majit-metainterp/src/trace_ctx.rs b/majit/majit-metainterp/src/trace_ctx.rs index 763500547cc..23ed45a4f58 100644 --- a/majit/majit-metainterp/src/trace_ctx.rs +++ b/majit/majit-metainterp/src/trace_ctx.rs @@ -270,6 +270,15 @@ pub struct TraceCtx { /// doing tuple-equality comparisons in [`recursive_depth`] and /// [`is_tracing_key`]. pub(crate) inline_frames: Vec<(usize, usize)>, + /// `portal_trace_positions` entries recorded by `JitCodeMachine` + /// while it cannot reach `MetaInterp`. `find_biggest_function` + /// walks these after the MetaInterp log. Retired together with + /// `MetaInterp.portal_trace_positions`. + pub(crate) portal_trace_events: Vec<( + usize, + Option, + crate::recorder::TracePosition, + )>, /// Structured green key values (if provided by the interpreter). green_key_values: Option, /// Declarative driver layout metadata, if provided by the interpreter. @@ -885,6 +894,21 @@ impl TraceCtx { &mut self.heap_cache } + /// pyjitpl.py `newframe` / `popframe` log half for a JitCodeMachine + /// that cannot reach `MetaInterp.portal_trace_positions`. + pub fn push_portal_trace_event( + &mut self, + jd_no: usize, + green_key: Option, + pos: crate::recorder::TracePosition, + ) { + self.portal_trace_events.push((jd_no, green_key, pos)); + } + + pub fn clear_portal_trace_events(&mut self) { + self.portal_trace_events.clear(); + } + /// Install the `self.metainterp.cpu` analog for the cache-hit /// sanity-check load. /// @@ -1766,6 +1790,7 @@ impl TraceCtx { green_key_raw: (0, 0), root_green_key_raw: (0, 0), inline_frames: Vec::new(), + portal_trace_events: Vec::new(), green_key_values: None, driver_descriptor: None, virtualizable_boxes: None, @@ -1865,6 +1890,7 @@ impl TraceCtx { green_key_raw: (0, 0), root_green_key_raw: (0, 0), inline_frames: Vec::new(), + portal_trace_events: Vec::new(), green_key_values: Some(green_key_values), driver_descriptor: None, virtualizable_boxes: None, @@ -2528,6 +2554,13 @@ impl TraceCtx { self.driver_descriptor = Some(descriptor); } + /// pyjitpl.py `initialize_withgreenfields`: the single red that owns + /// the green fields is the whole virtualizable box list. + pub fn set_greenfield_virtualizable_box(&mut self, box_ref: OpRef, value: Value) { + self.virtualizable_boxes = Some(vec![box_ref]); + self.virtualizable_values = Some(vec![value]); + } + /// Initialize standard virtualizable boxes from input args. /// Called at trace start when a virtualizable is registered. /// diff --git a/majit/majit-metainterp/src/warmspot.rs b/majit/majit-metainterp/src/warmspot.rs index 241ad490b4b..533c695673c 100644 --- a/majit/majit-metainterp/src/warmspot.rs +++ b/majit/majit-metainterp/src/warmspot.rs @@ -5,6 +5,22 @@ //! jitdriver metadata, warmstate, compile helpers, and the `pyre-jit` portal //! boundary. This module is the parity namespace that re-exports those pieces //! under the upstream module name without introducing a second implementation. +//! +//! `WarmRunnerDesc.apply_jit` graph rewrites have no mutable interpreter-graph +//! stage here (design.md §3.7 A1). Their source-level ports: +//! - `split_graph_and_record_jitdriver` — `register_configured_jitdrivers` +//! (autoreds run `autodetect_jit_markers_redvars` first) +//! - `rewrite_jit_merge_point` — `_unpackiterable_unknown_length` returns +//! `unpack_portal_runner`; jd0 is `eval_loop_jit` / `ll_portal_runner_shim` +//! - `rewrite_can_enter_jits` — `can_enter_jit` / `unpack_merge_point` bodies +//! - `rewrite_set_param_and_get_stats` — `set_jit_param` hook +//! - `rewrite_force_virtual` — `force_pyframe_vref` +//! - `rewrite_force_quasi_immutable` — `jtransform` + `do_force_quasi_immutable` +//! - `rewrite_jitcell_accesses` — `WarmEnterState` methods +//! - `make_driverhook_graphs` — `get_unique_id` / `get_printable_location` +//! - `inline_inlineable_portals` / `prejit_optimizations` / `add_finish` / +//! `create_jit_entry_points` — no `@jitdriver.inline` sites, no backendopt +//! pass over interpreter graphs, no translated finish callback pub use crate::jitdriver::{ DeclarativeJitDriver, JitDriver, JitDriverStaticData, TraceContinuationSuspendGuard, diff --git a/majit/majit-translate/src/codewriter/jtransform.rs b/majit/majit-translate/src/codewriter/jtransform.rs index c6c7ce29cfe..93f65c3c164 100644 --- a/majit/majit-translate/src/codewriter/jtransform.rs +++ b/majit/majit-translate/src/codewriter/jtransform.rs @@ -799,7 +799,7 @@ fn reversed_comparison_binop(name: &str) -> &str { clippy::mutable_key_type, reason = "Eq and Hash use immutable identity/value data; interior mutation is excluded, matching RPython identity-keyed dict semantics" )] -fn autodetect_jit_markers_redvars( +pub(crate) fn autodetect_jit_markers_redvars( graph: &FunctionGraph, greens: &[crate::flowspace::model::Variable], driver_roots: &[String], diff --git a/majit/majit-translate/src/lib.rs b/majit/majit-translate/src/lib.rs index 515c6f3e8f8..df1b8de556f 100644 --- a/majit/majit-translate/src/lib.rs +++ b/majit/majit-translate/src/lib.rs @@ -2437,12 +2437,11 @@ fn register_configured_jitdrivers( must share one JitDriverStaticData", spec.portal.canonical_key(), ); - // `spec.autoreds`: `support.py decode_hp_hint_args` requires a fixed - // numreds; `warmspot.py find_portals` obtains it from - // `support.autodetect_jit_markers_redvars` before splitting, while - // majit currently discovers autoreds later in - // `jtransform.py try_handle_jit_marker`. - let portal_path = if !spec.split_portal || spec.autoreds { + // `warmspot.py find_portals` runs `autodetect_jit_markers_redvars` + // before `split_graph_and_record_jitdriver`, so an autoreds portal + // splits once numreds is known. `jtransform.py try_handle_jit_marker` + // still rediscovers those reds when emitting the merge-point op. + let portal_path = if !spec.split_portal { spec.portal.clone() } else { // Ends the `call_control` borrow the `register_function_graph` @@ -2475,12 +2474,46 @@ fn register_configured_jitdrivers( let (marker_block, marker_index) = crate::codewriter::support::find_jit_merge_point(&portal, driver_roots) .expect("no jit_merge_point found in configured portal graph"); + // `support.py autodetect_jit_markers_redvars` before + // `split_before_jit_merge_point`: declared reds stay as written, + // autoreds learn the live set at the marker. + let numreds = if spec.autoreds { + let greens = match &portal + .block(marker_block) + .operations + .get(marker_index) + .expect("find_jit_merge_point returned a live marker") + .kind + { + crate::model::OpKind::Call { args, .. } => args + .iter() + .skip(1) + .take(spec.greens.len()) + .cloned() + .collect::>(), + _ => panic!("jit_merge_point must be a call"), + }; + let reds = crate::codewriter::jtransform::autodetect_jit_markers_redvars( + &portal, + &greens, + driver_roots, + ); + // support.py autodetect_jit_markers_redvars: `op.args.extend(reds_v)` + // so `decode_hp_hint_args` can split greens/reds off the marker. + match &mut portal.block_mut(marker_block).operations[marker_index].kind { + crate::model::OpKind::Call { args, .. } => args.extend(reds.iter().cloned()), + _ => panic!("jit_merge_point must be a call"), + } + reds.len() + } else { + spec.reds.len() + }; portal.startblock = crate::codewriter::support::split_before_jit_merge_point( &mut portal, marker_block, marker_index, spec.greens.len(), - spec.reds.len(), + numreds, driver_roots, ); // `warmspot.py WarmRunnerDesc.split_graph_and_record_jitdriver`: @@ -3049,4 +3082,79 @@ mod portal_driver_tests { assert!(!jd0.autoreds); assert_eq!(jd0.numreds, Some(2)); } + + #[test] + fn splits_an_autoreds_portal_after_autodetect() { + use crate::codewriter::type_state::ConcreteType; + use crate::flowspace::model::Variable; + use crate::model::Link; + + let mut call_control = call::CallControl::new(); + let portal = CallPath::from_segments(["fixture", "unpackiterable"]); + let mut graph = FunctionGraph::new("unpackiterable"); + let receiver = Variable::named("driver"); + let green = Variable::named("greenkey"); + let w_iterator = Variable::named("w_iterator"); + let items = Variable::named("items"); + for variable in [&receiver, &green, &w_iterator, &items] { + FunctionGraph::set_concretetype_of_inline(variable, ConcreteType::GcRef); + } + graph.block_mut(graph.startblock).inputargs = + vec![green.clone(), w_iterator.clone(), items.clone()]; + // RPython's marker receiver is a JitDriver Constant. Rematerialize it + // after the split the way `unsimplify.split_block` does for a + // const-produced driver singleton. + graph + .block_mut(graph.startblock) + .operations + .push(SpaceOperation { + result: Some(receiver.clone()), + kind: OpKind::ConstRefNull, + }); + graph + .block_mut(graph.startblock) + .operations + .push(SpaceOperation { + result: None, + kind: OpKind::Call { + target: CallTarget::method( + "jit_merge_point", + Some("UnpackIterableJitDriver".into()), + ), + args: vec![receiver, green.clone()], + result_ty: ValueType::Void, + }, + }); + graph.block_mut(graph.startblock).exits = vec![Link::from_variables( + &graph, + vec![green, w_iterator, items], + graph.startblock, + None, + )]; + call_control.register_function_graph(portal.clone(), graph); + + let mut spec = driver(portal); + spec.greens = vec!["greenkey".into()]; + spec.autoreds = true; + spec.split_portal = true; + register_configured_jitdrivers( + &mut call_control, + &[spec], + &GraphTransformConfig::default().jitdriver_receiver_roots, + ); + let split = CallPath::from_segments(["fixture", "unpackiterable_portal"]); + assert_eq!(call_control.jitdrivers_sd()[0].portal_graph, split); + let split_graph = call_control + .function_graphs() + .get(&split) + .expect("autoreds split registers the cut portal"); + assert!(split_graph.func.dont_inline); + assert!( + crate::codewriter::support::find_jit_merge_point( + split_graph, + &GraphTransformConfig::default().jitdriver_receiver_roots, + ) + .is_some() + ); + } } diff --git a/majit/majit-translate/src/pipeline.rs b/majit/majit-translate/src/pipeline.rs index 6abdbc4a0cb..d4d3d58a158 100644 --- a/majit/majit-translate/src/pipeline.rs +++ b/majit/majit-translate/src/pipeline.rs @@ -66,8 +66,10 @@ pub struct JitDriverSpec { /// before its `jit_merge_point`, instead of against the graph that /// contains the marker. /// - /// Off by default: with it off `register_configured_jitdrivers` passes - /// the configured path through unchanged and no graph copy is made. + /// Default true for a declared-reds portal: `register_configured_jitdrivers` + /// copies the graph and splits it before `jit_merge_point`. Autoreds + /// drivers run `autodetect_jit_markers_redvars` first so the split has + /// a fixed `numreds`, matching `warmspot.py find_portals`. #[serde(default)] pub split_portal: bool, } diff --git a/majit/majit-translate/tests/test_result_exc_lowering.rs b/majit/majit-translate/tests/test_result_exc_lowering.rs index 6301273182d..5768d616370 100644 --- a/majit/majit-translate/tests/test_result_exc_lowering.rs +++ b/majit/majit-translate/tests/test_result_exc_lowering.rs @@ -316,7 +316,7 @@ fn execute_wrapper_family_lowers_to_raise_links() { /// Facet A firing guard — the jd1 drain-loop `match next()` fusion. /// -/// `_unpackiterable_unknown_length`'s StopIteration drain loop is a +/// `unpackiterable_portal`'s StopIteration drain loop is a /// hand-written `match next() { Ok(w) => append, Err(e) if /// e.matches_stop_iteration() => break, Err(e) => return Err(e) }`. Lowered /// naively it materialises a `Result` shell and leaves the PyError predicate @@ -337,9 +337,9 @@ fn unpackiterable_drain_match_fuses_to_kind_test() { let llbc = interp(); let graph = lower_function( llbc, - "pyre_interpreter::baseobjspace::_unpackiterable_unknown_length", + "pyre_interpreter::baseobjspace::unpackiterable_portal", ) - .expect("lower _unpackiterable_unknown_length"); + .expect("lower unpackiterable_portal"); // Positive firing signal: only the fusion emits this object-level helper // FunctionPath, so its presence proves `try_fuse_drain_match` fired rather diff --git a/pyre/bench/synth/exception_traceback_frame_lineno.cranelift.jitstats b/pyre/bench/synth/exception_traceback_frame_lineno.cranelift.jitstats index d617e82e926..7acf951281d 100644 --- a/pyre/bench/synth/exception_traceback_frame_lineno.cranelift.jitstats +++ b/pyre/bench/synth/exception_traceback_frame_lineno.cranelift.jitstats @@ -12,7 +12,7 @@ fbw_rolled_back_with_effects=0 fbw_store_journal_rollback_failed=0 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 -guard_failures=824 +guard_failures=624 internal_compile_panics=0 loops_aborted=0 loops_compiled=17 diff --git a/pyre/bench/synth/exception_traceback_frame_lineno.dynasm.jitstats b/pyre/bench/synth/exception_traceback_frame_lineno.dynasm.jitstats index d617e82e926..7acf951281d 100644 --- a/pyre/bench/synth/exception_traceback_frame_lineno.dynasm.jitstats +++ b/pyre/bench/synth/exception_traceback_frame_lineno.dynasm.jitstats @@ -12,7 +12,7 @@ fbw_rolled_back_with_effects=0 fbw_store_journal_rollback_failed=0 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 -guard_failures=824 +guard_failures=624 internal_compile_panics=0 loops_aborted=0 loops_compiled=17 diff --git a/pyre/bench/synth/exception_traceback_frame_lineno.wasm.jitstats b/pyre/bench/synth/exception_traceback_frame_lineno.wasm.jitstats index d617e82e926..7acf951281d 100644 --- a/pyre/bench/synth/exception_traceback_frame_lineno.wasm.jitstats +++ b/pyre/bench/synth/exception_traceback_frame_lineno.wasm.jitstats @@ -12,7 +12,7 @@ fbw_rolled_back_with_effects=0 fbw_store_journal_rollback_failed=0 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 -guard_failures=824 +guard_failures=624 internal_compile_panics=0 loops_aborted=0 loops_compiled=17 diff --git a/pyre/bench/synth/trace_too_long_inline_multiframe.cranelift.jitstats b/pyre/bench/synth/trace_too_long_inline_multiframe.cranelift.jitstats index 8b06c34a2c7..ddd9d237bb1 100644 --- a/pyre/bench/synth/trace_too_long_inline_multiframe.cranelift.jitstats +++ b/pyre/bench/synth/trace_too_long_inline_multiframe.cranelift.jitstats @@ -3,7 +3,7 @@ descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_blackhole_adopted_multi_frame=5 -fbw_blackhole_adopted_single_frame=10 +fbw_blackhole_adopted_single_frame=11 fbw_escape_plain_fallback=0 fbw_escape_plain_fallback_unclean=0 fbw_foriter_item_dropped=0 @@ -12,8 +12,8 @@ fbw_rolled_back_with_effects=0 fbw_store_journal_rollback_failed=0 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 -guard_failures=429 +guard_failures=427 internal_compile_panics=0 -loops_aborted=15 +loops_aborted=16 loops_compiled=54 retraces_compiled=0 diff --git a/pyre/bench/synth/trace_too_long_inline_multiframe.dynasm.jitstats b/pyre/bench/synth/trace_too_long_inline_multiframe.dynasm.jitstats index 8b06c34a2c7..ddd9d237bb1 100644 --- a/pyre/bench/synth/trace_too_long_inline_multiframe.dynasm.jitstats +++ b/pyre/bench/synth/trace_too_long_inline_multiframe.dynasm.jitstats @@ -3,7 +3,7 @@ descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_blackhole_adopted_multi_frame=5 -fbw_blackhole_adopted_single_frame=10 +fbw_blackhole_adopted_single_frame=11 fbw_escape_plain_fallback=0 fbw_escape_plain_fallback_unclean=0 fbw_foriter_item_dropped=0 @@ -12,8 +12,8 @@ fbw_rolled_back_with_effects=0 fbw_store_journal_rollback_failed=0 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 -guard_failures=429 +guard_failures=427 internal_compile_panics=0 -loops_aborted=15 +loops_aborted=16 loops_compiled=54 retraces_compiled=0 diff --git a/pyre/bench/synth/trace_too_long_inline_multiframe.wasm.jitstats b/pyre/bench/synth/trace_too_long_inline_multiframe.wasm.jitstats index 4acf1693256..e1164e1a799 100644 --- a/pyre/bench/synth/trace_too_long_inline_multiframe.wasm.jitstats +++ b/pyre/bench/synth/trace_too_long_inline_multiframe.wasm.jitstats @@ -3,7 +3,7 @@ descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_blackhole_adopted_multi_frame=5 -fbw_blackhole_adopted_single_frame=7 +fbw_blackhole_adopted_single_frame=8 fbw_escape_plain_fallback=0 fbw_escape_plain_fallback_unclean=0 fbw_foriter_item_dropped=0 @@ -12,8 +12,8 @@ fbw_rolled_back_with_effects=0 fbw_store_journal_rollback_failed=0 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 -guard_failures=429 +guard_failures=427 internal_compile_panics=0 -loops_aborted=12 +loops_aborted=13 loops_compiled=53 retraces_compiled=0 diff --git a/pyre/gate-triage.md b/pyre/gate-triage.md index ba1135c9b55..fc189ed6894 100644 --- a/pyre/gate-triage.md +++ b/pyre/gate-triage.md @@ -222,12 +222,10 @@ defaults. Bridge inlining reaches module replacement on its own, so `PYRE_WASM_REEMIT` adds only the one-shot rebuild-with-unchanged-content that exercises the replacement machinery by itself. -`PYRE_JD1` is off for a third reason: the arm is incomplete rather than -wrong or unproven. pyre drives jd1 through the same `MetaInterp.tracing` -slot as the bytecode portal, so while a residual `next()` runs an -arbitrarily large Python computation the shared tracing flag suppresses -every jd0 merge point the generator body reaches. It stays dormant until -it has RPython's independent recursive-portal behavior. +`PYRE_JD1` is off because starting a second MetaInterp while one session +occupies `tracing` is still refused. Compiled-loop entry for another +driver is no longer suppressed: `maybe_compile_and_run` reads +`cell.flags & JC_TRACING` for that green key. `PYRE_FBW_INLINE_POISON` is off because its ON arm is known wrong, not merely unproven: the replay scan reports the pcs it objects to instead of collapsing @@ -243,7 +241,7 @@ build. |---|---|---| | PYRE_WASM_REEMIT | re-emits a compiled loop's wasm module into its own table slot once, on the first bridge installed against it | when the replacement path no longer needs an isolated arm | | PYRE_GUARD_RESUME_PC | prints the coordinate every walker-emitted guard resumes at (`resume_snapshot.rs guard_resume_pc_probe_enabled`); a guard whose `py_pc` is not the opcode it was emitted under re-executes the wrong bytecode on deopt, which reads as a livelock or a corrupted local rather than as a crash | the resume coordinate is covered by an ordinary test | -| PYRE_PORTAL_SPLIT | registers jd0 against the `warmspot.py split_graph_and_record_jitdriver` copy split immediately before `jit_merge_point`, instead of the unsplit `eval_loop_jit` graph; `=1` arms it and the prepass cache key includes the value | when the split portal path is the default and the unsplit registration arm is deleted | +| PYRE_PORTAL_SPLIT | default ON: jd0 registers against the `warmspot.py split_graph_and_record_jitdriver` copy split immediately before `jit_merge_point`; `=0`/`off`/`false` restores the unsplit `eval_loop_jit` graph | when the unsplit registration arm is deleted | | PYRE_WASM_INLINE_NONHEADER | admits an inlined region whose closing JUMP names a resumable LABEL other than the loop header AND whose source guard is in the LOOP BODY (`lib.rs inline_nonheader_enabled`); `=1`/`true`/`on` arms it. The preamble-sourced half of that class takes a different placement — blocks outside the header `loop`, body past its `end` — and is admitted unconditionally, so this flag now covers only the body-sourced half. Arming it removes 49.4M of the 257.3M cross-module crossings on the 81 fixtures that reach the decline and buys 0.74x/0.67x on two of them. The `spectral_norm` loss the retirement condition below was written against no longer reproduces: its two regions are deferred and their bridges never reach the trip count, so the flag leaves its crossings and its merges alike untouched. Across 536 bench fixtures, priced at the measured 0.67 ms per module + 0.493 ms/KB of cranelift and 4.3 ns per crossing, arming it models as 105.7 ms cheaper — four fixtures worth 188.6 ms against twenty-odd worth 83 ms, the worst being `kept_stack_deep_var_shortcircuit` at 53KB of added module for 40k crossings | the +18 ops per non-failing iteration it levies on the owner's fall-through is paid back on the fixtures it admits, measured on a machine quiet enough to grade wall clock rather than modelled | | PYRE_WASM_COMPILE_CENSUS | reports every cranelift compile of a trace module separately (`main.rs jit_compile_trace`) — the bytes handed over, the wall time it took, and whether the request was a first compile or the re-emission of an owner that took a merge. The stats line carries only the run's totals, which cannot separate a re-emission's cost from a first compile's nor say whether the per-module cost is linear in the bytes | trace compilation stops being on the critical path, or the two questions are answered and the answers stop moving | | PYRE_WASM_INLINE_EAGER_MAX_BYTES | `=N` declines the eager inline merge arm once the owner module it would re-emit is larger than N bytes (`lib.rs INLINE_EAGER_MAX_BYTES`, tally `inline_decl_eager_too_large`); unset leaves the built-in `DEFAULT_INLINE_EAGER_MAX_BYTES`, and `=4294967295` restores the unpriced arm. That arm merges before the compile returns so a quasi-immutable fold's dependencies attach to the owner's flag instead of a temporary bridge's, which is why no entry counter can reach it and `PYRE_WASM_INLINE_TRIP_BYTES` leaves it untouched — the owner's size is the only thing it can read. Set so the ceiling can be swept on ONE binary, the guest having no environment to read it from | the swept value becomes the built-in default, at which point this exists only to re-measure it | diff --git a/pyre/pyre-interpreter/src/baseobjspace.rs b/pyre/pyre-interpreter/src/baseobjspace.rs index 1ab035d2cd3..46a43ee51e2 100644 --- a/pyre/pyre-interpreter/src/baseobjspace.rs +++ b/pyre/pyre-interpreter/src/baseobjspace.rs @@ -14935,16 +14935,34 @@ fn _unpackiterable_unknown_length( let items = || pyre_object::gc_roots::shadow_stack_get(items_slot); // baseobjspace.py `greenkey = self.iterator_greenkey(w_iterator)`. let greenkey = iterator_greenkey(w_iterator()); + // `warmspot.py rewrite_jit_merge_point`: the original portal ends at + // `jit_merge_point` with `return portal_runner(*args)`. The split + // portal (`unpackiterable_portal`) owns the loop from that marker. + crate::call::unpack_portal_runner(greenkey, w_iterator(), items()) +} + +/// Split-portal body of `_unpackiterable_unknown_length`: the loop that +/// starts at `jit_merge_point`. `warmspot.py split_graph_and_record_jitdriver` +/// copies the original graph and cuts it immediately before the marker; +/// `rewrite_jit_merge_point` then makes the original return +/// `portal_runner(*args)`. `handle_jitexception` calls `portal_ptr(*args)` +/// with the merge-point greens and reds, not the `newlist_hint` prologue. +pub fn unpackiterable_portal( + greenkey: PyObjectRef, + w_iterator: PyObjectRef, + items: PyObjectRef, +) -> Result { + let _roots = pyre_object::gc_roots::push_roots(); + let root_base = pyre_object::gc_roots::shadow_stack_len(); + let _ = pyre_object::gc_roots::pin_root(w_iterator); + let _ = pyre_object::gc_roots::pin_root(items); + let w_iterator = || pyre_object::gc_roots::shadow_stack_get(root_base); + let items_slot = root_base + 1; + let items = || pyre_object::gc_roots::shadow_stack_get(items_slot); loop { - // baseobjspace.py:1012 - // `unpackiterable_driver.jit_merge_point(greenkey=greenkey)`. unpackiterable_driver.jit_merge_point(greenkey, w_iterator(), items()); match next(w_iterator()) { Ok(w_item) => unsafe { drain_append_at(items_slot, w_item) }, - // `except OperationError as e: if not e.match(space, - // w_StopIteration): raise; break` — the StopIteration test rides - // inside the handler (`e` is bound once, consumed only on the - // re-raise path), not as a match guard. Err(e) => { if e.matches_stop_iteration() { break; @@ -14953,13 +14971,6 @@ fn _unpackiterable_unknown_length( } } } - // `return items` — hand the grown `W_List` back as a single ref, matching - // the driver's `Type::Ref` result and RPython's `return items`. The - // `W_List` → `Vec` readback is caller-side host glue - // (`drain_collect_items`), kept out of the traced/blackholed drain body so - // the blackhole epilogue is a plain `ref_return` rather than a - // multi-word (`Vec`, sret-ABI) residual the single-register residual-call - // handlers cannot invoke. Ok(items()) } diff --git a/pyre/pyre-interpreter/src/call.rs b/pyre/pyre-interpreter/src/call.rs index 721281e2051..effd9b4632e 100644 --- a/pyre/pyre-interpreter/src/call.rs +++ b/pyre/pyre-interpreter/src/call.rs @@ -562,6 +562,9 @@ pub fn set_jit_param_string(text: &str) -> Result<(), ()> { /// `SET_JIT_PARAM_HOOK` / `EVAL_OVERRIDE` inversion pattern. /// `greenkey` is the merge-point green; `w_iterator` and `items` are the two /// `reds='auto'` values the JIT walk backs its InputArgs with. +/// +/// `warmspot.py rewrite_can_enter_jits` inserts `can_enter_jit` at the start +/// of a portal that has none; this hook is that `maybe_enter_jit` body. type UnpackMergeFn = fn(greenkey: PyObjectRef, w_iterator: PyObjectRef, items: PyObjectRef); static UNPACK_MERGE_HOOK: OnceLock = OnceLock::new(); @@ -578,6 +581,32 @@ pub fn unpack_merge_point(greenkey: PyObjectRef, w_iterator: PyObjectRef, items: } } +/// `warmspot.py rewrite_jit_merge_point`: the original portal graph ends in +/// `return portal_runner(*args)`. pyre-interpreter cannot import pyre-jit, so +/// the JIT registers the runner at boot. Without a hook the split portal +/// body runs directly — the no-JIT path. +type UnpackPortalRunnerFn = + fn(PyObjectRef, PyObjectRef, PyObjectRef) -> Result; +static UNPACK_PORTAL_RUNNER_HOOK: OnceLock = OnceLock::new(); + +pub fn register_unpack_portal_runner_hook(f: UnpackPortalRunnerFn) { + let _ = UNPACK_PORTAL_RUNNER_HOOK.set(f); +} + +/// `warmspot.py ll_portal_runner` for `unpackiterable_driver`. Falls back to +/// [`crate::unpackiterable_portal`] when the JIT has not installed a runner. +#[inline] +pub fn unpack_portal_runner( + greenkey: PyObjectRef, + w_iterator: PyObjectRef, + items: PyObjectRef, +) -> Result { + match UNPACK_PORTAL_RUNNER_HOOK.get() { + Some(f) => f(greenkey, w_iterator, items), + None => crate::unpackiterable_portal(greenkey, w_iterator, items), + } +} + thread_local! { static FORCE_PLAIN_EVAL: std::cell::Cell = const { std::cell::Cell::new(0) }; /// Last known valid execution context — for call_user_function_with_args. diff --git a/pyre/pyre-interpreter/src/runtime_ops.rs b/pyre/pyre-interpreter/src/runtime_ops.rs index b40deb3d617..76912f02827 100644 --- a/pyre/pyre-interpreter/src/runtime_ops.rs +++ b/pyre/pyre-interpreter/src/runtime_ops.rs @@ -1894,7 +1894,7 @@ pub extern "C" fn jit_exception_match(exc: i64, match_class: i64) -> i64 { } /// Ref-returning bridge for the `next(w_iterator)` residual call in -/// `_unpackiterable_unknown_length` (the `unpackiterable_driver` portal). +/// `unpackiterable_portal` (the `unpackiterable_driver` portal). /// /// Unlike [`jit_next`], StopIteration is published as an ordinary exception /// rather than collapsed to a null sentinel: the unpack loop body matches on diff --git a/pyre/pyre-jit-trace/build/prepass.rs b/pyre/pyre-jit-trace/build/prepass.rs index d0f8061a0b4..c02458f9758 100644 --- a/pyre/pyre-jit-trace/build/prepass.rs +++ b/pyre/pyre-jit-trace/build/prepass.rs @@ -239,14 +239,14 @@ fn forward_engine_env_aliases() { } } -/// `PYRE_PORTAL_SPLIT=1` registers the `eval::eval_loop_jit` driver against a -/// `warmspot.py split_graph_and_record_jitdriver` copy of the portal graph -/// split before its `jit_merge_point`, instead of against the graph holding -/// the marker. Listed in `LOWERING_GATE_ENV` above, which is both hashed -/// into `codegen_cache_key` and emitted as `cargo::rerun-if-env-changed`: -/// without both, flipping this serves the cached metadata snapshot. +/// `warmspot.py split_graph_and_record_jitdriver` is the default: jd0 is +/// registered against the copy split immediately before `jit_merge_point`. +/// `PYRE_PORTAL_SPLIT=0` restores the unsplit `eval_loop_jit` registration +/// for A/B. Listed in `LOWERING_GATE_ENV` so the cache key follows the +/// resolved boolean, not the raw unset string (unset used to mean unsplit). fn portal_split_enabled() -> bool { - std::env::var("PYRE_PORTAL_SPLIT").is_ok_and(|value| value == "1") + !std::env::var("PYRE_PORTAL_SPLIT") + .is_ok_and(|value| matches!(value.as_str(), "0" | "off" | "false")) } /// Entry of the real build: the translation prepass over the LLBC set. @@ -1070,13 +1070,18 @@ fn real_main() { split_portal: portal_split_enabled(), }, majit_translate::JitDriverSpec { - // pypy/interpreter/baseobjspace.py `_unpackiterable_unknown_length`; - // greens=['greenkey'], reds='auto' (baseobjspace.py `unpackiterable_driver`). + // `warmspot.py split_graph_and_record_jitdriver` copy of + // `_unpackiterable_unknown_length`, cut at `jit_merge_point`. + // greens=['greenkey'], reds='auto' (baseobjspace.py + // `unpackiterable_driver`). portal: majit_translate::CallPath::from_segments([ "baseobjspace", - "_unpackiterable_unknown_length", + "unpackiterable_portal", ]), - portal_runner: None, + portal_runner: Some(majit_translate::CallPath::from_segments([ + "eval", + "ll_unpackiterable_portal_runner_shim", + ])), greens: vec!["greenkey".to_string()], reds: vec![], green_kinds: vec![majit_ir::Type::Ref], @@ -1084,8 +1089,8 @@ fn real_main() { autoreds: true, virtualizables: vec![], red_types: vec![], - // `autoreds` drivers are not split; see the citation in - // `majit_translate::register_configured_jitdrivers`. + // Already the split body; `rewrite_jit_merge_point` lives + // in `_unpackiterable_unknown_length` at source level. split_portal: false, }, ], diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs index 183a652c5c1..89bc3bb8fb4 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs @@ -3747,9 +3747,9 @@ pub fn walk( // `note_root_trace_too_long` performs it instead — the // `find_biggest_function` → `disable_noninlinable_function` half as well // as the loop and bridge arms `prepare_trace_segmenting` (pyjitpl.py) - // keeps apart — and stages `ABORT_TOO_LONG` so the abort handler takes - // the staged reason rather than running that bookkeeping a second time - // against the log this one has already retired. + // keeps apart. `DispatchError::TraceTooLong` becomes the explicit + // `TraceAction::SwitchToBlackhole(ABORT_TOO_LONG)` unwind, so the catch + // consumes the reason without running this decision again. // // `blackhole_if_trace_too_long` raises AFTER `run_one_step`, so the // forward image must carry `pc`, the already-advanced `next_pc`, rather diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs index 2549ab0b43a..5b0dc3a2f3a 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs @@ -11614,7 +11614,7 @@ pub(crate) fn try_walker_specialize_builtin_zip( /// estimated, the same `history.length() > trace_limit` decides, and what /// happens on a yes is the abort `mod.rs` performs one opcode later -- /// `latch_abort_blackhole`, `note_root_trace_too_long` -/// (`stage_abort_reason(ABORT_TOO_LONG)`), `TraceTooLong`. The trace is +/// and the reason-bearing `TraceTooLong` unwind. The trace is /// discarded whole, so the half-built expansion above this point is never /// published; resuming re-executes the opcode from `pc`, which is the position /// every guard this expansion emits already side-exits to diff --git a/pyre/pyre-jit-trace/src/jitcode_runtime.rs b/pyre/pyre-jit-trace/src/jitcode_runtime.rs index c2dbb69fe1d..7e36ecd7a6e 100644 --- a/pyre/pyre-jit-trace/src/jitcode_runtime.rs +++ b/pyre/pyre-jit-trace/src/jitcode_runtime.rs @@ -323,7 +323,7 @@ pub fn portal_jitcode() -> Option> { /// Resolve the portal `JitCode` for the configured driver whose portal /// graph has canonical key `key` (e.g. a secondary driver's -/// `baseobjspace::_unpackiterable_unknown_length`). Per-driver analogue of +/// `baseobjspace::unpackiterable_portal`). Per-driver analogue of /// [`portal_jitcode`] — `warmspot.py:281-282` /// `jd.mainjitcode = self.get_jitcode(jd.portal_graph)`. pub fn portal_jitcode_for_key(key: &str) -> Option> { @@ -3701,9 +3701,17 @@ mod tests { // canonical object that build.rs persisted. let bt_jc = portal_jitcode().expect("configured portal must resolve to a jitcode"); assert!(!bt_jc.code.is_empty()); + // `warmspot.py split_graph_and_record_jitdriver` registers the copy + // cut at `jit_merge_point` (`eval::eval_loop_jit_portal`). The unsplit + // key remains when `PYRE_PORTAL_SPLIT=0`. let eval_driver = COMPILED_JIT_DRIVERS .iter() - .find(|driver| driver.portal.canonical_key() == "eval::eval_loop_jit") + .find(|driver| { + matches!( + driver.portal.canonical_key().as_str(), + "eval::eval_loop_jit" | "eval::eval_loop_jit_portal" + ) + }) .expect("compiled drivers must contain the main eval portal"); assert_eq!(eval_driver.main_jitcode_index, bt_jc.index()); assert_eq!( diff --git a/pyre/pyre-jit-trace/src/pyjitcode.rs b/pyre/pyre-jit-trace/src/pyjitcode.rs index 66aea237133..7a2c23131e0 100644 --- a/pyre/pyre-jit-trace/src/pyjitcode.rs +++ b/pyre/pyre-jit-trace/src/pyjitcode.rs @@ -1053,7 +1053,7 @@ impl PyJitCode { /// Wrap an already-populated runtime `JitCode` core with a degenerate /// (identity/empty) `PyJitCodeMetadata`. For a build-time-extracted - /// interpreter portal (e.g. jd1's `_unpackiterable_unknown_length`) whose + /// interpreter portal (e.g. jd1's `unpackiterable_portal`) whose /// CPython-pc↔jitcode-pc translation tables are degenerate and, on the /// jd1 `compile_loop`/resume path, read by no consumer — only /// `frame_value_count_at` runs, off the byte stream + `liveness_info`. diff --git a/pyre/pyre-jit-trace/src/state.rs b/pyre/pyre-jit-trace/src/state.rs index d4384898ee1..af537f78b59 100644 --- a/pyre/pyre-jit-trace/src/state.rs +++ b/pyre/pyre-jit-trace/src/state.rs @@ -841,7 +841,7 @@ pub fn install_jitcode_for( /// /// Unlike [`install_jitcode_for`] (which appends at `len()` and stamps a fresh /// slot), this honours the jitcode's baked absolute `JitCode::index` — a -/// build-time interpreter portal (jd1's `_unpackiterable_unknown_length`, +/// build-time interpreter portal (jd1's `unpackiterable_portal`, /// index 0) carries that index in its serialized `OnceLock`, and a resume frame /// records it verbatim (`dispatch.rs` `frame.jitcode.try_index()`). For the /// resume decoders to resolve `sd.jitcodes[index]` to the portal, it must sit @@ -3070,10 +3070,10 @@ pub struct PyreMeta { /// This stays separate from `valuestackdepth`, which is the live depth /// (`pyframe.py:111`) in the RPython model. pub array_capacity: usize, - /// Temporary staging count for extra portal reds that sit between the - /// frame red and the expanded virtualizable payload. Root portal traces - /// now carry `ec` here; guard-resume bridge traces still use 0 until the - /// resumedata path is migrated to the same contract. + /// Extra portal reds between the frame red and the virtualizable + /// payload. The Python portal carries `ec` here (`interp_jit.py` + /// `reds = ['frame', 'ec']`). Guard-resume rebuilds the same + /// contract in `rebuild_from_resumedata`. pub trace_extra_reds: usize, pub has_virtualizable: bool, #[vable(slot_types)] @@ -3984,25 +3984,14 @@ pub(crate) fn note_root_trace_too_long( // ever do here. let huge_fn = crate::driver::try_driver_pair().and_then(|(driver, _)| { let meta = driver.meta_interp_mut(); - // pyjitpl.py:2831 `raise SwitchToBlackhole(ABORT_TOO_LONG)`: the raise - // carries the reason from here to the `_interpret` catch, and the catch - // never re-runs the check that produced it. The walker's counterpart of - // that raise is the `DispatchError::TraceTooLong` the caller returns, - // which has no room for a `Counters.ABORT_*`, so the reason travels in - // the staging slot the abort handler consults FIRST. Staging it is - // therefore not only accounting: it is what stops the handler falling - // through to `MetaInterp::blackhole_if_trace_too_long`, whose bookkeeping - // this function has just performed. A second run reads the log this one - // retires below, so `find_biggest_function` answers `None` however the - // trace overflowed and the root takes `prepare_trace_segmenting`'s - // permanent `JC_FORCE_FINISH` + `JC_DONT_TRACE_HERE` even when an inlined - // callee was named here and disabled on its own. - meta.stage_abort_reason(majit_metainterp::counters::ABORT_TOO_LONG); + // pyjitpl.py `raise SwitchToBlackhole(ABORT_TOO_LONG)`: the caller's + // DispatchError::TraceTooLong becomes TraceAction::SwitchToBlackhole. + // The reason travels with the unwind, never in MetaInterp staging. // pyjitpl.py `jd_sd, greenkey_of_huge_function = self.find_biggest_function()`. let huge_fn = meta.find_biggest_function(); // pyjitpl.py `self.portal_trace_positions = None` — the log's `_pos` // cursors index the recorder this abort is discarding. - meta.portal_trace_positions = None; + meta.retire_portal_trace_positions(); if let Some((jd_no, huge_key)) = huge_fn.clone() { // pyjitpl.py:2821-2822 `jd_sd.warmstate.disable_noninlinable_function( // greenkey_of_huge_function)`. Upstream's `dont_trace_here` @@ -4085,18 +4074,30 @@ pub(crate) fn note_root_trace_too_long( /// and passed in — the driver is reached separately, exactly as /// [`note_root_trace_too_long`] does. /// -/// Returns whether an entry was appended; the caller must call -/// [`note_inline_subwalk_end`] if and only if it did, or the start/end walk -/// goes out of step. +/// Returns the owning driver when this is a recursive portal activation. +/// The caller pairs every such decision with [`note_inline_subwalk_end`], +/// including an abort that retires the log before the close. pub(crate) fn note_inline_subwalk_start( green_key: majit_metainterp::PortalGreenKey, pos: majit_metainterp::recorder::TracePosition, ) -> Option { let (driver, _) = crate::driver::try_driver_pair()?; + // Every Python callee re-enters the Python driver's declared portal. + // eval.rs registers that driver in slot 0; portal_jitcode resolves its + // actual mainjitcode from CompiledJitDriver, including a split portal. + // Read that JitCode's owner as pyjitpl.py `MetaInterp.newframe` does, + // rather than choosing the first recursive driver in the process. + let jitcode = crate::jitcode_runtime::portal_jitcode()?; let meta = driver.meta_interp_mut(); - // `is_main_jitcode(jitcode)` in its jitcode-free form: no recursive portal - // driver means upstream would not have logged this frame either. - let jd_no = meta.main_jitdriver_index()?; + if !meta.is_main_jitcode(&jitcode) { + return None; + } + let jd_no = jitcode.jitdriver_sd()?; + // pyjitpl.py `newframe`: ENTER_PORTAL_FRAME sits on the same greenkey + // path as the log append. The walker never builds an MIFrame, so this + // is the counterpart of that record. + let unique_id = meta.unique_id_for_greenkey(jd_no, &green_key); + meta.enter_portal_frame(jd_no, unique_id); meta.push_portal_trace_position(jd_no, Some(green_key), pos); // Counted HERE, not where the entry is appended: an abort retires the log // mid-sub-walk, so counting appends would read `push` far above `pop` for a @@ -4112,17 +4113,18 @@ pub(crate) fn note_inline_subwalk_end( jd_no: usize, pos: majit_metainterp::recorder::TracePosition, ) { - // Below the driver lookup, matching where `note_inline_subwalk_start` - // bumps 58: both counters then measure an APPENDED ENTRY, so an unclosed - // entry left by a driverless call shows up as `ptp_push != ptp_pop` rather - // than hiding behind equal counts. + // Like note_inline_subwalk_start, count the activation decision even + // when the abort has retired the log. These are not append counters. let Some((driver, _)) = crate::driver::try_driver_pair() else { return; }; majit_metainterp::mc_diag_bump(59); - driver - .meta_interp_mut() - .push_portal_trace_position(jd_no, None, pos); + let meta = driver.meta_interp_mut(); + // pyjitpl.py `popframe(leave_portal_frame=True)`: the walker close is + // a normal return, so LEAVE is recorded even when the abort has + // already retired the log. + meta.leave_portal_frame(jd_no); + meta.push_portal_trace_position(jd_no, None, pos); } /// Stage `reason` as the abort the walker is returning, so the single diff --git a/pyre/pyre-jit-trace/src/trace.rs b/pyre/pyre-jit-trace/src/trace.rs index 28fd6ee9be4..92cc279c871 100644 --- a/pyre/pyre-jit-trace/src/trace.rs +++ b/pyre/pyre-jit-trace/src/trace.rs @@ -6827,6 +6827,12 @@ fn full_body_walk_trace( ); } match e { + DE::TraceTooLong { .. } => TraceAction::SwitchToBlackhole( + majit_metainterp::SwitchToBlackhole { + reason: majit_metainterp::counters::ABORT_TOO_LONG, + raising_exception: false, + }, + ), // A kept-stack branch guard whose not-taken arm reads an // unrestorable boxed Ref register is a structural abort. // Keeping the permanent mapping is behavior-neutral: a plain diff --git a/pyre/pyre-jit-trace/src/unpack_state.rs b/pyre/pyre-jit-trace/src/unpack_state.rs index ba4a164282d..6d5d3bd036a 100644 --- a/pyre/pyre-jit-trace/src/unpack_state.rs +++ b/pyre/pyre-jit-trace/src/unpack_state.rs @@ -2,7 +2,7 @@ //! //! `baseobjspace.py:29` //! `unpackiterable_driver = JitDriver(greens=['greenkey'], reds='auto', ...)` -//! drives the unknown-length unpack loop `_unpackiterable_unknown_length` +//! drives the unknown-length unpack loop `unpackiterable_portal` //! (`baseobjspace.py:1003-1024`, merge at `:1012`). This module supplies the //! *dormant* second-driver types for the generic LLBC meta-tracer, via //! [`UnpackJitState`]'s [`JitState`] implementation, without touching jd0's @@ -53,10 +53,9 @@ impl JitCodeSym for UnpackSym { // body's op layout (e.g. how the `unpackiterable_driver` receiver read // lowers), so discover it rather than hardcode. The drain loop carries // exactly one merge point (`baseobjspace.py:1012`, no `can_enter_jit`). - let canonical = crate::jitcode_runtime::portal_jitcode_for_key( - "baseobjspace::_unpackiterable_unknown_length", - ) - .expect("jd1 portal jitcode must be registered"); + let canonical = + crate::jitcode_runtime::portal_jitcode_for_key("baseobjspace::unpackiterable_portal") + .expect("jd1 portal jitcode must be registered"); crate::jitcode_runtime::decoded_ops(&canonical.code) .find(|op| op.opname == "jit_merge_point") .expect("jd1 drain body must contain a jit_merge_point") @@ -184,10 +183,9 @@ mod tests { /// unit test. #[test] fn jd1_build_time_descrs_resolve_through_global_pool() { - let canonical = crate::jitcode_runtime::portal_jitcode_for_key( - "baseobjspace::_unpackiterable_unknown_length", - ) - .expect("jd1's extracted main JitCode must be registered"); + let canonical = + crate::jitcode_runtime::portal_jitcode_for_key("baseobjspace::unpackiterable_portal") + .expect("jd1's extracted main JitCode must be registered"); // The extracted body is the walkable unpack loop: exactly one merge // point, whose byte offset depends on the drain body's op layout // (`UnpackSym::loop_header_pc` discovers it rather than hardcoding). diff --git a/pyre/pyre-jit/src/eval.rs b/pyre/pyre-jit/src/eval.rs index 23c76807ced..c1aa35aef12 100644 --- a/pyre/pyre-jit/src/eval.rs +++ b/pyre/pyre-jit/src/eval.rs @@ -5538,6 +5538,7 @@ fn build_jit_driver_pair() -> JitDriverPair { // driver this process installs a Rust-side runner for; a second driver // registers its own under its own index. majit_metainterp::blackhole::register_portal_runner_hook(0, pyre_portal_runner); + majit_metainterp::blackhole::register_portal_runner_hook(1, unpackiterable_portal_runner); // pypy/module/pypyjit/interp_jit.py PyPyJitDriver(..., is_recursive=True). // Drives MetaInterp.is_main_jitcode() / is_portal_jitcode dispatch // — without this flag the recursive-portal bookkeeping stays @@ -5557,10 +5558,12 @@ fn build_jit_driver_pair() -> JitDriverPair { jd.result_type = majit_ir::Type::Ref; jd.virtualizable_info = Some(info.clone()); jd.portal_runner_adr = crate::call_jit::ll_portal_runner_shim as *const () as i64; + // warmstate.py get_unique_id(greenkey) → interp_jit.py get_unique_id. + jd.get_unique_id = Some(portal_unique_id_from_greens); d.meta_interp_mut().register_jitdriver_sd(jd); // baseobjspace.py `unpackiterable_driver = JitDriver(greens=['greenkey'], // reds='auto', ...)` — the second portal driver (jd1) for the - // unknown-length unpack loop `_unpackiterable_unknown_length`. Registered + // unknown-length unpack loop `unpackiterable_portal`. Registered // here, right after jd0, so it lands at `jitdrivers_sd[1]` and inherits the // same `portal_finishtoken` / `propagate_exc_descr` via the // `finish_setup_descrs_for_jitdrivers` tail. jd1 is novable @@ -5568,7 +5571,13 @@ fn build_jit_driver_pair() -> JitDriverPair { // vinfo-scan would elect jd0 for a jd1 trace; what keeps that from // happening is `drive_unpack_iterable_trace` handing the door this // registration's own slot. - let jd1 = pyre_jit_trace::unpack_state::UnpackJitState::unpackiterable_driver_descriptor(); + let mut jd1 = pyre_jit_trace::unpack_state::UnpackJitState::unpackiterable_driver_descriptor(); + jd1.result_type = majit_ir::Type::Ref; + // warmspot.py `jd.portal_runner_adr = adr_of(ll_portal_runner)` — every + // driver gets one, even a non-recursive autoreds portal. `get_portal_runner` + // / `compile_tmp_callback` read it; unpackiterable is not recursive so + // `do_recursive_call` will not jump here. + jd1.portal_runner_adr = ll_unpackiterable_portal_runner_shim as *const () as i64; d.meta_interp_mut().register_jitdriver_sd(jd1); // `warmspot.py metainterp_sd.finish_setup(codewriter)` always installs // the assembler's opcode ids and liveness stream before either tracing or @@ -6609,6 +6618,21 @@ pub fn get_unique_id( unsafe { pyre_interpreter::pycode::w_code_get_ptr(w_pycode) as usize } } +/// warmstate.py `get_unique_id(greenkey)` for the Python portal. +/// +/// Greens are `['next_instr', 'is_being_profiled', 'pycode']`. The hook +/// itself ignores the first two and answers the code object's unique id. +fn portal_unique_id_from_greens(greens: &[i64]) -> i64 { + match greens { + [next_instr, is_being_profiled, pycode] => get_unique_id( + *next_instr as usize, + *is_being_profiled != 0, + *pycode as pyre_object::PyObjectRef, + ) as i64, + _ => 0, + } +} + /// RPython interp_jit.py helper: get_location. pub fn get_location( next_instr: usize, @@ -7105,15 +7129,13 @@ fn set_jit_param_string_via_warmstate(text: &str) -> Result<(), ()> { } /// Gate for jd1 (`unpackiterable_driver`): the merge-point hook drives a -/// `JitCodeMachine` trace of `_unpackiterable_unknown_length` on hot unpack +/// `JitCodeMachine` trace of `unpackiterable_portal` on hot unpack /// sites, closing and compiling the drain loop. This remains opt-in with -/// `PYRE_JD1=1`: unlike RPython, pyre currently drives jd1 through the same -/// `MetaInterp.tracing` slot as the bytecode portal. A residual `next()` on a -/// generator can run an arbitrarily large Python computation before yielding; -/// while that happens the jd1 trace consists only of the opaque `next()` call, -/// but the shared tracing flag suppresses every jd0 merge point reached by the -/// generator body. Keep the incomplete second-driver experiment dormant until -/// it has RPython's independent recursive-portal behavior. It also follows the +/// `PYRE_JD1=1`. `maybe_compile_and_run` skips only the cell that carries +/// `JC_TRACING` for those greens (`warmstate.py`), so a jd1 session does +/// not suppress jd0 compiled-loop entry. Starting a second MetaInterp +/// while one session occupies `tracing` is still refused — pyre has one +/// MetaInterp object. It also follows the /// master JIT off-switches (`PYRE_NO_JIT`, `PYRE_JIT=0`) so "no JIT" means no /// jd1. /// @@ -7656,7 +7678,7 @@ fn jd1_counter_tick(green_key: u64) -> bool { /// jd1 (`unpackiterable_driver`) merge-point hook body. On the hot iterator /// type, drives one `JitCodeMachine` trace of the extracted -/// `_unpackiterable_unknown_length` loop with `w_iterator`/`items` as the two +/// `unpackiterable_portal` loop with `w_iterator`/`items` as the two /// `reds='auto'` values. Inert when jd1 is disabled (see /// [`jd1_experiment_enabled`]). fn unpack_merge_point_jit( @@ -7716,7 +7738,7 @@ fn drain_error_from_exc_ref(exc: i64) -> Option jc, None => { @@ -8238,6 +8260,7 @@ pub fn init_jit_hooks() { pyre_interpreter::call::register_set_jit_param_hook(set_jit_param_via_warmstate); pyre_interpreter::call::register_set_jit_param_string_hook(set_jit_param_string_via_warmstate); pyre_interpreter::call::register_unpack_merge_hook(unpack_merge_point_jit); + pyre_interpreter::call::register_unpack_portal_runner_hook(unpackiterable_ll_portal_runner); // Install the dict key `eq_w` / `hash_w` / `compares_by_identity` // trampolines here, at boot, before any user statement runs. They are // also registered inside the `JIT_DRIVER` initializer for the @@ -9416,6 +9439,8 @@ fn eval_with_jit_inner( pyre_interpreter::call::register_eval_override(eval_with_jit); pyre_interpreter::call::register_set_jit_param_hook(set_jit_param_via_warmstate); pyre_interpreter::call::register_set_jit_param_string_hook(set_jit_param_string_via_warmstate); + pyre_interpreter::call::register_unpack_merge_hook(unpack_merge_point_jit); + pyre_interpreter::call::register_unpack_portal_runner_hook(unpackiterable_ll_portal_runner); // The backend-agnostic registrations here — notably the JIT exception // raiser (`register_jit_exc_raiser`) that `jit_publish_exception` routes // residual-call raises through — are required on every backend; the @@ -9723,6 +9748,78 @@ pub(crate) fn pyre_portal_runner( } } +/// `warmspot.py ll_portal_runner` for `unpackiterable_driver`. +/// +/// `rewrite_jit_merge_point` makes the original `_unpackiterable_unknown_length` +/// return this runner. Function-entry `maybe_compile_and_run` is the loop-entry +/// hook already installed on the portal's `jit_merge_point`; this wrapper +/// owns only the activation call to `portal_ptr(*args)`. +fn unpackiterable_ll_portal_runner( + greenkey: pyre_object::PyObjectRef, + w_iterator: pyre_object::PyObjectRef, + items: pyre_object::PyObjectRef, +) -> Result { + pyre_interpreter::unpackiterable_portal(greenkey, w_iterator, items) +} + +/// C ABI of [`unpackiterable_ll_portal_runner`]. `warmspot.py` stores +/// `llmemory.cast_ptr_to_adr(portal_runner_ptr)` on the driver. +#[majit_macros::jit_may_force] +pub extern "C" fn ll_unpackiterable_portal_runner_shim( + greenkey: i64, + w_iterator: i64, + items: i64, +) -> i64 { + match unpackiterable_ll_portal_runner( + greenkey as pyre_object::PyObjectRef, + w_iterator as pyre_object::PyObjectRef, + items as pyre_object::PyObjectRef, + ) { + Ok(result) => result as i64, + Err(mut err) => { + pyre_interpreter::stack_check::park_jit_pending_error(err); + 0 + } + } +} + +/// `warmspot.py handle_jitexception` for `unpackiterable_driver`. +/// +/// Greens are `['greenkey']`; reds are auto `w_iterator`, `items`. +/// `portal_ptr(*args)` is the split-portal loop, not the `newlist_hint` +/// prologue and not the bytecode portal's frame runner. +fn unpackiterable_portal_runner( + exc: &majit_metainterp::jitexc::JitException, +) -> Result< + (majit_metainterp::blackhole::BhReturnType, i64), + majit_metainterp::blackhole::PortalRunnerFailure, +> { + use majit_metainterp::blackhole::{BhReturnType, PortalRunnerFailure}; + use majit_metainterp::jitexc::JitException; + + let JitException::ContinueRunningNormally(args) = exc else { + return Ok((BhReturnType::Void, 0)); + }; + let mut all_r = args.green_ref.clone(); + all_r.extend(&args.red_ref); + let greenkey = all_r.first().copied().unwrap_or(0) as pyre_object::PyObjectRef; + let w_iterator = all_r.get(1).copied().unwrap_or(0) as pyre_object::PyObjectRef; + let items = all_r.get(2).copied().unwrap_or(0) as pyre_object::PyObjectRef; + assert!( + !w_iterator.is_null() && !items.is_null(), + "unpackiterable portal runner: ContinueRunningNormally missing reds \ + (iterator/items); greens={} reds={}", + args.green_ref.len(), + args.red_ref.len(), + ); + match pyre_interpreter::unpackiterable_portal(greenkey, w_iterator, items) { + Ok(result) => Ok((BhReturnType::Ref, result as i64)), + Err(mut err) => Err(PortalRunnerFailure::jit( + JitException::ExitFrameWithExceptionRef(majit_ir::GcRef(err.to_exc_object() as usize)), + )), + } +} + /// warmspot.py handle_jitexception. /// /// RPython: CRN → portal_ptr(*args) re-invokes the interpreter. @@ -10791,8 +10888,10 @@ fn maybe_compile_and_run( return None; } } - // warmstate.py: JC_TRACING → skip entirely (no counter tick) - if driver.is_tracing() { + // warmstate.py: `cell.flags & JC_TRACING` → skip this key only. + // A live session on another key or driver must not suppress enter + // or the counter tick here. + if driver.cell_is_tracing(green_key) { return None; } // warmstate.py: procedure_token exists → EnterJitAssembler. diff --git a/pyre/pyre-jit/src/jit/codewriter.rs b/pyre/pyre-jit/src/jit/codewriter.rs index f3cc5cadc34..12fec65e739 100644 --- a/pyre/pyre-jit/src/jit/codewriter.rs +++ b/pyre/pyre-jit/src/jit/codewriter.rs @@ -9514,10 +9514,9 @@ impl CodeWriter { // frame (r2) feeds `get_builtin()`. // // This placeholder-plus-frame form is portal-only. - // In a non-portal callee the frame register aliases - // the outermost frame on a chained / inlined-callee - // resume, so the residual's frame operand names the - // wrong activation. A + // A non-portal callee graph declares its own + // frame/ec inputs; `frame_var` is that callee red, + // not the outermost portal frame. A // non-portal callee instead keeps the // `flowcontext.py find_global` const-fold, // which the inliner needs as a foldable constant call @@ -13174,83 +13173,28 @@ impl CodeWriter { push_and_bump!(result_value.into(), py_pc); } - // Both are frame methods whose only operand is the - // receiver, so both take the `emit_frontend_load_name` - // shape: `bh_load_locals_fn(frame)` / - // `bh_load_build_class_fn(frame)`. - // - // They occur only in a class body, which runs once per - // class definition, and that is why this arm used to - // emit `abort_permanent` instead. Execution frequency - // is the wrong measure: the full-body walk covers a - // frame statically, so one unlowerable op anywhere in - // the reachable region retires the whole frame, and a - // class body reached by the tracer at all — every one - // of them, once `threshold` is low — costs a - // `loops_aborted`. - // - // The two split on whether the receiver's identity can - // change the answer, because in a non-portal callee - // `frame_var` aliases the OUTERMOST frame (the same - // aliasing the LoadGlobal namespace split above - // describes, where it resolved the caller's `names` - // table). Threading the callee's own frame is not an - // option: an inlined callee has no materialised frame - // at all (`frame_ptr == 0`, `portal_frame_reg` - // unseeded), which is what inlining a virtualizable - // means. - // - // `load_locals` returns THIS frame's `w_locals` - // (`get_or_create_w_locals`), so an aliased receiver is - // a wrong value with no guard able to catch it, and - // there is no frame-free way to compute it. Portal - // only; the non-portal side keeps the permanent - // decline. - // - // `load_build_class` reads `frame.get_builtin()`, which - // under `objspace.honor__builtins__` false is - // `space.builtin` for every frame - // (`baseobjspace::frame_builtin_obj`), so the answer - // does not depend on which frame asks and the aliasing - // is harmless. That is a property of the flag, not of - // this arm, so assert it here: flipping the flag makes - // the receiver significant and turns this into exactly - // the LoadGlobal miscompile. + // pyopcode.py `PyFrame.LOAD_LOCALS` and + // `PyFrame.LOAD_BUILD_CLASS` read the live receiver. + // Every graph declares its own frame/ec inputs, and + // recursive call setup seeds the callee's own red frame + // even when this code was not registered as a portal. Instruction::LoadLocals | Instruction::LoadBuildClass => { - const _: () = assert!( - !pyre_interpreter::baseobjspace::HONOR_BUILTINS, - "honor__builtins__ makes frame.get_builtin() frame-specific, so \ - load_build_class can no longer take the portal-aliased frame in \ - a non-portal callee — gate it on is_true_portal like load_locals", - ); - let is_locals = matches!(instruction, Instruction::LoadLocals); - if is_locals && !is_true_portal { - push_fresh_ref(&mut current_state, &mut graph); - current_depth += 1; - emit_abort_permanent!(py_pc); + let opname = if matches!(instruction, Instruction::LoadLocals) { + "load_locals" } else { - let opname = if is_locals { - "load_locals" - } else { - "load_build_class" - }; - let loaded_dst_reg = stack_base + current_depth; - let result_value = emit_frontend_frame_only_ref( - &mut graph, - ¤t_block.block(), - opname, - frame_var.into(), - py_pc as i64, - ); - let result_fv: super::flow::FlowValue = result_value.into(); - current_state.stack.push(result_fv.clone()); - emit_pushvalue_ref!( - current_depth, - loaded_dst_reg, - result_fv, - py_pc - ); - } + "load_build_class" + }; + let loaded_dst_reg = stack_base + current_depth; + let result_value = emit_frontend_frame_only_ref( + &mut graph, + ¤t_block.block(), + opname, + frame_var.into(), + py_pc as i64, + ); + let result_fv: super::flow::FlowValue = result_value.into(); + current_state.stack.push(result_fv.clone()); + emit_pushvalue_ref!(current_depth, loaded_dst_reg, result_fv, py_pc); } // FormatSimple: pops value, pushes str(value). Net 0. @@ -16634,6 +16578,71 @@ mod tests { .expect("expected nested function code object") } + #[test] + fn non_portal_frame_loads_keep_their_receiver_after_lowering() { + use pyre_interpreter::bytecode::{CodeUnit, CodeUnits, OpArgByte}; + for instruction in [Instruction::LoadLocals, Instruction::LoadBuildClass] { + let mut code = first_nested_function_code("def f():\n return None\n"); + code.instructions = CodeUnits::from([ + CodeUnit::new(instruction, OpArgByte::new(0)), + CodeUnit::new(Instruction::ReturnValue, OpArgByte::new(0)), + ]); + let w_code = pyre_interpreter::box_code_constant(&code); + let code = unsafe { &*(pyre_interpreter::w_code_get_ptr(w_code) as *const CodeObject) }; + let writer = CodeWriter::new(); + // No setup_jitdriver: this is a callee graph that still declares + // frame/ec inputs, not a portal registration. + let pyjit = writer.transform_graph_to_jitcode(code).unwrap(); + assert!(pyjit.jitcode.jitdriver_sd().is_none()); + assert!( + !pyjit.has_abort, + "{instruction:?} must use the callee frame" + ); + assert_ne!(pyjit.metadata.portal_frame_reg, u16::MAX); + let calls: Vec<_> = pyre_jit_trace::jitcode_runtime::decoded_ops(&pyjit.jitcode.code) + .filter(|op| op.key == "residual_call_r_r/iRd>r") + .collect(); + assert!(!calls.is_empty(), "the frame method must survive lowering"); + // R-list follows the function Int register in this opcode. Each + // frame method takes precisely the graph's own frame red. + assert!( + calls.iter().any(|op| { + let code = &pyjit.jitcode.code; + code[op.pc + 2] == 1 + && code[op.pc + 3] as u16 == pyjit.metadata.portal_frame_reg + }), + "the residual must read this callee's frame register" + ); + } + } + + #[test] + fn non_portal_load_global_residual_reads_this_graph_frame() { + let code = first_nested_function_code("def f():\n return x\n"); + let w_code = pyre_interpreter::box_code_constant(&code); + let code = unsafe { &*(pyre_interpreter::w_code_get_ptr(w_code) as *const CodeObject) }; + let writer = CodeWriter::new(); + let pyjit = writer.transform_graph_to_jitcode(code).unwrap(); + assert!(pyjit.jitcode.jitdriver_sd().is_none()); + assert!(!pyjit.has_abort); + assert_ne!(pyjit.metadata.portal_frame_reg, u16::MAX); + let calls: Vec<_> = pyre_jit_trace::jitcode_runtime::decoded_ops(&pyjit.jitcode.code) + .filter(|op| op.key.contains("residual_call")) + .collect(); + assert!( + !calls.is_empty(), + "a non-foldable global must stay a residual" + ); + assert!( + calls.iter().any(|op| { + let code = &pyjit.jitcode.code; + (op.pc + 3..op.pc + 8) + .any(|i| code.get(i).copied() == Some(pyjit.metadata.portal_frame_reg as u8)) + }), + "the residual must name this callee's frame register" + ); + } + #[test] fn for_iter_jitcode_emits_stop_iteration_catch_arm() { let code = first_nested_function_code(