majit-translate, majit, jit: a payload enum's variant fields seated after the inherited discriminant, PCSEQ at every frame depth, and the portal's can_enter_jit tallies

majit/gate-triage.md

@@ -386,7 +386,7 @@ cover the condition they diagnose.
 
 - Read sites: 1 — `majit/majit-metainterp/src/lib.rs`
 - Accessor: `pcseq_enabled()`
-- What it does: Prints the control-flow decisions a walk of the portal jitcode makes: the interpreter pc every `jit_merge_point` visit carries, and — for the root jitcode frame alone — every `switch`, every `goto_if_not`, and every `loop_header`. A walk that records without ever closing gives the same reading at the top (`seen_loop_header_for_jdindex` stays -1 at every merge point) whether the `loop_header` op was never reached or was reached and declined, and the two call for opposite fixes; the branch lines name which edge diverted the walk.
+- What it does: Prints the control-flow decisions a walk makes: the interpreter pc every `jit_merge_point` visit carries, and every `switch`, `goto_if_not` and `loop_header`, each tagged with the jitcode that owns it and its frame depth (`d=`). A walk that records without ever closing gives the same reading at the top (`seen_loop_header_for_jdindex` stays -1 at every merge point) whether the `loop_header` op was never reached or was reached and declined, and the two call for opposite fixes; the branch lines name which edge diverted the walk, at whatever depth it happened.
 - Retirement condition: **UNRECORDED** — owed by this gate's owner.
 
 ### `MAJIT_PORTAL_INLINE`

majit/majit-metainterp/src/executor.rs

@@ -330,10 +330,10 @@ pub fn execute_varargs<M: Clone>(
                 // `funcbox.2` as a hand-written or `#[jit_module]`-generated
                 // function pointer with i64-return ABI:
                 //   * `do_recursive_call` (`pyjitpl.rs`) sets funcbox.2
-                //     to `targetjitdriver_sd.portal_runner_adr`.  Pyre's
-                //     portal entry is `bh_portal_runner(all_i, all_r, all_f)
-                //     -> i64` (pyre-jit/src/call_jit.rs); it never
-                //     declares an f64 return.
+                //     to `targetjitdriver_sd.portal_runner_adr`, which is
+                //     `bh_portal_runner_c(i64, i64, i64, i64, i64) -> i64`
+                //     (pyre-jit/src/call_jit.rs); it never declares an f64
+                //     return.
                 //   * `#[jit_module]` (majit-macros/src/lib.rs) emits a
                 //     Float helper's `concrete_ptr` as `extern "C" fn(...)
                 //     -> i64` with the f64 result pre-packed via

majit/majit-metainterp/src/lib.rs

@@ -494,18 +494,24 @@ pub fn mptrace_enabled() -> bool {
     *FLAG.get_or_init(|| std::env::var_os("MAJIT_MPTRACE").is_some())
 }
 
-/// Prints the control-flow decisions a walk of the portal jitcode makes: the
-/// interpreter pc every `jit_merge_point` visit carries, and — for the root
-/// jitcode frame alone — every `switch`, every `goto_if_not`, and every
-/// `loop_header`.
+/// Prints the control-flow decisions a walk makes: the interpreter pc every
+/// `jit_merge_point` visit carries, and every `switch`, `goto_if_not` and
+/// `loop_header`, each tagged with the jitcode that owns it and its frame
+/// depth (`d=`).
 ///
 /// A walk that records without ever closing gives the same reading at the top
 /// (`seen_loop_header_for_jdindex` stays -1 at every merge point) whether the
 /// `loop_header` op was never reached or was reached and declined, and the two
 /// call for opposite fixes. The branch lines name which edge diverted the walk,
 /// so the answer is the sequence itself rather than an inference from its
-/// absence. Restricted to the root frame because the ops in question are the
-/// portal's own; an inlined callee's branches would bury them.
+/// absence.
+///
+/// Every depth, not the root frame alone. Restricting it to the root was how
+/// this was first written, and it cost a whole build to learn better: the
+/// portal reported `Continue` on a back edge whose entire callee chain read
+/// correct when decoded statically, and the line that settled it was
+/// `execute_jump_backward`'s own `switch` — three frames down. A reading taken
+/// only at the top says which arm the portal took and nothing about why.
 pub fn pcseq_enabled() -> bool {
     static FLAG: std::sync::OnceLock<bool> = std::sync::OnceLock::new();
     *FLAG.get_or_init(|| std::env::var_os("MAJIT_PCSEQ").is_some())

majit/majit-metainterp/src/pyjitpl/dispatch.rs

@@ -2315,25 +2315,26 @@ where
         );
     }
 
-    /// Print one control-flow edge of the root jitcode frame under
-    /// `MAJIT_PCSEQ` (see `crate::pcseq_enabled`).
+    /// Print one control-flow edge under `MAJIT_PCSEQ`
+    /// (see `crate::pcseq_enabled`).
     ///
     /// `value` is the concrete word the edge was decided on and `target` the
-    /// cursor it moves to, so a run's lines read as the walk's own path
-    /// through the portal.  Silent for an inlined callee: the ops this exists
-    /// to follow are the portal's, and the callees run orders of magnitude
-    /// more of them.
+    /// cursor it moves to, so a run's lines read as the walk's own path.
+    /// `d=` is the frame depth and `jitcode=` the frame that owns the edge:
+    /// an inlined callee prints too, because the edge that diverts a portal
+    /// walk is routinely one of theirs.
     fn pcseq_branch(&mut self, kind: &str, opcode_pc: usize, value: i64, target: Option<usize>) {
-        if !crate::pcseq_enabled() || self.frames.len() != 1 {
+        if !crate::pcseq_enabled() {
             return;
         }
+        let depth = self.frames.len();
         let name = &self.frames.current_mut().jitcode.name;
         match target {
-            Some(t) => {
-                eprintln!("@@@PCSEQ {kind} jitcode={name} pc={opcode_pc} value={value} -> {t}")
-            }
+            Some(t) => eprintln!(
+                "@@@PCSEQ {kind} d={depth} jitcode={name} pc={opcode_pc} value={value} -> {t}"
+            ),
             None => eprintln!(
-                "@@@PCSEQ {kind} jitcode={name} pc={opcode_pc} value={value} -> fallthrough"
+                "@@@PCSEQ {kind} d={depth} jitcode={name} pc={opcode_pc} value={value} -> fallthrough"
             ),
         }
     }

majit/majit-translate/src/front/mir.rs

@@ -1395,6 +1395,19 @@
 /// A fixed 16 would truncate any shell that ever records more than one payload
 /// word.  The floor keeps a tag-only shell at the full `[tag | payload]` width,
 /// matching the `size.max(16)` the codewriter applies to the same shell.
+/// Byte width of a Rust integer spelling, for the enum tag `TypeLayout`
+/// records beside its offset (`discriminant_int_type`).  Anything unspelled
+/// answers a machine word, which is the conservative direction: it widens the
+/// tag's byte range and so decides more enums need the explicit sum shell.
+fn int_type_byte_width(int_ty: &str) -> u64 {
+    match int_ty {
+        "i8" | "u8" => 1,
+        "i16" | "u16" => 2,
+        "i32" | "u32" => 4,
+        _ => 8,
+    }
+}
+
 fn sum_shell_size(field_offsets: &std::collections::HashMap<String, u64>) -> u64 {
     field_offsets
         .values()
@@ -1812,8 +1825,56 @@
                 // base tag stays at offset 0 either way, so the shell only
                 // moves the payload of an Option this front end actually
                 // builds.
-                let explicit_sum_shell =
-                    name == "core::result::Result" || name == "core::option::Option";
+                //
+                // Those two names were the cases this shell was first written
+                // for, but the condition they share is physical rather than
+                // nominal: whenever the host layout seats a variant's payload
+                // field on the tag's own bytes, the constructor's payload store
+                // overwrites the tag it just wrote.  Asking the layout catches
+                // every such enum instead of the two that were noticed.
+                //
+                // `pyre_interpreter::pyopcode::StepResult` — what every opcode
+                // handler returns — is one of them.  Charon gives
+                // `CloseLoop.jump_args` and `Return.__pos_0` offset 0, the tag's
+                // own offset, so `close_loop`'s jitcode stamped discriminant 2
+                // and then overwrote it with `jump_args`; `execute_jump_backward`
+                // read the null back as tag 0 and reported `Continue`, and the
+                // portal took its `Continue` arm on every back edge.  A walk of
+                // the portal could therefore never reach `loop_header`, and no
+                // portal trace could close.
+                // The shell is needed exactly when the base registered a
+                // `__discriminant` row that the variant subclass inherits at
+                // byte 0 (`rclass.py:499-518`): the payload must then start
+                // after it.  `tag_recorded_but_unspellable` registers no base
+                // row at all, so a payload at 0 there aliases nothing.
+                let base_has_discriminant = !fieldless && !tag_recorded_but_unspellable;
+                // An unrecorded tag width reads as a machine word: the wider
+                // guess only moves more enums onto the explicit shell, which
+                // is the representation that cannot alias in the first place.
+                let tag_offset = enum_layout
+                    .as_ref()
+                    .and_then(|l| l.discriminant_offset())
+                    .unwrap_or(0);
+                let tag_size = enum_layout
+                    .as_ref()
+                    .and_then(|l| l.discriminant_int_type())
+                    .map_or(8, int_type_byte_width);
+                let payload_seats_on_tag = match enum_layout.as_ref() {
+                    // Charon recorded no layout for this decl — the case that
+                    // hid this bug.  The variant offsets then fall back to
+                    // default packing from byte 0, which is the inherited
+                    // discriminant's byte, on every payload enum in the set.
+                    None => true,
+                    Some(l) => variants.iter().enumerate().any(|(vidx, v)| {
+                        (0..v.fields.len()).any(|i| {
+                            l.field_offset(vidx, i)
+                                .is_some_and(|off| off >= tag_offset && off < tag_offset + tag_size)
+                        })
+                    }),
+                };
+                let explicit_sum_shell = name == "core::result::Result"
+                    || name == "core::option::Option"
+                    || (base_has_discriminant && payload_seats_on_tag);
                 // Register the enum BASE in `exact_layouts`: a single
                 // `__discriminant` field at the tag's real byte position
                 // (`discriminator.Branch.offset` via `discriminant_offset`).

pyre/pyre-interpreter/src/eval.rs

@@ -3660,7 +3660,7 @@ impl ControlFlowOpcodeHandler for PyFrame {
         // Signal a back-edge to the main eval_loop, which handles
         // JIT counting and compiled code execution via try_back_edge_jit.
         Ok(StepResult::CloseLoop {
-            jump_args: vec![],
+            jump_args: None,
             loop_header_pc: target,
         })
     }

pyre/pyre-interpreter/src/pyopcode.rs

@@ -127,7 +127,14 @@ pub enum StepResult<V> {
     Continue,
     Return(V),
     CloseLoop {
-        jump_args: Vec<V>,
+        /// The arguments [`ControlFlowOpcodeHandler::close_loop_args`]
+        /// supplied, carried as an `Option` so a handler that has none does
+        /// not have to build an empty `Vec` for the field.  The distinction
+        /// costs nothing at the reader — the back edge is the report itself,
+        /// and no consumer reads these — while an owned empty `Vec` costs an
+        /// allocation on every back edge and, in a jitcode, a residual call to
+        /// `Vec::new` that the translator has no binding for.
+        jump_args: Option<Vec<V>>,
         loop_header_pc: usize,
     },
     Yield(V),
@@ -523,7 +530,7 @@ pub trait ControlFlowOpcodeHandler: SharedOpcodeHandler {
     fn close_loop(&mut self, target: usize) -> Result<StepResult<Self::Value>, PyError> {
         match self.close_loop_args(target)? {
             Some(args) => Ok(StepResult::CloseLoop {
-                jump_args: args,
+                jump_args: Some(args),
                 loop_header_pc: target,
             }),
             None => Ok(StepResult::Continue),

pyre/pyre-jit/src/eval.rs

@@ -6268,6 +6268,10 @@
         let green_key = driver.resolve_cell_key(green_key_hash, || {
             pyre_jit_trace::driver::make_green_key_typed(loop_pycode, next_instr, is_being_profiled)
         });
+        // Safe to tally here and nowhere upstream of it: `jtransform` rewrites
+        // the `can_enter_jit` call into the `loop_header` operation, so this
+        // body is never part of a traced graph.  See `PORTAL_DIAG_LABELS`.
+        portal_diag_bump(0);
         if portal_metatrace_enabled()
             && PORTAL_METATRACE_SEEN.fetch_add(1, std::sync::atomic::Ordering::Relaxed)
                 >= portal_metatrace_skip()
@@ -6297,6 +6301,7 @@
             return false;
         };
         set_pending_loop_exit(ec, loop_result);
+        portal_diag_bump(1);
         true
     }
 }
@@ -7154,6 +7159,68 @@
     })
 }
 
+/// Legend for [`PORTAL_DIAG`], positionally.
+///
+/// The portal's own decisions are otherwise unobservable on wasm32: every
+/// existing portal knob (`PYRE_PORTAL_METATRACE`, `MAJIT_PCSEQ`) is a
+/// `std::env::var` read, and the guest has no environment, so those probes are
+/// dead code there rather than silent evidence.  A counter the host reads back
+/// through an export is the shape that carries out of the guest — the same
+/// channel `majit_backend_wasm::BRIDGE_DIAG` and the full-body-walk census
+/// already use.
+///
+/// Every tally is bumped from code the portal graph does **not** contain.
+/// That constraint is not stylistic: `pyre-jit` is one of the four LLBC crates,
+/// so a counter placed in `eval_loop_jit`'s `CloseLoop` arm is lowered into the
+/// portal jitcode, and `AtomicU64::fetch_add` is outside the LLBC set — it
+/// would become a symbolic residual sitting between the arm and its
+/// `loop_header`, which no `pyre-jit` path can ever bind (every production
+/// fnaddr comes from `pyre_interpreter::jit_trace_fnaddrs`, and that table
+/// cannot name a crate above it). It would block the portal walk *and* regress
+/// any production trace that inlines a nested activation. `can_enter_jit`'s
+/// body is safe for the opposite reason: `jtransform` rewrites the call into
+/// the `loop_header` operation, so the body is never traced.
+///
+/// That rules out the sites the arm's own decisions would need. The two
+/// filters sit in the arm itself, and `portal_activation_bracketed` is reached
+/// from `funccall_valuestack` with no `dont_look_inside` or may-force boundary
+/// in between, so none of them is provably outside a traced graph. This line
+/// counts what it can count safely rather than what would read most directly.
+pub const PORTAL_DIAG_LABELS: &[&str] = &[
+    // `can_enter_jit` was reached (warmspot.py:446).  Only `eval_loop_jit`'s
+    // `StepResult::CloseLoop` arm calls it, and only after both of that arm's
+    // filters passed, so a run with back edges and `can_enter_jit=0` says the
+    // arm never completed — which is what a clobbered `StepResult` discriminant
+    // looks like from outside the trace.
+    "can_enter_jit",
+    // …and it answered true: the driver took the trace or ran compiled code.
+    "can_enter_jit_taken",
+];
+
+/// Portal decision tallies, indexed by [`PORTAL_DIAG_LABELS`].
+///
+/// `Relaxed` throughout: these are diagnostics, no other state is ordered
+/// against them, and the back edge they sit on is hot.
+pub static PORTAL_DIAG: [std::sync::atomic::AtomicU64; PORTAL_DIAG_LABELS.len()] =
+    [const { std::sync::atomic::AtomicU64::new(0) }; PORTAL_DIAG_LABELS.len()];
+
+#[inline]
+fn portal_diag_bump(slot: usize) {
+    if let Some(cell) = PORTAL_DIAG.get(slot) {
+        cell.fetch_add(1, std::sync::atomic::Ordering::Relaxed);
+    }
+}
+
+/// One portal tally, by [`PORTAL_DIAG_LABELS`] index; out of range reads 0.
+///
+/// The wasm export and the native `[jit-stats]` line share this reader so the
+/// two backends report the same numbers from the same place.
+pub fn portal_diag(slot: usize) -> u64 {
+    PORTAL_DIAG
+        .get(slot)
+        .map_or(0, |cell| cell.load(std::sync::atomic::Ordering::Relaxed))
+}
+
 fn portal_metatrace_enabled() -> bool {
     static ENABLED: std::sync::OnceLock<bool> = std::sync::OnceLock::new();
     *ENABLED.get_or_init(|| std::env::var("PYRE_PORTAL_METATRACE").as_deref() == Ok("1"))

pyre/pyre-jit/src/jit/call.rs

@@ -438,16 +438,21 @@
     ///     return (fnaddr, calldescr)
     /// ```
     ///
-    /// Note: pyre's blackhole calls every Python
-    /// function through one `bh_portal_runner(frame: ref) -> ref`
-    /// stub — the C-ABI is identical for every CodeObject because the
-    /// portal runner unwraps the frame and dispatches dynamically. So
-    /// `(fnaddr, calldescr)` is constant across all graphs in pyre,
-    /// while RPython has one pair per `FUNC` type because lltype-typed
-    /// `direct_call` ops can carry varying signatures. Keeping the
-    /// method shape preserves the call.py:167
-    /// `(fnaddr, calldescr) = self.get_jitcode_calldescr(graph)` flow
-    /// even though both values are constants here.
+    /// The pair is constant across every graph, where RPython has one per
+    /// `FUNC` type because lltype-typed `direct_call` ops carry varying
+    /// signatures. Keeping the method shape preserves the call.py:167
+    /// `(fnaddr, calldescr) = self.get_jitcode_calldescr(graph)` flow even so.
+    ///
+    /// Nothing dispatches the address it returns. `bhimpl_recursive_call_*`
+    /// reaches the portal through `get_portal_runner`, which answers
+    /// `bh_portal_runner_c` with the `"iirrr"` descr that really describes it;
+    /// and where upstream's `bhimpl_inline_call_*` calls
+    /// `cpu.bh_call_*(jitcode.fnaddr, …)` (blackhole.py:1279-1320), pyre reads
+    /// its callee out of a build-time descr pool whose `fnaddr` comes from
+    /// `JitCodeBuilder::set_native_entry`, and a CodeObject jitcode minted here
+    /// is never in that pool. So this stamps a non-null placeholder, and the
+    /// descr beside it describes `bh_portal_runner_c`, not the slice-taking
+    /// `bh_portal_runner` whose address it takes.
     pub fn get_jitcode_calldescr(&self, _graph: *const CodeObject) -> (i64, BhCallDescr) {
         let fnaddr = crate::call_jit::bh_portal_runner as *const () as usize as i64;
         let calldescr = BhCallDescr::from_arg_classes(

pyre/pyre-wasm-runner/src/main.rs

@@ -911,6 +911,27 @@ fn run(module_path: &Path, source: &str, script: &Path) -> Result<i32> {
             }
             eprintln!("[jit-stats] bridge_diag {}", parts.join(" "));
         }
+        // The portal's own decision tallies. POSITIONAL MIRROR of
+        // `pyre_jit::eval::PORTAL_DIAG_LABELS`, but unlike `bridge_diag` above
+        // the guest also exports the slot count, so a slot added there without
+        // a label here is reported as `slot<N>` rather than silently dropped.
+        if let Ok(diag) = instance.get_typed_func::<u32, u64>(&mut store, "pyre_jit_portal_diag") {
+            let labels = ["can_enter_jit", "can_enter_jit_taken"];
+            let guest_len = instance
+                .get_typed_func::<(), u32>(&mut store, "pyre_jit_portal_diag_len")
+                .ok()
+                .and_then(|f| f.call(&mut store, ()).ok())
+                .unwrap_or(labels.len() as u32) as usize;
+            let mut parts = Vec::new();
+            for i in 0..guest_len.max(labels.len()) {
+                let n = diag.call(&mut store, i as u32).unwrap_or(0);
+                match labels.get(i) {
+                    Some(lbl) => parts.push(format!("{lbl}={n}")),
+                    None => parts.push(format!("slot{i}={n}")),
+                }
+            }
+            eprintln!("[jit-stats] portal_diag {}", parts.join(" "));
+        }
         if let Ok(geometry) =
             instance.get_typed_func::<u32, u64>(&mut store, "pyre_jit_inline_geometry_diag")
         {