From 6b30254e46bbe85e1d61efac373dfc5247ae391d Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Mon, 10 Aug 2026 13:57:00 +0900 Subject: [PATCH 1/3] jit-trace: count the buckets of the bridge semantic-map py_pc fallback `bridge_semantic_maps_at_with_jitcode_pc` indexes the Python-PC-keyed `LiveVars::depth_at_py_pc` with its `pc` parameter on two legs: when the carried jitcode coordinate decodes but the twin tables hold no entry, and when no coordinate is carried at all. `bridge_semantic_maps_from_pc` passes one word into both the `pc` and `jitcode_pc` slots, and all five of its callers supply a JitCode byte offset -- three named `jitcode_pc` / `op_pc`, and two `RebuiltFrame::pc`, which `majit-ir/src/resumedata.rs` documents as the byte offset with the Python coordinate held in the separate `py_pc` field. Adds `PYRE_M73_EMPTYTWIN_CENSUS`, which sorts each fallback read into null_code / out_of_range / in_range_zero / in_range_nonzero and prints the first witness of every bucket per call site. Only in_range_nonzero reaches `setup_bridge_sym`'s `semantic_prefix_len`. Over the 397-file synth corpus the leg runs in 20 files, every one of them `site=non_decodable bucket=out_of_range`; in_range_nonzero is 0 and site=twin_miss does not execute. With the variable unset every counter path is skipped. `check.py --backend dynasm,cranelift` is 415/415 and 414/414. Assisted-by: Claude --- pyre/pyre-jit-trace/src/state.rs | 107 ++++++++++++++++++++++++++++--- 1 file changed, 99 insertions(+), 8 deletions(-) diff --git a/pyre/pyre-jit-trace/src/state.rs b/pyre/pyre-jit-trace/src/state.rs index 71bb189db62..0744bf65768 100644 --- a/pyre/pyre-jit-trace/src/state.rs +++ b/pyre/pyre-jit-trace/src/state.rs @@ -1791,6 +1791,97 @@ pub(crate) struct BridgeSemanticMaps { pub pcdep_entries: Vec<(u8, u16, u16)>, } +/// Measurement gate for the `via_py_pc` fallback of +/// [`bridge_semantic_maps_at_with_jitcode_pc`]. The fallback indexes the +/// Python-PC-keyed `LiveVars::depth_at_py_pc` with that function's `pc` +/// parameter, but `bridge_semantic_maps_from_pc` supplies ONE word for both +/// the `pc` and `jitcode_pc` slots, and five of its callers pass a JitCode +/// byte offset (`RebuiltFrame::pc` is documented as the byte offset, with the +/// Python coordinate in the separate `py_pc` field). +/// +/// The partition that decides whether that mis-keying is observable: +/// an offset past the end of the table reads `None` and the fallback already +/// answers 0, but an offset that lands inside the table returns a real depth +/// belonging to a different coordinate. `in_range` with a non-zero depth is +/// the only case that reaches `setup_bridge_sym`'s `semantic_prefix_len`. +/// +/// Pure telemetry — with the variable unset every counter path is skipped and +/// the fallback's value is unchanged. +fn empty_twin_census_enabled() -> bool { + static E: std::sync::OnceLock = std::sync::OnceLock::new(); + *E.get_or_init(|| std::env::var_os("PYRE_M73_EMPTYTWIN_CENSUS").is_some()) +} + +/// One counter set per call site. The heartbeat has to be per site, not per +/// process: with a single shared counter only the site that fires first is +/// ever named, so a run in which one leg never executed is indistinguishable +/// from one in which it executed second. +struct EmptyTwinSite { + name: &'static str, + hits: std::sync::atomic::AtomicU64, + null_code: std::sync::atomic::AtomicU64, + out_of_range: std::sync::atomic::AtomicU64, + in_range_zero: std::sync::atomic::AtomicU64, + in_range_nonzero: std::sync::atomic::AtomicU64, +} + +impl EmptyTwinSite { + const fn new(name: &'static str) -> Self { + const fn zero() -> std::sync::atomic::AtomicU64 { + std::sync::atomic::AtomicU64::new(0) + } + Self { + name, + hits: zero(), + null_code: zero(), + out_of_range: zero(), + in_range_zero: zero(), + in_range_nonzero: zero(), + } + } +} + +/// The carried jitcode coordinate decoded, but the twin tables had no entry. +static EMPTY_TWIN_MISS: EmptyTwinSite = EmptyTwinSite::new("twin_miss"); +/// No jitcode coordinate was carried at all. +static EMPTY_TWIN_NON_DECODABLE: EmptyTwinSite = EmptyTwinSite::new("non_decodable"); + +fn empty_twin_census(site: &EmptyTwinSite, rp: usize, table_len: Option, depth: u16) { + if !empty_twin_census_enabled() { + return; + } + use std::sync::atomic::Ordering; + let name = site.name; + let hits = site.hits.fetch_add(1, Ordering::Relaxed) + 1; + // `in_range_nonzero` is the only bucket that can reach `setup_bridge_sym`'s + // `semantic_prefix_len`: a real depth read at a coordinate the caller never + // meant as a Python PC. + let (bucket, counter, live) = match table_len { + None => ("null_code", &site.null_code, false), + Some(len) if rp >= len => ("out_of_range", &site.out_of_range, false), + Some(_) if depth == 0 => ("in_range_zero", &site.in_range_zero, false), + Some(_) => ("in_range_nonzero", &site.in_range_nonzero, true), + }; + let n = counter.fetch_add(1, Ordering::Relaxed) + 1; + // Every bucket announces its first witness, so the buckets that do fire are + // the positive control for the ones that do not: a bucket that stays silent + // on a stream carrying other buckets' lines is empty, not unprinted. + if n == 1 || (live && n <= 20) { + eprintln!( + "[M73-EMPTYTWIN] site={name} bucket={bucket} n={n} rp={rp} len={table_len:?} depth={depth}" + ); + } + if hits % 100_000 == 0 { + eprintln!( + "[M73-EMPTYTWIN] totals site={name} hits={hits} null_code={} out_of_range={} in_range_zero={} in_range_nonzero={}", + site.null_code.load(Ordering::Relaxed), + site.out_of_range.load(Ordering::Relaxed), + site.in_range_zero.load(Ordering::Relaxed), + site.in_range_nonzero.load(Ordering::Relaxed), + ); + } +} + /// When a kept-stack branch guard carries the guard's own jitcode /// coordinate (`jitcode_pc != NO_JITCODE_PC`), the pcdep/depth tables /// must be keyed at the GUARD's Python PC — the encode side @@ -1830,15 +1921,15 @@ pub(crate) fn bridge_semantic_maps_at_with_jitcode_pc( // `liveness_py_pc = guard_py_pc` keying. The guard PC is where the // computed kept operand-stack temps are live; at the merge-target PC // they've been consumed and carry no pcdep entry. - let via_py_pc = |rp: usize| -> usize { + let via_py_pc = |rp: usize, site: &'static EmptyTwinSite| -> usize { if payload.code_ptr.is_null() { + empty_twin_census(site, rp, None, 0); return 0; } - crate::liveness::liveness_for(payload.code_ptr) - .depth_at_py_pc() - .get(rp) - .copied() - .unwrap_or(0) as usize + let table = crate::liveness::liveness_for(payload.code_ptr).depth_at_py_pc(); + let depth = table.get(rp).copied().unwrap_or(0); + empty_twin_census(site, rp, Some(table.len()), depth); + depth as usize }; let (stack_depth_at_pc, pcdep_entries) = if jitcode_pc >= 0 && payload @@ -1860,12 +1951,12 @@ pub(crate) fn bridge_semantic_maps_at_with_jitcode_pc( payload.pcdep_for_jitcode_pc(jp), ) { (Some(depth), Some(pcdep)) => (depth as usize, pcdep), - _ => (via_py_pc(pc as usize), Vec::new()), + _ => (via_py_pc(pc as usize, &EMPTY_TWIN_MISS), Vec::new()), } } else { // A non-decodable carried coordinate falls back to the merge-target // PC so liveness and pcdep key the same point. - (via_py_pc(pc as usize), Vec::new()) + (via_py_pc(pc as usize, &EMPTY_TWIN_NON_DECODABLE), Vec::new()) }; BridgeSemanticMaps { // #73: the codewriter colored this jitcode iff `pcdep_color_slots` From 397320f4d6dac7c7fa5e37c6d3afb00470e84665 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Mon, 10 Aug 2026 19:03:39 +0900 Subject: [PATCH 2/3] jit-trace: give the bridge semantic-map lookup an absent Python coordinate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `bridge_semantic_maps_at_with_jitcode_pc` indexes the Python-PC-keyed `LiveVars::depth_at_py_pc` with its second parameter. Six of its seven call sites reach it holding a JitCode byte offset: the five through `bridge_semantic_maps_from_pc`, which passed one word into both the Python and the JitCode slot, and `resume_snapshot.rs`'s `marker`, which is `inline_call_return_marker`'s `decode_op_at(..).next_pc`. `py_coord.rs` states the invariant they violate: "runtime consumers never project a JitCode PC through a Python-keyed map". The second parameter becomes `Option`, and the fallback answers 0 without reading the table when it is `None`. `bridge_semantic_maps_from_pc` becomes `bridge_semantic_maps_from_jitcode_pc` and passes `None`; only `walker_capture_snapshot_for_last_guard_impl`'s `liveness_py_pc` stays `Some`. The sibling `try_frame_liveness_reg_indices_by_bank_at_with_jitcode_pc` already declines on an absent coordinate rather than reconstructing one. This does not derive a depth from the JitCode offset. The fallback reads the static dense liveness, while the jitcode-keyed twin carries the walk-visited `depth_at_pc`; `codewriter.rs:14808` records these as deliberately different tables, and `depth_at_pc` is not published into `PyJitCodeMetadata`. Census over the synth corpus, before and after: the same 20 files reach the leg, moving from `bucket=out_of_range` (rp=231..1214 against table lengths 28..257) to `bucket=no_py_pc`. `in_range_nonzero` — the only bucket reaching `setup_bridge_sym`'s `semantic_prefix_len` — is 0 in both. The census now shares `py_coord::emptytwin_census_enabled` instead of re-reading `PYRE_M73_EMPTYTWIN_CENSUS`, which that module already owns, and prints `[m73-bridge-maps]` so its lines stay distinct from `note_empty_twin_fallback`'s. `check.py --backend dynasm,cranelift`: cranelift 416/416; dynasm 416 passed with `test.test_format` failing at `test_locale`, which reproduces under `PYRE_NO_JIT=1`. Assisted-by: Claude --- .../src/jitcode_dispatch/bridge_subwalk.rs | 3 +- .../src/jitcode_dispatch/residual_call.rs | 3 +- .../src/jitcode_dispatch/resume_snapshot.rs | 12 +- pyre/pyre-jit-trace/src/state.rs | 121 +++++++++++------- 4 files changed, 88 insertions(+), 51 deletions(-) diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rs index ab4798844bd..c4a93b0e597 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rs @@ -925,7 +925,8 @@ pub(crate) fn recipe_parent_frame_from_recipe( recipe.jitcode_pc, ); let stack_only = recipe.valuestackdepth.saturating_sub(recipe.nlocals); - let maps = crate::state::bridge_semantic_maps_from_pc(recipe.jitcode_index, recipe.jitcode_pc); + let maps = + crate::state::bridge_semantic_maps_from_jitcode_pc(recipe.jitcode_index, recipe.jitcode_pc); let null_ref = ctx.const_ref(pyre_object::PY_NULL as i64); let mut boxes = Vec::with_capacity(banks.total_len()); // Per-color `(color, box)` pairs for the blackhole capture below, collected diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs index c577daea295..683ab8eea5a 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs @@ -2171,7 +2171,8 @@ fn carrier_stack_box_for_ref_arg( return None; } let nlocals = unsafe { (&(*raw_code).varnames).len() }; - let maps = crate::state::bridge_semantic_maps_from_pc(consts.jitcode_index, op_pc as i32); + let maps = + crate::state::bridge_semantic_maps_from_jitcode_pc(consts.jitcode_index, op_pc as i32); let semantic = crate::state::semantic_ref_slot_for_reg_color( nlocals, maps.stack_depth_at_pc, diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/resume_snapshot.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/resume_snapshot.rs index 133afaab554..af7fcf1c30e 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/resume_snapshot.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/resume_snapshot.rs @@ -1004,7 +1004,7 @@ pub(crate) fn walker_capture_snapshot_for_last_guard_impl( if after_residual_call && sym.owns_virtualizable_shadow() { let maps = crate::state::bridge_semantic_maps_at_with_jitcode_pc( jitcode_index as i32, - liveness_py_pc as i32, + Some(liveness_py_pc as i32), guard_jitcode_pc, ); let banks = crate::state::frame_liveness_reg_indices_by_bank_at_with_jitcode_pc( @@ -2058,9 +2058,12 @@ pub(crate) fn compute_nested_inline_caller_frame( // `depth` is the operand-stack depth at the CALL return point, // top slot = the pending nested-call result. let pending_top_slot = nlocals + depth - 1; + // No Python coordinate here: `marker` is + // `inline_call_return_marker`'s `decode_op_at(..).next_pc`, a + // JitCode byte offset. let maps = crate::state::bridge_semantic_maps_at_with_jitcode_pc( jitcode_index as i32, - marker as i32, + None, caller_liveness_word, ); let array_descr = crate::state::pyobject_gcarray_descr(); @@ -2526,7 +2529,10 @@ pub(crate) fn walker_capture_multi_frame_inline_snapshot( let mut recovered_regs_r = ctx.registers_r.to_vec(); let code = unsafe { &*callee_pjc.code_ptr }; let (stack_base, _) = crate::state::callee_layout_for_call_assembler(code); - let maps = crate::state::bridge_semantic_maps_from_pc(callee_jitcode_index, callee_jitcode_pc); + let maps = crate::state::bridge_semantic_maps_from_jitcode_pc( + callee_jitcode_index, + callee_jitcode_pc, + ); // The kept-stack guard gate is certified by this callee frame's // operand-stack mirror. Publish the same boxes into the carried // coordinate's Ref colors before collecting the frame snapshot, so the diff --git a/pyre/pyre-jit-trace/src/state.rs b/pyre/pyre-jit-trace/src/state.rs index 0744bf65768..9caa645f47d 100644 --- a/pyre/pyre-jit-trace/src/state.rs +++ b/pyre/pyre-jit-trace/src/state.rs @@ -1791,34 +1791,30 @@ pub(crate) struct BridgeSemanticMaps { pub pcdep_entries: Vec<(u8, u16, u16)>, } -/// Measurement gate for the `via_py_pc` fallback of -/// [`bridge_semantic_maps_at_with_jitcode_pc`]. The fallback indexes the -/// Python-PC-keyed `LiveVars::depth_at_py_pc` with that function's `pc` -/// parameter, but `bridge_semantic_maps_from_pc` supplies ONE word for both -/// the `pc` and `jitcode_pc` slots, and five of its callers pass a JitCode -/// byte offset (`RebuiltFrame::pc` is documented as the byte offset, with the -/// Python coordinate in the separate `py_pc` field). +/// Per-call-site counters for the `via_py_pc` fallback of +/// [`bridge_semantic_maps_at_with_jitcode_pc`], which indexes the +/// Python-PC-keyed `LiveVars::depth_at_py_pc`. /// -/// The partition that decides whether that mis-keying is observable: -/// an offset past the end of the table reads `None` and the fallback already -/// answers 0, but an offset that lands inside the table returns a real depth -/// belonging to a different coordinate. `in_range` with a non-zero depth is -/// the only case that reaches `setup_bridge_sym`'s `semantic_prefix_len`. +/// The partition that decides whether a Python coordinate is being read at a +/// point the caller did not mean: an offset past the end of the table reads +/// `None` and the fallback answers 0, but one that lands inside returns a real +/// depth belonging to a different instruction. `in_range_nonzero` is the only +/// bucket that reaches `setup_bridge_sym`'s `semantic_prefix_len`, so it is the +/// standing check on the one caller that still supplies a `Some` coordinate +/// (`resume_snapshot.rs` `walker_capture_snapshot_for_last_guard_impl`, +/// `liveness_py_pc`); a fire there means that argument is not a Python PC after +/// all. +/// +/// The counters are per site, not per process: with one shared counter only the +/// site that fires first is ever named, so a run in which one leg never +/// executed is indistinguishable from one in which it executed second. /// /// Pure telemetry — with the variable unset every counter path is skipped and /// the fallback's value is unchanged. -fn empty_twin_census_enabled() -> bool { - static E: std::sync::OnceLock = std::sync::OnceLock::new(); - *E.get_or_init(|| std::env::var_os("PYRE_M73_EMPTYTWIN_CENSUS").is_some()) -} - -/// One counter set per call site. The heartbeat has to be per site, not per -/// process: with a single shared counter only the site that fires first is -/// ever named, so a run in which one leg never executed is indistinguishable -/// from one in which it executed second. struct EmptyTwinSite { name: &'static str, hits: std::sync::atomic::AtomicU64, + no_py_pc: std::sync::atomic::AtomicU64, null_code: std::sync::atomic::AtomicU64, out_of_range: std::sync::atomic::AtomicU64, in_range_zero: std::sync::atomic::AtomicU64, @@ -1833,6 +1829,7 @@ impl EmptyTwinSite { Self { name, hits: zero(), + no_py_pc: zero(), null_code: zero(), out_of_range: zero(), in_range_zero: zero(), @@ -1846,8 +1843,16 @@ static EMPTY_TWIN_MISS: EmptyTwinSite = EmptyTwinSite::new("twin_miss"); /// No jitcode coordinate was carried at all. static EMPTY_TWIN_NON_DECODABLE: EmptyTwinSite = EmptyTwinSite::new("non_decodable"); -fn empty_twin_census(site: &EmptyTwinSite, rp: usize, table_len: Option, depth: u16) { - if !empty_twin_census_enabled() { +fn empty_twin_census( + site: &EmptyTwinSite, + rp: Option, + table_len: Option, + depth: u16, +) { + // Shares `PYRE_M73_EMPTYTWIN_CENSUS` with `py_coord::note_empty_twin_fallback`, + // so one run reports both. That one prints `[m73-emptytwin]`; this one prints + // `[m73-bridge-maps]`, distinct under a case-insensitive filter too. + if !crate::py_coord::emptytwin_census_enabled() { return; } use std::sync::atomic::Ordering; @@ -1855,12 +1860,13 @@ fn empty_twin_census(site: &EmptyTwinSite, rp: usize, table_len: Option, let hits = site.hits.fetch_add(1, Ordering::Relaxed) + 1; // `in_range_nonzero` is the only bucket that can reach `setup_bridge_sym`'s // `semantic_prefix_len`: a real depth read at a coordinate the caller never - // meant as a Python PC. - let (bucket, counter, live) = match table_len { - None => ("null_code", &site.null_code, false), - Some(len) if rp >= len => ("out_of_range", &site.out_of_range, false), - Some(_) if depth == 0 => ("in_range_zero", &site.in_range_zero, false), - Some(_) => ("in_range_nonzero", &site.in_range_nonzero, true), + // meant as a Python PC. `no_py_pc` is the leg that used to read one. + let (bucket, counter, live) = match (rp, table_len) { + (None, _) => ("no_py_pc", &site.no_py_pc, false), + (Some(_), None) => ("null_code", &site.null_code, false), + (Some(rp), Some(len)) if rp >= len => ("out_of_range", &site.out_of_range, false), + (Some(_), Some(_)) if depth == 0 => ("in_range_zero", &site.in_range_zero, false), + (Some(_), Some(_)) => ("in_range_nonzero", &site.in_range_nonzero, true), }; let n = counter.fetch_add(1, Ordering::Relaxed) + 1; // Every bucket announces its first witness, so the buckets that do fire are @@ -1868,12 +1874,13 @@ fn empty_twin_census(site: &EmptyTwinSite, rp: usize, table_len: Option, // on a stream carrying other buckets' lines is empty, not unprinted. if n == 1 || (live && n <= 20) { eprintln!( - "[M73-EMPTYTWIN] site={name} bucket={bucket} n={n} rp={rp} len={table_len:?} depth={depth}" + "[m73-bridge-maps] site={name} bucket={bucket} n={n} rp={rp:?} len={table_len:?} depth={depth}" ); } if hits % 100_000 == 0 { eprintln!( - "[M73-EMPTYTWIN] totals site={name} hits={hits} null_code={} out_of_range={} in_range_zero={} in_range_nonzero={}", + "[m73-bridge-maps] totals site={name} hits={hits} no_py_pc={} null_code={} out_of_range={} in_range_zero={} in_range_nonzero={}", + site.no_py_pc.load(Ordering::Relaxed), site.null_code.load(Ordering::Relaxed), site.out_of_range.load(Ordering::Relaxed), site.in_range_zero.load(Ordering::Relaxed), @@ -1891,9 +1898,16 @@ fn empty_twin_census(site: &EmptyTwinSite, rp: usize, table_len: Option, /// through the carried `jitcode_pc`. Without this the decode-side /// color→slot inversion reads the merge-target PC's pcdep (where the /// computed kept temp is dead), leaving the temp as `OpRef::NONE`. +/// `py_pc` is the caller's OWN Python coordinate, or `None` when it holds +/// only a JitCode coordinate. It is not interchangeable with `jitcode_pc`: +/// it indexes the Python-PC-keyed static liveness, and `py_coord.rs`'s +/// module invariant is that "runtime consumers never project a JitCode PC +/// through a Python-keyed map". `None` is what makes that statable — +/// passing the JitCode word here reads a foreign instruction's depth +/// whenever the offset happens to fall inside the table. pub(crate) fn bridge_semantic_maps_at_with_jitcode_pc( jitcode_index: i32, - pc: i32, + py_pc: Option, jitcode_pc: i32, ) -> BridgeSemanticMaps { ensure_finish_setup(); @@ -1921,14 +1935,25 @@ pub(crate) fn bridge_semantic_maps_at_with_jitcode_pc( // `liveness_py_pc = guard_py_pc` keying. The guard PC is where the // computed kept operand-stack temps are live; at the merge-target PC // they've been consumed and carry no pcdep entry. - let via_py_pc = |rp: usize, site: &'static EmptyTwinSite| -> usize { + // + // A caller holding no Python coordinate answers 0: the static + // liveness table cannot be queried without a Python PC, and the + // JitCode offset is not one. `codewriter.rs:14808` records that this + // table is deliberately NOT the walk-visited `depth_at_pc` the + // jitcode-keyed twins carry, so there is no jitcode-keyed spelling of + // this read to fall back to either. + let via_py_pc = |rp: Option, site: &'static EmptyTwinSite| -> usize { + let Some(rp) = rp.and_then(|p| usize::try_from(p).ok()) else { + empty_twin_census(site, None, None, 0); + return 0; + }; if payload.code_ptr.is_null() { - empty_twin_census(site, rp, None, 0); + empty_twin_census(site, Some(rp), None, 0); return 0; } let table = crate::liveness::liveness_for(payload.code_ptr).depth_at_py_pc(); let depth = table.get(rp).copied().unwrap_or(0); - empty_twin_census(site, rp, Some(table.len()), depth); + empty_twin_census(site, Some(rp), Some(table.len()), depth); depth as usize }; let (stack_depth_at_pc, pcdep_entries) = if jitcode_pc >= 0 @@ -1941,22 +1966,20 @@ pub(crate) fn bridge_semantic_maps_at_with_jitcode_pc( // `jitcode_pc` via the predecessor-keyed twins. Every real carried // coordinate is an op-start or block-head offset of a colored // jitcode, for which the codewriter seeds these twins; a twin miss - // degrades to the merge-target PC (the same fallback the - // non-decodable path uses below), keeping liveness and pcdep on one - // coordinate. A portal bridge is uncolored, so `via_py_pc` returns - // `(0, empty)` for any resolved py; the merge-target fallback is - // identical to the containing JitCode-PC coordinate lookup. + // degrades to the caller's own merge-target Python PC, keeping + // liveness and pcdep on one coordinate, and to 0 when the caller + // has none. match ( payload.depth_for_jitcode_pc_pred(jp), payload.pcdep_for_jitcode_pc(jp), ) { (Some(depth), Some(pcdep)) => (depth as usize, pcdep), - _ => (via_py_pc(pc as usize, &EMPTY_TWIN_MISS), Vec::new()), + _ => (via_py_pc(py_pc, &EMPTY_TWIN_MISS), Vec::new()), } } else { // A non-decodable carried coordinate falls back to the merge-target // PC so liveness and pcdep key the same point. - (via_py_pc(pc as usize, &EMPTY_TWIN_NON_DECODABLE), Vec::new()) + (via_py_pc(py_pc, &EMPTY_TWIN_NON_DECODABLE), Vec::new()) }; BridgeSemanticMaps { // #73: the codewriter colored this jitcode iff `pcdep_color_slots` @@ -1969,8 +1992,13 @@ pub(crate) fn bridge_semantic_maps_at_with_jitcode_pc( }) } -pub(crate) fn bridge_semantic_maps_from_pc(jitcode_index: i32, pc: i32) -> BridgeSemanticMaps { - bridge_semantic_maps_at_with_jitcode_pc(jitcode_index, pc, pc) +/// For a caller that holds only a JitCode coordinate. Named for that +/// coordinate so the word cannot be re-routed into the Python-PC slot. +pub(crate) fn bridge_semantic_maps_from_jitcode_pc( + jitcode_index: i32, + jitcode_pc: i32, +) -> BridgeSemanticMaps { + bridge_semantic_maps_at_with_jitcode_pc(jitcode_index, None, jitcode_pc) } /// Operand-stack Ref constants (`(semantic_slot, raw_ref)`) at the resume PC of @@ -7715,7 +7743,7 @@ fn reconstruct_inline_recipe( let (pframe_reg, pec_reg) = portal_red_regs_at(frame.jitcode_index); let (pframe_reg, pec_reg) = (pframe_reg as u32, pec_reg as u32); let has_frame = reg_indices.ref_.contains(&pframe_reg); - let maps = bridge_semantic_maps_from_pc(frame.jitcode_index, frame.pc); + let maps = bridge_semantic_maps_from_jitcode_pc(frame.jitcode_index, frame.pc); // resume.py:1042-1057 `rebuild_from_resumedata` / `consume_boxes` rebuilds // every paused frame and fills EVERY live register from the resume stream // with no shape classifier. Pyre recovers a portal callee's locals from the @@ -9999,7 +10027,8 @@ impl JitState for PyreJitState { // `[0,nlocals)` prefix to identity colors (now retired). Invert each // live color to its slot via `semantic_ref_slot_for_reg_color` so the // mirror is correct under freely-colored locals. - let maps = crate::state::bridge_semantic_maps_from_pc(frame0.jitcode_index, frame0.pc); + let maps = + crate::state::bridge_semantic_maps_from_jitcode_pc(frame0.jitcode_index, frame0.pc); // For a kept-stack branch guard, the vable's runtime // `valuestackdepth` reflects the merge-target depth (post // consumption) rather than the guard's deeper live depth. The From a2bb94ddb14a8ab6b47ef1cffe6e268fdd8502f3 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Mon, 10 Aug 2026 23:40:20 +0900 Subject: [PATCH 3/3] jit-trace: correct the bridge semantic-map fallback's comments and census MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comment justifying the `None` fallback's 0 asserted that "there is no jitcode-keyed spelling of this read to fall back to either", citing `codewriter.rs:14808`. Three such spellings exist: `depth_containing_for_jitcode_pc`, built in `finalize_jitcode`'s `depth_containing_by_jit_pc` block to reproduce `depth_at_py_pc[containing_py]` for every offset, plus the trivia and block-head twins. `collect_outer_active_boxes`, the encode half this function mirrors, already reads its own `stack_depth_at_pc` from those twins with no Python PC in hand. The cited line is the closing brace of an unrelated `[pcmap-residual]` block; the passage it meant is `finalize_jitcode`'s `static_depth` construction, and it records the converse — the trivia twins deliberately carry the static `depth_at_py_pc`, and `depth_pred_by_jit_pc` alone carries the walk-visited `depth_at_pc`. The replacement cites by symbol rather than by line. `empty_twin_census` gains `jit_pc` and `twin`: the coordinate the leg declined on, and what `depth_containing_for_jitcode_pc` would answer there. Over the 399-file synth corpus all 20 executions carry `jit_pc >= 0` with `twin = Some(3..=5)`, so routing the fallback through the twin would change the reconstructed frame's width rather than nothing. The leg still answers 0; what it declines is now recorded instead of asserted away. The counters remain per leg, so those 20 are still unattributed to a call site — the comment says so, because `setup_bridge_sym` is the only caller whose `stack_depth_at_pc` read is unconditional. The census doc claimed `in_range_nonzero` is the only bucket reaching `setup_bridge_sym`'s `semantic_prefix_len`. The preceding commit made that site pass `None`, so the bucket is unreachable from there; the doc now names `no_py_pc` as that site's only bucket, and `semantic_limit` in `walker_capture_snapshot_for_last_guard_impl` as the consumer of the one `Some` caller's depth. `EMPTY_TWIN_NON_DECODABLE`'s doc said no coordinate was carried, while its branch also covers a carried offset that does not decode. `bridge_semantic_maps_from_jitcode_pc`'s doc said its callers hold only a JitCode coordinate; the two `RebuiltFrame` callers also hold `py_pc`, which `py_coord.rs` names as a sanctioned Python-coordinate source, so the doc now records that and why it is declined. `resume_marker_jit_pc`'s binding in `compute_nested_inline_caller_frame` no longer names a value and becomes `is_some()`. The kept-stack comment in `setup_bridge_sym`'s caller attributed `stack_depth_at_pc` to `depth_at_py_pc`; it reads the `depth_pred_by_jit_pc` twin. Retracts two claims from 397320f4d6d's message: the `codewriter.rs:14808` citation together with its reading of which table the jitcode-keyed twins carry, and "`in_range_nonzero` — the only bucket reaching `setup_bridge_sym`'s `semantic_prefix_len`". 6b30254e46b's `415/415 and 414/414` was measured on a base that has since moved twice and does not describe this tree. Assisted-by: Claude --- .../src/jitcode_dispatch/resume_snapshot.rs | 15 ++- pyre/pyre-jit-trace/src/state.rs | 119 +++++++++++++----- 2 files changed, 96 insertions(+), 38 deletions(-) diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/resume_snapshot.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/resume_snapshot.rs index af7fcf1c30e..8d4affe35c9 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/resume_snapshot.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/resume_snapshot.rs @@ -2041,7 +2041,7 @@ pub(crate) fn compute_nested_inline_caller_frame( // here (before the nested callee's body walk) — not at capture — places the // stores ahead of every in-callee guard, so `_number_virtuals` reads the // populated array when it numbers those guards' resume data. - if let Some(marker) = resume_marker_jit_pc { + if resume_marker_jit_pc.is_some() { if caller_liveness_word != majit_ir::resumedata::NO_JITCODE_PC && depth > 1 { let (frame_reg, _) = crate::state::portal_red_regs_at(jitcode_index as i32); let frame_red = (frame_reg != u16::MAX) @@ -2058,9 +2058,10 @@ pub(crate) fn compute_nested_inline_caller_frame( // `depth` is the operand-stack depth at the CALL return point, // top slot = the pending nested-call result. let pending_top_slot = nlocals + depth - 1; - // No Python coordinate here: `marker` is - // `inline_call_return_marker`'s `decode_op_at(..).next_pc`, a - // JitCode byte offset. + // No Python coordinate here. This slot used to carry + // `resume_marker_jit_pc`, which `inline_call_return_marker` + // mints as `decode_op_at(..).next_pc` — a JitCode byte offset, + // not a Python PC. It gates the block above and nothing else. let maps = crate::state::bridge_semantic_maps_at_with_jitcode_pc( jitcode_index as i32, None, @@ -2529,10 +2530,8 @@ pub(crate) fn walker_capture_multi_frame_inline_snapshot( let mut recovered_regs_r = ctx.registers_r.to_vec(); let code = unsafe { &*callee_pjc.code_ptr }; let (stack_base, _) = crate::state::callee_layout_for_call_assembler(code); - let maps = crate::state::bridge_semantic_maps_from_jitcode_pc( - callee_jitcode_index, - callee_jitcode_pc, - ); + let maps = + crate::state::bridge_semantic_maps_from_jitcode_pc(callee_jitcode_index, callee_jitcode_pc); // The kept-stack guard gate is certified by this callee frame's // operand-stack mirror. Publish the same boxes into the carried // coordinate's Ref colors before collecting the frame snapshot, so the diff --git a/pyre/pyre-jit-trace/src/state.rs b/pyre/pyre-jit-trace/src/state.rs index 9caa645f47d..d6b7796ed1f 100644 --- a/pyre/pyre-jit-trace/src/state.rs +++ b/pyre/pyre-jit-trace/src/state.rs @@ -1791,23 +1791,35 @@ pub(crate) struct BridgeSemanticMaps { pub pcdep_entries: Vec<(u8, u16, u16)>, } -/// Per-call-site counters for the `via_py_pc` fallback of +/// Counters for the `via_py_pc` fallback of /// [`bridge_semantic_maps_at_with_jitcode_pc`], which indexes the -/// Python-PC-keyed `LiveVars::depth_at_py_pc`. +/// Python-PC-keyed `LiveVars::depth_at_py_pc`. One instance per *leg* that +/// reaches the fallback — the `site=` field of the printed line names the leg, +/// not the caller. /// /// The partition that decides whether a Python coordinate is being read at a /// point the caller did not mean: an offset past the end of the table reads /// `None` and the fallback answers 0, but one that lands inside returns a real -/// depth belonging to a different instruction. `in_range_nonzero` is the only -/// bucket that reaches `setup_bridge_sym`'s `semantic_prefix_len`, so it is the -/// standing check on the one caller that still supplies a `Some` coordinate -/// (`resume_snapshot.rs` `walker_capture_snapshot_for_last_guard_impl`, -/// `liveness_py_pc`); a fire there means that argument is not a Python PC after -/// all. +/// depth belonging to a different instruction. /// -/// The counters are per site, not per process: with one shared counter only the -/// site that fires first is ever named, so a run in which one leg never -/// executed is indistinguishable from one in which it executed second. +/// Which bucket a caller can produce is fixed by the argument it passes, so the +/// two ends must not be welded together: +/// * `setup_bridge_sym` is the one consumer that reads `stack_depth_at_pc` +/// unconditionally (`stack_only.max(..)` → `semantic_prefix_len`), and it +/// passes `None` — so `no_py_pc` is the only bucket reachable from there and +/// `in_range_*` is structurally unreachable. +/// * `in_range_nonzero` can only come from the one caller that supplies a +/// `Some` coordinate, `resume_snapshot.rs` +/// `walker_capture_snapshot_for_last_guard_impl`'s `liveness_py_pc`, whose +/// depth feeds that function's own `semantic_limit`. +/// +/// That caller's coordinate is a Python PC by construction, so a nonzero depth +/// there is the designed result of the leg, not an anomaly — the bucket records +/// which leg ran, and does not by itself witness a coordinate mixup. +/// +/// The counters are per leg, not per process: with one shared counter only the +/// leg that fires first is ever named, so a run in which one leg never executed +/// is indistinguishable from one in which it executed second. /// /// Pure telemetry — with the variable unset every counter path is skipped and /// the fallback's value is unchanged. @@ -1840,14 +1852,28 @@ impl EmptyTwinSite { /// The carried jitcode coordinate decoded, but the twin tables had no entry. static EMPTY_TWIN_MISS: EmptyTwinSite = EmptyTwinSite::new("twin_miss"); -/// No jitcode coordinate was carried at all. +/// No usable jitcode coordinate: either none was carried (`NO_JITCODE_PC`) or +/// the carried offset does not decode as a `-live-` anchored startpoint. static EMPTY_TWIN_NON_DECODABLE: EmptyTwinSite = EmptyTwinSite::new("non_decodable"); +/// `jit_pc` / `twin` report the coordinate this leg declined on and what the +/// jitcode-keyed containing-depth twin would have answered there — the +/// measurement the "route the fallback through the twins instead of answering +/// 0" question needs. On the 399-file synth corpus all 20 executions answer +/// `jit_pc >= 0` with `twin = Some(3..=5)`, so that change is not inert. +/// +/// What this does NOT resolve: the counters are per leg, not per call site, so +/// a fire cannot be attributed to one of the callers. That matters, because +/// `setup_bridge_sym` is the only one whose `stack_depth_at_pc` read is +/// unconditional; at the others an empty `pcdep_entries` makes the depth inert +/// regardless of its value. fn empty_twin_census( site: &EmptyTwinSite, rp: Option, table_len: Option, depth: u16, + jit_pc: i32, + twin: Option, ) { // Shares `PYRE_M73_EMPTYTWIN_CENSUS` with `py_coord::note_empty_twin_fallback`, // so one run reports both. That one prints `[m73-emptytwin]`; this one prints @@ -1858,9 +1884,9 @@ fn empty_twin_census( use std::sync::atomic::Ordering; let name = site.name; let hits = site.hits.fetch_add(1, Ordering::Relaxed) + 1; - // `in_range_nonzero` is the only bucket that can reach `setup_bridge_sym`'s - // `semantic_prefix_len`: a real depth read at a coordinate the caller never - // meant as a Python PC. `no_py_pc` is the leg that used to read one. + // `rp >= len` must be tested before the `depth == 0` arm: `table.get(rp)` + // already collapses an out-of-range read to 0, so the zero arm would + // otherwise swallow it and the two would be indistinguishable. let (bucket, counter, live) = match (rp, table_len) { (None, _) => ("no_py_pc", &site.no_py_pc, false), (Some(_), None) => ("null_code", &site.null_code, false), @@ -1872,9 +1898,12 @@ fn empty_twin_census( // Every bucket announces its first witness, so the buckets that do fire are // the positive control for the ones that do not: a bucket that stays silent // on a stream carrying other buckets' lines is empty, not unprinted. + // `in_range_nonzero` gets 20 rather than one because it is the only bucket + // whose value varies — the rest answer 0 by construction, so one witness + // says everything about them. if n == 1 || (live && n <= 20) { eprintln!( - "[m73-bridge-maps] site={name} bucket={bucket} n={n} rp={rp:?} len={table_len:?} depth={depth}" + "[m73-bridge-maps] site={name} bucket={bucket} n={n} rp={rp:?} len={table_len:?} depth={depth} jit_pc={jit_pc} twin={twin:?}" ); } if hits % 100_000 == 0 { @@ -1926,8 +1955,8 @@ pub(crate) fn bridge_semantic_maps_at_with_jitcode_pc( // usize would otherwise be a huge OOB index. No-op for offsets / // NO_JITCODE_PC (flip-off), so byte-identical when off. let jitcode_pc = crate::jitcode_dispatch::expand_branch_carried(payload, jitcode_pc); - // The rd_numb pc word is already the published resume coordinate. Use - // it directly for the py_pc-keyed liveness/depth table fallback. + // A `Some` py_pc is already the published resume coordinate, so the + // py_pc-keyed liveness/depth table fallback reads it directly. // // When the carried `jitcode_pc` is set (kept-stack branch // guard), resolve the guard's Python PC from the jitcode coordinate @@ -1936,24 +1965,43 @@ pub(crate) fn bridge_semantic_maps_at_with_jitcode_pc( // computed kept operand-stack temps are live; at the merge-target PC // they've been consumed and carry no pcdep entry. // - // A caller holding no Python coordinate answers 0: the static - // liveness table cannot be queried without a Python PC, and the - // JitCode offset is not one. `codewriter.rs:14808` records that this - // table is deliberately NOT the walk-visited `depth_at_pc` the - // jitcode-keyed twins carry, so there is no jitcode-keyed spelling of - // this read to fall back to either. + // A caller holding no Python coordinate answers 0. That is a decline, + // and a lossy one — not the best available answer. + // + // Jitcode-keyed spellings of this same static table exist: + // `depth_containing_for_jitcode_pc` is built (`finalize_jitcode`, the + // `depth_containing_by_jit_pc` block) to reproduce + // `depth_at_py_pc[containing_py]` for every offset, and + // `depth_trivia_for_jitcode_pc` reads it at the trivia-skipped py. + // `collect_outer_active_boxes`, the encode half this mirrors, already + // sources its own `stack_depth_at_pc` from those twins with no Python + // PC in hand. The census below measured what they would answer here: + // over the 399-file synth corpus every one of the 20 executions of this + // leg carried a non-negative offset at which the containing twin + // answers 3, 4 or 5 — so routing through it is a real change in the + // reconstructed frame's width, not a no-op, and it is left to its own + // measurement rather than folded into a coordinate repair. + // + // What this leg must not do, and did before the `Option`, is index the + // Python-keyed table with the JitCode word. let via_py_pc = |rp: Option, site: &'static EmptyTwinSite| -> usize { + // Census-only: what the jitcode-keyed twin would answer at the + // coordinate this leg is declining on. Behind the census gate, so a + // production run does no extra work for it. + let twin = (crate::py_coord::emptytwin_census_enabled() && jitcode_pc >= 0) + .then(|| payload.depth_containing_for_jitcode_pc(jitcode_pc as usize)) + .flatten(); let Some(rp) = rp.and_then(|p| usize::try_from(p).ok()) else { - empty_twin_census(site, None, None, 0); + empty_twin_census(site, None, None, 0, jitcode_pc, twin); return 0; }; if payload.code_ptr.is_null() { - empty_twin_census(site, Some(rp), None, 0); + empty_twin_census(site, Some(rp), None, 0, jitcode_pc, twin); return 0; } let table = crate::liveness::liveness_for(payload.code_ptr).depth_at_py_pc(); let depth = table.get(rp).copied().unwrap_or(0); - empty_twin_census(site, Some(rp), Some(table.len()), depth); + empty_twin_census(site, Some(rp), Some(table.len()), depth, jitcode_pc, twin); depth as usize }; let (stack_depth_at_pc, pcdep_entries) = if jitcode_pc >= 0 @@ -1992,8 +2040,19 @@ pub(crate) fn bridge_semantic_maps_at_with_jitcode_pc( }) } -/// For a caller that holds only a JitCode coordinate. Named for that -/// coordinate so the word cannot be re-routed into the Python-PC slot. +/// For a caller that passes a JitCode coordinate and no Python one. Named for +/// that coordinate so the word cannot be re-routed into the Python-PC slot. +/// +/// Three callers genuinely hold nothing else (`bridge_subwalk.rs` +/// `recipe.jitcode_pc`, `residual_call.rs` `op_pc`, `resume_snapshot.rs` +/// `callee_jitcode_pc`). The two `RebuiltFrame` callers (`state.rs` +/// `reconstruct_inline_recipe` and `setup_bridge_sym`) do hold one — the +/// forward-carried `RebuiltFrame::py_pc`, which `py_coord.rs` names as a +/// sanctioned Python-coordinate source — and decline it anyway: they carried +/// `.pc` here before, and supplying `.py_pc` instead would widen +/// `semantic_prefix_len` off a coordinate the old code never read. Declining is +/// what keeps this a coordinate repair; what the declined leg leaves on the +/// table is recorded at `via_py_pc`. pub(crate) fn bridge_semantic_maps_from_jitcode_pc( jitcode_index: i32, jitcode_pc: i32, @@ -9872,7 +9931,7 @@ impl JitState for PyreJitState { // For a kept-stack branch guard, the vable's // `valuestackdepth` may reflect the merge-target depth (consumed // stack) rather than the guard's deeper live depth. The guard PC's - // pcdep `stack_depth_at_pc` (from `depth_at_py_pc`, resolved through + // pcdep `stack_depth_at_pc` (the `depth_pred_by_jit_pc` twin, keyed by // the carried `jitcode_pc`) IS the guard-time depth. Use the larger // of the two so the color→slot inversion covers the kept temps. // This is deferred until after `maps` is read (below) via a