From a4e22243de293db4843a43c92461e49f9789c3cf Mon Sep 17 00:00:00 2001 From: technomancer Date: Wed, 2 Sep 2026 22:20:07 -0700 Subject: [PATCH 1/3] switch to use flat map array instead of hash map for physical code page access --- profile.sh | 2 +- ...absolute_pc_exit-in_delay_slot-followup.md | 6 + rules/jitv2/jit-v2-design.md | 10 + src/jitv2/codegen.rs | 21 +- src/jitv2/jitv2.rs | 242 +++++++++++++++--- src/jitv2/mod.rs | 14 +- src/mips_exec.rs | 99 +++++++ 7 files changed, 358 insertions(+), 36 deletions(-) diff --git a/profile.sh b/profile.sh index 04c81c6c..19313bf5 100755 --- a/profile.sh +++ b/profile.sh @@ -1,2 +1,2 @@ #!/bin/bash -PERFFLAGS="-F 200 -g --call-graph dwarf" cargo flamegraph --profile profiling --features rex-jit,lightning --bin iris \ No newline at end of file +PERFFLAGS="-F 200 -g --call-graph dwarf" cargo flamegraph --profile profiling --features rex-jit,lightning,j2wp,tcache --bin iris \ No newline at end of file diff --git a/rules/jitv2/emit_absolute_pc_exit-in_delay_slot-followup.md b/rules/jitv2/emit_absolute_pc_exit-in_delay_slot-followup.md index f3f70369..2597a264 100644 --- a/rules/jitv2/emit_absolute_pc_exit-in_delay_slot-followup.md +++ b/rules/jitv2/emit_absolute_pc_exit-in_delay_slot-followup.md @@ -11,6 +11,12 @@ because something upstream already cleared the flag first: `emit_branch_taken_edge`/`emit_nested_branch_slot`) runs after `emit_slot_semantics`'s non-terminating tail, which unconditionally clears the flag and restores `saved_pc` before returning. + **(2026-09-01)** An attempt to cfg-gate this clear (and the whole + `in_delay_slot=1`/`pc` save/restore bracket) down to + `jitv2_lockstep`/`developer` was made and **reverted** — `deliver_exception` + reads both fields out of memory, so they are load-bearing on every config. + See [[inlined-slot-pc-bd-bracket-is-dead]]. This bullet's guarantee is + intact and unconditional, exactly as originally written. - The annulling-Likely not-taken arm never sets the flag in the first place (the slot is skipped entirely, mirroring `handle_branch_likely_skip`). diff --git a/rules/jitv2/jit-v2-design.md b/rules/jitv2/jit-v2-design.md index 8084a821..1ebdcdd4 100644 --- a/rules/jitv2/jit-v2-design.md +++ b/rules/jitv2/jit-v2-design.md @@ -150,6 +150,10 @@ Under decline-and-defer, every stub does one thing: write the interpreter's nati - `vPC = vbase + static_offset` at every point; the architectural `cpu.pc` field is written **only** in stubs and helpers (the only places it is observable). No per-instruction PC store on the fast path. +> **(as-built, 2026-09-01)** For in-region *addressing* only **bits 12..63** of `core.pc` are consumed: every in-region address is `emit_vbase` (`pc & !0xFFF`) plus a *compile-time* word offset — `emit_word_addr`, `emit_write_link_register`, `emit_jump_target_addr` take the word as an argument; `emit_exit_block_body` takes it as a **block parameter**; `emit_bail` passes an `iconst`. Every store that moves `core.pc` off-page (`emit_absolute_pc_exit`, `emit_runtime_pc_exit`, `emit_foreign_page_slot_exit`, the Likely-skip arms) is immediately followed by `return_`, so no `emit_vbase` can observe one. Memory callouts don't read `core.pc` at all — `jit_read*`/`jit_write*` take the VA as an explicit argument. +> +> **This does NOT make the low bits dead.** `deliver_exception` (mips_core.rs) computes `cp0_epc` from live `core.pc` — the exact word, not the page — and the JIT reaches it via `emit_exception_call_block_body`, which passes only `(core_ptr, status)`. So `core.pc` must be the *faulting instruction's own* address whenever an exception can be raised, which includes inside an inlined delay slot. A 2026-09-01 attempt to drop `emit_slot_semantics`' per-slot pc save/restore on the "low bits are dead" argument was reverted after six `equiv_test` delay-slot exception failures. See `rules/jitv2/inlined-slot-pc-bd-bracket-is-dead.md`. + ### 3.5 Region exits Exit to dispatcher on: `jr`/`jalr` (every return), page-leaving `j`/`jal`/branches, excluded instructions, event-counter fire, exceptions. Exit hands the dispatcher a resolved `(pfn, offset)` where statically computable (KSEG0 targets free; mapped targets need the TLB probe in the stub). @@ -182,6 +186,12 @@ Both arms that need "run the interpreter for real" (FR-mismatch's fallback, and - 0xFFC rule (§2.3). - BD stub variants (§3.3). + +> **(as-built, 2026-09-01) An inlined slot MUST maintain live `core.pc`/`in_delay_slot`, and this is the one real exception to §3.4's "no per-instruction PC store on the fast path".** `emit_slot_semantics` brackets every inlined slot with `in_delay_slot = 1` / slot-address into `core.pc`, restoring both afterward. It is tempting to call this dead — an in-region branch edge is a plain `jump` (`emit_target_edge`) writing neither field, and `exception_other_word_block` stores both itself — but `deliver_exception` reads **both fields out of memory** to decide `Cause.BD` and `cp0_epc = pc - 4`, and `emit_exception_call_block_body` passes it only `(core_ptr, status)`. `ctx.bd` selects which exception stage block runs; it is not an argument to the callee. Dropping either store yields `BD=0, EPC=pc` where the interpreter yields `BD=1, EPC=pc-4`. +> +> Removing this traffic (~29% of all emitted stores) would require changing the JIT→Rust exception ABI to pass BD and the faulting word as arguments. See `rules/jitv2/inlined-slot-pc-bd-bracket-is-dead.md`. +> +> `in_delay_slot` is additionally read by the entry word's foreign-slot check (§6.1.4) and `jitv2_lockstep`'s compare. - Branch-likely: annul semantics compiled explicitly; annulled slot still charges its interpreter-equivalent cycles. `[Q4.1]` confirm interpreter's cycle charge for annulled slots and mirror it. ### 4.4 Excluded instructions (interpreter-only, end region) diff --git a/src/jitv2/codegen.rs b/src/jitv2/codegen.rs index 67c1abe0..0362ea4e 100644 --- a/src/jitv2/codegen.rs +++ b/src/jitv2/codegen.rs @@ -6046,7 +6046,26 @@ fn emit_slot_semantics(ctx: &mut EmitCtx, instrs: &[CompiledInstr; ENTRIES_PER_P // an exception, control never returns here (emit_exception_exit is a // block terminator), so there's nothing to restore on that path: the // slot's `core.pc` write is exactly what deliver_exception needs to see - // in that case. The slot's address itself is derived from this same + // in that case. + // + // That last sentence is the load-bearing one, and it is easy to miss: + // this store is NOT removable. `deliver_exception` (mips_core.rs) reads + // live `core.pc` to compute `cp0_epc = pc - 4` for a delay-slot fault, + // and the JIT reaches it through `emit_exception_call_block_body`, which + // passes only `(core_ptr, status)` — the faulting word is not an + // argument. Nor is `Cause.BD`: the `in_delay_slot = 1` store above is + // read out of memory by the same function. `ctx.bd` only selects which + // exception *stage block* runs, not what the callee sees. + // + // The "only bits 12..63 of core.pc matter in-region" argument (true for + // addressing — every in-region address is emit_vbase + a compile-time + // word) does NOT apply here: EPC needs the exact word. A 2026-09-01 + // attempt to cfg-gate this whole bracket down to + // jitv2_lockstep/developer was reverted after six equiv_test delay-slot + // exception failures (Cause differing by exactly bit 31). cpu-tests and + // a full IRIX boot both passed the broken build — equiv_test is the only + // suite that covers this. See + // rules/jitv2/inlined-slot-pc-bd-bracket-is-dead.md. The slot's address itself is derived from this same // live pc load (emit_word_addr's vbase, §2.2 position independence) — // never from compile-time page_base, which is a physical address in // production (`comp.rs`'s `phys_base`) and would be wrong to bake into diff --git a/src/jitv2/jitv2.rs b/src/jitv2/jitv2.rs index c0babaf8..72b1c109 100644 --- a/src/jitv2/jitv2.rs +++ b/src/jitv2/jitv2.rs @@ -17,7 +17,6 @@ //! a mutable pointer. Only the compile-request queue itself is added in this pass //! — the compile thread and publish path land with codegen (Phase 2). -use std::collections::HashMap; use std::sync::atomic::{AtomicBool, AtomicU32, AtomicU64, AtomicU8, Ordering}; use std::sync::{Arc, Weak}; use std::thread::JoinHandle; @@ -84,6 +83,92 @@ unsafe impl Send for CompileRequest {} // module actually uses them. // ============================================================================ +/// Number of 4 KiB physical frames in the 32-bit physical address space the +/// guest can address (`phys_addr: u32` everywhere on the fetch path) — the +/// exact entry count of `PfnMap` below. +pub const PFN_MAP_ENTRIES: usize = (u32::MAX as usize / PAGE_SIZE as usize) + 1; + +/// Direct-mapped `pfn -> PageSlot` lookup, replacing the `HashMap` both page pools used to carry. +/// +/// One `u32` per physical frame: 1 Mi entries x 4 B = 4 MiB, allocated once +/// and never resized. `PFN_MAP_EMPTY` means "no pool slot for this frame". +/// +/// Why not a hash map: `page_for` is on the page-crossing path (see +/// `jitv2_track_pcp`), and a profile of a real IRIX boot put it at ~4.5% of +/// total runtime with `alloc::alloc::Global`/`RawTable` frames directly above +/// it — the map's own incremental rehash-and-grow, since the pool claims a +/// fresh pfn on every page switch it hasn't seen. A direct index has no hash, +/// no probe sequence, no allocation ever, and no `pages[slot]` bounds-check +/// dance beyond the one the caller already does. +/// +/// Boxed rather than inline so `Jitv2` itself stays small enough to move +/// cheaply (it is constructed into an `Arc>`). +pub struct PfnMap(Box<[u32]>); + +/// `PfnMap`'s own "no slot here" sentinel. Numerically identical to each +/// impl module's private `NO_SLOT` (both `u32::MAX`), spelled separately +/// because `PfnMap` is shared code that predates either module in this file +/// and cannot name a `mod`-private constant. +pub const PFN_MAP_EMPTY: u32 = u32::MAX; + +impl PfnMap { + /// One allocation, every frame empty. + pub fn new() -> Self { + PfnMap(vec![PFN_MAP_EMPTY; PFN_MAP_ENTRIES].into_boxed_slice()) + } + + /// The slot for `pfn`, or `None` when unmapped. + #[inline(always)] + pub fn get(&self, pfn: u32) -> Option { + let slot = self.0[pfn as usize]; + if slot == PFN_MAP_EMPTY { None } else { Some(slot) } + } + + #[inline(always)] + pub fn insert(&mut self, pfn: u32, slot: u32) { + self.0[pfn as usize] = slot; + } + + #[inline(always)] + pub fn remove(&mut self, pfn: u32) { + self.0[pfn as usize] = PFN_MAP_EMPTY; + } + + /// Drop every mapping (`mega_flush`). A 4 MiB memset — flushes are rare + /// and already far more expensive than this, so no generation-tag + /// indirection is worth paying for on every lookup to avoid it. + pub fn clear(&mut self) { + self.0.fill(PFN_MAP_EMPTY); + } +} + +impl PfnMap { + /// Raw pointer to the backing array, for the CPU thread's lock-free + /// lookup fast path (`MipsExecutor::jitv2_pfn_map`). + /// + /// Valid for the process's whole lifetime: the array is allocated once in + /// `new()` and never resized — `mega_flush` clears it in place + /// (`PfnMap::clear` is a `fill`, not a reallocation), exactly like the + /// `pages` pool array it indexes into. + /// + /// # Safety (for the caller) + /// Reading through this pointer without holding `Mutex` is sound + /// only because the page pool is *CPU-thread-only* state: `page_for` is + /// called from the CPU thread alone, and the one cross-thread writer — + /// a compile worker's `flush_from_jit_thread` — runs only after + /// `cpu.stop()`, which joins the CPU thread outright + /// (`MipsCpu::stop`). So no reader is live while a writer runs. + #[inline] + pub fn as_ptr(&self) -> *const u32 { + self.0.as_ptr() + } +} + +impl Default for PfnMap { + fn default() -> Self { Self::new() } +} + /// Upfront reservation for the shared `Codegen`'s Cranelift `ArenaMemoryProvider` /// (`Codegen::new_module`'s own doc comment for why this exists at all). A /// `PROT_NONE` virtual-address-space reservation (cheap — nothing is actually @@ -1230,10 +1315,11 @@ pub type PageSlot = u32; /// pre-existing slot in place avoids that entirely: nothing is ever copied /// after `Jitv2::new` builds the array. /// -/// Lookup from `pfn` to pool slot goes through `pfn_to_slot`. This is a -/// HashMap for now — simplest thing that works. If page-switch lookup shows -/// up hot in profiling, the design doc's dense pfn-indexed alternative -/// (§2.4) is the fallback; not built preemptively. +/// Lookup from `pfn` to pool slot goes through `pfn_to_slot` — the design +/// doc's dense pfn-indexed alternative (§2.4), now built: page-switch lookup +/// *did* show up hot in profiling (~4.5% of a real IRIX boot, with the +/// HashMap's own `RawTable` grow-and-rehash allocation directly above it), +/// which is exactly the trigger this comment used to name. See [`PfnMap`]. pub struct Jitv2 { /// The full-capacity page pool, allocated once — see this struct's own /// doc comment. Indices are stable for the pool's entire lifetime, @@ -1258,8 +1344,8 @@ pub struct Jitv2 { next_free: usize, /// pfn -> index into `pages`. Consulted only on a page switch (fetch /// lands on a different PFN than the currently-tracked one) — not on - /// every fetch. - pfn_to_slot: HashMap, + /// every fetch. Direct-mapped, not hashed — see [`PfnMap`]. + pfn_to_slot: PfnMap, /// Pool capacity, fixed at construction (== `pages.len()`). Claiming past /// this triggers `mega_flush` (the "ran out of PCPs" resource-exhaustion /// trigger). @@ -1335,7 +1421,7 @@ impl Jitv2 { Self { pages: (0..capacity).map(|_| PhysicalCodePage::new(0, std::ptr::null())).collect(), next_free: 0, - pfn_to_slot: HashMap::new(), + pfn_to_slot: PfnMap::new(), capacity, compile_queue: CompileQueue::new(), codegen: Mutex::new(Some(crate::jitv2::codegen::Codegen::new())), @@ -1345,6 +1431,24 @@ impl Jitv2 { } } + /// Raw pointers backing the CPU thread's lock-free `page_for` fast path: + /// `(pfn_map, pages)`. Both arrays are allocated once and never resized + /// (see `PfnMap::as_ptr` and the `pages` field's own doc comment), so + /// both stay valid for the process's whole lifetime, across `mega_flush` + /// included. + /// + /// Captured once by the executor (`MipsExecutor::jitv2_bind_fast_lookup`) + /// and used to resolve `pfn -> *mut PhysicalCodePage` with no + /// `Mutex` acquisition at all on a hit. A miss still goes through + /// the lock, since claiming a slot mutates the free list/MRU list. + /// + /// # Safety (for the caller) + /// See `PfnMap::as_ptr` — sound only because the pool is CPU-thread-only + /// state and the sole cross-thread writer runs with the CPU thread + /// joined. + pub fn fast_lookup_ptrs(&mut self) -> (*const u32, *mut PhysicalCodePage) { + (self.pfn_to_slot.as_ptr(), self.pages.as_mut_ptr()) + } /// Look up the pool slot for `pfn`, claiming the next unclaimed slot /// in place (`PhysicalCodePage::claim`, `gen_ptr(phys_addr)` on the bus) /// if this is the first arrival at this page. Returns `None` if the pool @@ -1356,7 +1460,7 @@ impl Jitv2 { /// rather than reconstructed here because callers already have it from /// translation and multiplying back out is wasted work on the hot path. pub fn page_for(&mut self, pfn: Pfn, phys_addr: u32, bus: &dyn BusDevice) -> Option { - if let Some(&slot) = self.pfn_to_slot.get(&pfn) { + if let Some(slot) = self.pfn_to_slot.get(pfn) { return Some(slot); } if self.next_free >= self.capacity { @@ -4836,6 +4940,11 @@ pub type PageSlot = u32; /// `pages.len()` can never reach `u32::MAX` in practice (would be 16TB+ of /// `PhysicalCodePage`s), so this can't collide with a real slot index. const NO_SLOT: u32 = u32::MAX; +/// `PfnMap` is shared code and spells its own sentinel (`PFN_MAP_EMPTY`) +/// because it cannot name this mod-private constant. They must stay the +/// same value: `page_for` compares slots that came out of `PfnMap::get` +/// against this module's `NO_SLOT` in the free-list/MRU paths. +const _: () = assert!(NO_SLOT == super::PFN_MAP_EMPTY); /// JIT v2 engine state embedded in the mips executor. /// @@ -4859,10 +4968,11 @@ const NO_SLOT: u32 = u32::MAX; /// least-recently-used list (`prev`/`next` again) that `mega_flush` walks /// from the front to decide which slots survive. /// -/// Lookup from `pfn` to pool slot goes through `pfn_to_slot`. This is a -/// HashMap for now — simplest thing that works. If page-switch lookup shows -/// up hot in profiling, the design doc's dense pfn-indexed alternative -/// (§2.4) is the fallback; not built preemptively. +/// Lookup from `pfn` to pool slot goes through `pfn_to_slot` — the design +/// doc's dense pfn-indexed alternative (§2.4), now built: page-switch lookup +/// *did* show up hot in profiling (~4.5% of a real IRIX boot, with the +/// HashMap's own `RawTable` grow-and-rehash allocation directly above it), +/// which is exactly the trigger this comment used to name. See [`PfnMap`]. pub struct Jitv2 { /// The full-capacity page pool, allocated once — see this struct's own /// doc comment. Indices are stable for the pool's entire lifetime, @@ -4903,8 +5013,8 @@ pub struct Jitv2 { free_head: u32, /// pfn -> index into `pages`. Consulted only on a page switch (fetch /// lands on a different PFN than the currently-tracked one) — not on - /// every fetch. - pfn_to_slot: HashMap, + /// every fetch. Direct-mapped, not hashed — see [`PfnMap`]. + pfn_to_slot: PfnMap, /// Pool capacity, fixed at construction (== `pages.len()`). Claiming past /// this (free list AND MRU-preserved-but-recompiling slots both /// exhausted) triggers `mega_flush` (the "ran out of PCPs" @@ -4993,7 +5103,7 @@ impl Jitv2 { mru_head: NO_SLOT, mru_tail: NO_SLOT, free_head: if capacity > 0 { 0 } else { NO_SLOT }, - pfn_to_slot: HashMap::new(), + pfn_to_slot: PfnMap::new(), capacity, compile_queue: CompileQueue::new(), codegen: Mutex::new(Some(crate::jitv2::codegen::Codegen::new())), @@ -5029,6 +5139,55 @@ impl Jitv2 { if self.mru_tail == NO_SLOT { self.mru_tail = slot; } } + /// Release `slot` back to the free list, undoing everything `page_for`'s + /// claim path set up. The single owner of that teardown sequence — every + /// step has to happen, in this order, and getting any of them wrong has + /// already cost this file real bugs: + /// + /// 1. **`mru_unlink` first**, while the slot's `prev`/`next` still hold + /// real MRU links. Doing it after step 3 would unlink against + /// already-cleared pointers and corrupt the list. + /// 2. **Drop the pfn mapping**, keyed off the slot's *own* `pfn` — read + /// it before `reset_to_unclaimed` zeroes it. A slot freed while still + /// mapped is exactly the "map and the slot it points at have desynced" + /// case `page_for`'s own `debug_assert` exists to catch. + /// 3. **`reset_to_unclaimed`**, clearing `pfn`/`gen` and every entry's + /// published state, so a later `claim` starts from a clean slot (its + /// own `debug_assert` enforces this). + /// 4. **Push onto the free list**, `prev` explicitly `NO_SLOT`. The free + /// list threads through `next` only, but `prev` must not be left + /// holding a stale MRU link: `touch_mru`'s "am I already linked?" + /// guard tests both, and free-list garbage in `prev` made it call + /// `mru_unlink` on a slot that was never in the MRU list — confirmed + /// live as a boot hang (a cycle in the list that `mega_flush`'s walk + /// then spun on forever). See `page_for`'s own comment on the same trap. + fn free_page(&mut self, slot: PageSlot) { + self.mru_unlink(slot); + let pfn = self.pages[slot as usize].pfn; + self.pfn_to_slot.remove(pfn); + self.pages[slot as usize].reset_to_unclaimed(); + self.pages[slot as usize].prev = NO_SLOT; + self.pages[slot as usize].next = self.free_head; + self.free_head = slot; + } + /// Raw pointers backing the CPU thread's lock-free `page_for` fast path: + /// `(pfn_map, pages)`. Both arrays are allocated once and never resized + /// (see `PfnMap::as_ptr` and the `pages` field's own doc comment), so + /// both stay valid for the process's whole lifetime, across `mega_flush` + /// included. + /// + /// Captured once by the executor (`MipsExecutor::jitv2_bind_fast_lookup`) + /// and used to resolve `pfn -> *mut PhysicalCodePage` with no + /// `Mutex` acquisition at all on a hit. A miss still goes through + /// the lock, since claiming a slot mutates the free list/MRU list. + /// + /// # Safety (for the caller) + /// See `PfnMap::as_ptr` — sound only because the pool is CPU-thread-only + /// state and the sole cross-thread writer runs with the CPU thread + /// joined. + pub fn fast_lookup_ptrs(&mut self) -> (*const u32, *mut PhysicalCodePage) { + (self.pfn_to_slot.as_ptr(), self.pages.as_mut_ptr()) + } /// Look up the pool slot for `pfn`, claiming the next unclaimed slot /// in place (`PhysicalCodePage::claim`, `gen_ptr(phys_addr)` on the bus) /// if this is the first arrival at this page. Returns `None` if the pool @@ -5045,7 +5204,7 @@ impl Jitv2 { /// `PhysicalCodePage::fr1`'s own doc comment for why it can't be /// re-decided per lookup). pub fn page_for(&mut self, pfn: Pfn, phys_addr: u32, bus: &dyn BusDevice, fr1: bool) -> Option { - if let Some(&slot) = self.pfn_to_slot.get(&pfn) { + if let Some(slot) = self.pfn_to_slot.get(pfn) { debug_assert_eq!(self.pages[slot as usize].pfn, pfn, "pfn_to_slot[{:#x}] -> slot {} whose own pfn is {:#x} — the map and the slot it points at have \ desynced (a slot was reused/evicted without this map entry being updated to match)", @@ -5092,9 +5251,21 @@ impl Jitv2 { /// Number of pool slots currently claimed (since construction or the /// last `mega_flush`). Exit-time diagnostic — see `MipsCpu::stop`. + /// + /// Counts the MRU list rather than the pfn map: [`PfnMap`] is a + /// direct-mapped array with no live-entry count of its own, and the MRU + /// list holds exactly the claimed set (`page_for` calls `touch_mru` on + /// both a lookup hit and a fresh claim; `free_page` unlinks). O(claimed), + /// and this is a diagnostic, not a hot path. #[inline] pub fn pages_used(&self) -> usize { - self.pfn_to_slot.len() + let mut n = 0usize; + let mut slot = self.mru_head; + while slot != NO_SLOT { + n += 1; + slot = self.pages[slot as usize].next; + } + n } /// Pool capacity, as passed to `new()`. @@ -5162,17 +5333,27 @@ impl Jitv2 { /// need the raw per-page detail instead of a summary statistic. No /// longer a contiguous `pages[..next_free]` slice — claimed slots can be /// scattered anywhere in `pages` now that `mega_flush` preserves some and - /// frees others non-contiguously — so this goes through `pfn_to_slot` - /// instead (same set of slots `code_bytes_used`/`code_size_by_instr_count` + /// frees others non-contiguously — so this walks the **MRU list** + /// instead, which holds exactly the claimed set (`page_for` calls + /// `touch_mru` on both a lookup hit and a fresh claim; `free_page` + /// unlinks). Same set of slots `code_bytes_used`/`code_size_by_instr_count` /// reach a different way, by scanning the whole array and filtering on - /// `func().is_null()` — either is a valid definition of "claimed" since a - /// slot is in `pfn_to_slot` iff it isn't sitting on the free list, and - /// filtering by `pfn_to_slot` here avoids visiting the potentially-large - /// free portion of the array at all). `pages` itself stays private - /// (index stability, see the field's own doc comment, is an invariant - /// only this module should rely on). + /// `func().is_null()` — either is a valid definition of "claimed", since + /// a slot is MRU-linked iff it isn't sitting on the free list. `pages` + /// itself stays private (index stability, see the field's own doc + /// comment, is an invariant only this module should rely on). pub fn claimed_pages(&self) -> impl Iterator { - self.pfn_to_slot.values().map(move |&slot| &self.pages[slot as usize]) + // Walks the MRU list (exactly the claimed set — see `pages_used`) + // rather than the pfn map: [`PfnMap`] is a 1 Mi-entry direct-mapped + // array, so iterating *it* would visit a million mostly-empty slots + // to find at most `capacity` (4096) real ones. + let mut slot = self.mru_head; + std::iter::from_fn(move || { + if slot == NO_SLOT { return None; } + let cur = slot as usize; + slot = self.pages[cur].next; + Some(&self.pages[cur]) + }) } /// Reset the compiled-code arena to empty while preserving the @@ -5253,12 +5434,7 @@ impl Jitv2 { // codegen decisions, not the new one's. requests.push(CompileRequest { page: page as *mut PhysicalCodePage, compiled_for_fr1: page.is_fr1() }); } else { - self.mru_unlink(slot); - self.pfn_to_slot.remove(&self.pages[slot as usize].pfn); - self.pages[slot as usize].reset_to_unclaimed(); - self.pages[slot as usize].prev = NO_SLOT; - self.pages[slot as usize].next = self.free_head; - self.free_head = slot; + self.free_page(slot); } rank += 1; slot = next; diff --git a/src/jitv2/mod.rs b/src/jitv2/mod.rs index 7d2752e8..e555d74b 100644 --- a/src/jitv2/mod.rs +++ b/src/jitv2/mod.rs @@ -106,7 +106,19 @@ mod zz_corpus { // callout-only code — invisible to any change in the inline path. cg.dc_geometry = geom; let f: Option = cg.compile_region(&mut ins, off, true, false); - if f.is_some() { total += cg.last_code_size() as u64; n_ok += 1; } + // `last_code_size` is `developer`-gated, but this test must be + // runnable WITHOUT `developer`: that feature also flips + // `opt_level` to `none` and injects a per-instruction + // `emit_dev_trace_bp` callout, so a developer build measures + // code that production never emits (71% of all callouts in one + // measurement were the trace hook alone). Report 0 bytes there + // rather than refusing to build — the ok/declined counts and + // `IRIS_JIT_DISASM=1` output are still the useful part. + #[cfg(feature = "developer")] + let sz = cg.last_code_size() as u64; + #[cfg(not(feature = "developer"))] + let sz = 0u64; + if f.is_some() { total += sz; n_ok += 1; } else { n_decl += 1; } std::mem::forget(cg); } diff --git a/src/mips_exec.rs b/src/mips_exec.rs index 5e71866c..a060541d 100644 --- a/src/mips_exec.rs +++ b/src/mips_exec.rs @@ -1143,6 +1143,26 @@ pub struct MipsExecutor { /// `jitv2_compile_queue`/`jitv2_stats` below. #[cfg(feature = "jitv2")] pub jitv2: std::sync::Arc>, + /// Lock-free `pfn -> PageSlot` lookup: a raw pointer to the `PfnMap` + /// array inside `jitv2`, captured once by `jitv2_bind_fast_lookup`. + /// + /// `page_for` is CPU-thread-only, and its hit path is a pure read — but + /// it used to pay a `jitv2.lock()` anyway, because the pool happens to + /// live inside the same `Mutex` as genuinely-shared state + /// (`codegen`, `compile_queue`, …). Every one of those shared fields + /// already carries its *own* synchronization, so the outer mutex was + /// protecting only the pool — from a thread that never touches it except + /// during a flush, with the CPU thread joined. + /// + /// Null until bound; a null pointer just means "take the lock", so this + /// degrades safely for the (many) test paths that never call the binder. + #[cfg(feature = "jitv2")] + jitv2_pfn_map: *const u32, + /// Companion to `jitv2_pfn_map`: the pool's `pages` array base, so a + /// lookup hit can produce the `*mut PhysicalCodePage` the caller wants + /// without going back through `Jitv2`. Same lifetime/safety story. + #[cfg(feature = "jitv2")] + jitv2_pages_base: *mut crate::jitv2::PhysicalCodePage, /// Cheap handle to `jitv2.lock().compile_queue`'s underlying push queue, /// cloned once at construction (`CompileQueue::queue_handle`) — lets the /// per-dispatch compile-request send (`exec_decoded`'s JIT gate) skip @@ -2466,6 +2486,12 @@ impl MipsExecutor { // self-contained jitv2 pool with no special-casing. #[cfg(feature = "jitv2")] jitv2: std::sync::Arc::new(Mutex::new(jitv2)), + // Bound by `jitv2_bind_fast_lookup` — null means "always take + // the lock", which is correct, just slower. + #[cfg(feature = "jitv2")] + jitv2_pfn_map: std::ptr::null(), + #[cfg(feature = "jitv2")] + jitv2_pages_base: std::ptr::null_mut(), #[cfg(feature = "jitv2")] jitv2_compile_queue_handle, #[cfg(feature = "jitv2")] @@ -2528,6 +2554,16 @@ impl MipsExecutor { pub fn rebind_atomic_ptrs(&mut self) { #[cfg(feature = "idle-pause")] { self.idle_profile_on_ptr = Arc::as_ptr(&self.idle_profile_on); } + // Same "re-sync raw pointers after Arc injection" job for the page + // pool's lock-free lookup arrays — `Machine::new` replaces this + // executor's standalone default `Arc>` with the shared + // one, which has its own (differently-allocated) pool, so the + // pointers captured at construction would otherwise still address + // the abandoned default pool. Binding here covers both: construction + // (below `MipsExecutor::new`'s own struct literal) and every + // post-injection re-sync. + #[cfg(feature = "jitv2")] + self.jitv2_bind_fast_lookup(); } /// ppmem: pointer to this executor's inline `ppmem_bitmap` word, for @@ -3051,6 +3087,47 @@ va={:#018x} phys={:#010x} (code pfn {:#x}, page {:#010x}, word {}/{})", /// nanotlb hit path above and a single PFN comparison here. #[cfg(feature = "jitv2")] #[inline(always)] + /// Capture the page pool's array base pointers for the lock-free + /// `page_for` fast path (`jitv2_pfn_map`/`jitv2_pages_base`). Call once, + /// after the executor's final `Arc>` is in place — in + /// production that's `Machine::new`, right where the other jitv2 handles + /// are injected; `MipsExecutor::new`'s own standalone default pool binds + /// itself so direct-construction callers (equiv_test's ~30 sites) get the + /// fast path too. + /// + /// Idempotent and safe to skip: leaving the pointers null just routes + /// every lookup through `jitv2.lock()`, exactly as before. + #[cfg(feature = "jitv2")] + pub fn jitv2_bind_fast_lookup(&mut self) { + let (map, pages) = self.jitv2.lock().fast_lookup_ptrs(); + self.jitv2_pfn_map = map; + self.jitv2_pages_base = pages; + } + + /// Lock-free `pfn -> *mut PhysicalCodePage`, or `None` on a miss (which + /// the caller must then resolve under the lock, since claiming mutates + /// the free/MRU lists). + /// + /// # Safety + /// Sound because the page pool is CPU-thread-only state and this is the + /// CPU thread: `page_for` has no other callers, and the one cross-thread + /// writer (a compile worker's `flush_from_jit_thread`) runs only after + /// `cpu.stop()` has joined this very thread. Both arrays are allocated + /// once and never resized, so the pointers stay valid across + /// `mega_flush` (which clears in place). See `PfnMap::as_ptr`. + #[cfg(feature = "jitv2")] + #[inline(always)] + fn jitv2_lookup_page_fast(&self, pfn: u32) -> Option<*mut crate::jitv2::PhysicalCodePage> { + if self.jitv2_pfn_map.is_null() { + return None; + } + let slot = unsafe { *self.jitv2_pfn_map.add(pfn as usize) }; + if slot == crate::jitv2::jitv2::PFN_MAP_EMPTY { + return None; + } + Some(unsafe { self.jitv2_pages_base.add(slot as usize) }) + } + fn jitv2_track_pcp(&mut self, phys_addr: u32) { let pfn = phys_addr / crate::jitv2::PAGE_SIZE; let same_page = !self.pcp.is_null() && unsafe { (*self.pcp).pfn == pfn }; @@ -3101,6 +3178,28 @@ va={:#018x} phys={:#010x} (code pfn {:#x}, page {:#010x}, word {}/{})", // CompileRequest::compiled_for_fr1). #[cfg(feature = "j2wp")] let fr1 = (self.core.cp0_status & crate::mips_core::STATUS_FR) != 0; + + // Lock-free hit path (default build only — see below). An + // already-claimed page needs nothing but the `pfn -> slot -> page` + // indirection, which is two loads from arrays that never move; the + // `jitv2.lock()` this skips was only ever protecting the *claim* + // path's free-list/MRU mutation, plus unrelated fields that all + // carry their own synchronization anyway. + // + // NOT enabled under `j2wp`: that pool's `page_for` also calls + // `touch_mru` on a hit, so a hit genuinely mutates the MRU list + // there and cannot skip the lock. (Its flush-preservation policy + // depends on that ordering — see `mega_flush`'s rank walk.) + #[cfg(not(feature = "j2wp"))] + if let Some(page) = self.jitv2_lookup_page_fast(pfn) { + self.pcp = page; + self.core.cur_code_pfn = pfn; + debug_assert_eq!(unsafe { (*self.pcp).pfn }, pfn, + "jitv2_track_pcp fast path: pfn_map[{:#x}] pointed at a slot whose own pfn is {:#x}", + pfn, unsafe { (*self.pcp).pfn }); + return; + } + let mut jit = self.jitv2.lock(); #[cfg(not(feature = "j2wp"))] let lookup = jit.page_for(pfn, page_base, self.sysad.as_ref()); From 4143426f3213810e932191d021aef470ba058e62 Mon Sep 17 00:00:00 2001 From: technomancer Date: Wed, 2 Sep 2026 22:20:48 -0700 Subject: [PATCH 2/3] do not emit pc/bd stores if they are not needed --- rules/jitv2/block-fragmentation-blocks-cse.md | 115 +++++ .../inlined-slot-pc-bd-bracket-is-dead.md | 112 +++++ src/jitv2/codegen.rs | 393 +++++++++--------- src/jitv2/mod.rs | 15 + src/mips_core.rs | 54 ++- src/mips_exec.rs | 62 +++ 6 files changed, 541 insertions(+), 210 deletions(-) create mode 100644 rules/jitv2/block-fragmentation-blocks-cse.md create mode 100644 rules/jitv2/inlined-slot-pc-bd-bracket-is-dead.md diff --git a/rules/jitv2/block-fragmentation-blocks-cse.md b/rules/jitv2/block-fragmentation-blocks-cse.md new file mode 100644 index 00000000..95c40784 --- /dev/null +++ b/rules/jitv2/block-fragmentation-blocks-cse.md @@ -0,0 +1,115 @@ +# Block fragmentation, not callouts, is what starves `opt_level=speed` + +Measured 2026-09-01 on 300 real IRIX corpus pages (`jitv2_corpus/`) via +`zz_corpus_sizes` with `IRIS_JIT_DISASM=1` and `IRIS_OPT_SPEED=1`. + +## First: measure without `developer` + +`CODEGEN_OPT_LEVEL_SPEED` (codegen.rs) defaults to +`!cfg!(feature = "developer")` — production and `lightning` get +`opt_level=speed`; `developer` gets `none`. Two consequences for anyone +measuring emitted code: + +1. A `developer` build measures **unoptimized** codegen unless you set + `IRIS_OPT_SPEED=1`. +2. Even with that set, `emit_dev_trace_bp` adds a `call_indirect` **per + instruction**. In the first run of this investigation, **11,214 of 15,723 + callouts (71%) were the dev-trace hook** — and each one is an opaque + clobber of the whole core struct plus a `brif`, so it wrecks both the + callout statistics and the block-size distribution. + +`zz_corpus_sizes` used to require `developer` (it calls `last_code_size()`, +which is developer-gated) and so silently measured the wrong thing. It now +builds without it, reporting size 0 in that case. + +**Rule: any claim about emitted-code shape must come from a non-`developer` +build.** The first pass of this investigation produced a completely wrong +field-traffic ranking (it made `pc`/`in_delay_slot` look like a 5:1 majority +of stores; the real figure is 1.59:1) purely from this contamination. + +## The actual numbers (clean build, `speed`, 300 pages) + +``` +regions=300 total_asm_instrs=495353 bytes=2146233 + mean region 7154 B, median 3200 B + loads 65908 (13%) stores 22639 (4%) calls 4509 (0.9%) + machine blocks=89906 mean 5.5 instrs median 3.0 +``` + +Field-level traffic (offsets: `hot.interrupts`=0x0, `hot.cycles`=0x8, +`pc`=0x50, `in_delay_slot`=0x58, `gpr[]`=0x68..0x164, `fpr[]`=0x178): + +| field | loads | stores | +|---|---|---| +| `gpr[*]` | 12,027 | 6,790 | +| `hot.interrupts` | 8,720 | — | +| `pc` | 3,959 | 5,884 | +| `in_delay_slot` | — | 4,900 | +| `hot.cycles` | — | 2,509 | + +## Finding 1: Cranelift's CSE works *within* a block, and only there + +Zero redundant same-address loads inside any machine block. That zero is real, +not broken instrumentation — ignoring block boundaries finds **27,668** +duplicate loads (11,522 in the clean build). Of the clean build's duplicates: + +- **58% separated by a block boundary only** — no call in between. Pure + structural loss: Cranelift would have eliminated these had the instructions + shared a block. +- 24% separated by call + block. +- 17% within the scan window with nothing between (mostly cross-region + artifacts of a flat-file scan). + +## Finding 2: callouts are *not* the main barrier + +Only 4,509 calls across 495,353 instructions (0.9%). The inline L1-D fast path +(`emit_inline_mem_guard`, gated on `dc_geometry.supported`) is doing its job — +most loads/stores never reach `emit_mem_read_callout`. Callout clobbering of +the core struct is real but rare enough not to dominate. + +**Corollary for benchmarking**: `Codegen::dc_geometry` defaults to +`unsupported()`, which skips the inline path entirely and makes every access +call out. Any harness measuring emitted code must stamp real geometry (as +`zz_corpus_sizes` does) or it measures callout-only code and is blind to the +whole inline path. + +## Finding 3: the interrupt preamble is smaller than it looks, but fragments everything + +`emit_pending_interrupt_preamble` emits, per head instruction, an +`atomic_load` of `core.hot.interrupts` + test + `brif` to a cold bail block. +Exactly 8,720 sequences, matching the 8,720 `hot.interrupts` loads (the two +counts cross-validate the detector). That is 26,160 instructions = **5.3%** of +emitted code — not the main cost by volume. + +Its real cost is structural: it is a **seqcst** load (deliberately — see the +comment at its definition, `speed` mode would otherwise be free to hoist it), +which is a full barrier for alias analysis, and its `brif` splits the block at +**every instruction boundary**. That is what holds machine blocks at a median +of 3 instructions. + +## Ranking (by evidence, not intuition) + +1. **Block fragmentation** — 6,745 provably-recoverable redundant loads, + median block of 3 instructions. Merging straight-line runs into single + blocks is the real lever. +2. **Hoisting the interrupt check to block granularity** — small by volume, + but it is the *enabler* for (1): merging pass-1 blocks without hoisting the + preamble buys little, since the preamble re-splits every instruction. + Must stay per-instruction under `jitv2_lockstep`. +3. **`pc`/`in_delay_slot` traffic** — partly addressed, see + [[inlined-slot-pc-bd-bracket-is-dead]]. +4. **GPR load/store traffic** — smaller than expected, and partly fixed for + free by (1), since Cranelift already CSEs these within a block. + +## What was *wrong* about the initial hypothesis + +The intuition going in was "callouts clobber the core struct, so nothing can +stay in host registers." That is true in principle and near-irrelevant in +practice at 0.9% call density. The measurement inverted the ranking: the +barrier is the block structure the JIT itself emits, not the callouts. + +Note also that `emit_read_gpr`/`emit_write_gpr` are plain `load`/`store` +against `core_ptr` with `MemFlagsData::trusted()` (= `notrap + aligned`, **no +alias region**). There is no register cache; promoting GPRs to host registers +is entirely Cranelift's redundant-load-elimination, which is why block scope +determines how much of it happens. diff --git a/rules/jitv2/inlined-slot-pc-bd-bracket-is-dead.md b/rules/jitv2/inlined-slot-pc-bd-bracket-is-dead.md new file mode 100644 index 00000000..ce76d096 --- /dev/null +++ b/rules/jitv2/inlined-slot-pc-bd-bracket-is-dead.md @@ -0,0 +1,112 @@ +# The inlined delay slot's `core.pc` / `in_delay_slot` bracket looks dead. It is not. + +**Status: a removal was attempted on 2026-09-01 and reverted the same day.** +This note exists so the next person who spots this "obviously redundant" +bracket does not repeat it. + +`emit_slot_semantics` (src/jitv2/codegen.rs) wraps every **inlined** delay +slot in six memory operations: + +``` +in_delay_slot = 1 +saved_pc = load core.pc ; save +core.pc = + ... the slot instruction's real semantics ... +in_delay_slot = 0 ; restore +core.pc = saved_pc ; restore +``` + +Measured on 300 real IRIX corpus pages (`zz_corpus_sizes`, +`IRIS_JIT_DISASM=1`, `opt_level=speed`, no `developer`), that is 3,574 `pc` +stores + 3,574 `in_delay_slot` stores + 1,741 `pc` loads out of 24,222 total +emitted stores — about 29% of all store traffic, all inline hot-path. It is +genuinely expensive, and it is genuinely necessary. + +## Why it looks dead + +Every argument below is *true* and still leads to the wrong conclusion: + +- An in-region branch edge is a plain `jump` to the target's block + (`emit_target_edge`) — it writes neither field. +- A region-leaving exit writes its own final `core.pc` (`emit_bail`, + `emit_absolute_pc_exit`, `emit_runtime_pc_exit`). +- Only bits 12..63 of `core.pc` are used for in-region *addressing*: every + address is `emit_vbase` (`pc & !0xFFF`) plus a compile-time word offset, + and an inlined slot is always on its branch's own page (the 0xFFC + cross-page slot is never inlined — `is_inlinable` rejects + `word >= ENTRIES_PER_PAGE`). So the low bits look irrelevant. +- `emit_exception_exit` picks its outer stage from the **compile-time** + `ctx.bd`, and `exception_other_word_block` stores both `core.pc` and + `core.in_delay_slot` itself — so the inline pair looks overwritten before + anything reads it. + +## Why it is actually live + +**`deliver_exception` (mips_core.rs) reads both fields straight out of +memory**, and the JIT's exception path gives it no other channel: + +```rust +if core.in_delay_slot { + cause |= CAUSE_BD; + core.cp0_epc = core.pc.wrapping_sub(4); +} else { + cause &= !CAUSE_BD; + core.cp0_epc = core.pc; +} +``` + +`emit_exception_call_block_body` calls `handle_exception` with **only +`(core_ptr, status)`** — `Cause.BD` is not an argument. `ctx.bd` selects +*which block runs*, not what the callee sees; `exception_other_word_block` +stores `ctx.bd` into `core.in_delay_slot` precisely *because* the callee is +about to read it back out of memory. + +And EPC needs the **exact word**, not the page: `cp0_epc = core.pc - 4` for a +delay-slot fault. The "only bits 12..63 matter" argument is correct about +in-region addressing and irrelevant here. + +So a faulting inlined delay slot needs, live in memory at the moment it +faults: `in_delay_slot = true` and `core.pc` = the slot's own address. +Removing either produces `BD=0, EPC=pc` where the interpreter produces +`BD=1, EPC=pc-4`. + +## How the removal was caught + +`cargo test --release --features jitv2 --lib jitv2::` — six `equiv_test` +failures, all delay-slot exception shapes: + +``` +adel_in_delay_slot_epc_and_bd_match_interpreter +ades_in_delay_slot_epc_and_bd_match_interpreter +overflow_in_delay_slot_traps_with_epc_and_bd_matching_interpreter +overflow_in_delay_slot_not_taken_traps_with_epc_and_bd_matching_interpreter +nested_likely_at_last_word_not_taken_annuls_foreign_slot_like_interpreter +word_both_inlined_delay_slot_and_independent_branch_target_faults_on_slot_pass_matches_interpreter +``` + +with `cp0_cause` differing by exactly bit 31 (`0x30` vs `0x8000_0030`). + +**`cpu-tests` did NOT catch it** — it ran 2101 passed / 61 failed, byte-identical +to baseline, because its coverage of delay-slot exception BD/EPC is thin. +IRIX also booted fine with the broken build. Neither is sufficient +validation for a codegen change in this area; `equiv_test` is the suite that +covers it, and it must be run before believing any change to +`emit_slot_semantics`, the exception stages, or anything else touching +`core.pc`/`in_delay_slot`. + +## If you want this traffic back + +The stores are only observable through `handle_exception`. To remove them you +would have to change the JIT→Rust exception ABI so BD and the faulting word +are passed as *arguments* (they are already compile-time constants at every +inlined-slot call site — `ctx.bd` and `ctx.word`), and have the callee use +those instead of reading `core.in_delay_slot`/`core.pc`. That is a real +design change to `emit_exception_call_block_body` + +`deliver_exception`, not a local cleanup, and it has to keep working for the +interpreter's own callers of `deliver_exception`, which genuinely do maintain +those fields live. + +Related: [[emit_absolute_pc_exit-in_delay_slot-followup]] (the clear belongs +inside `emit_absolute_pc_exit`, not in each caller), and +[[block-fragmentation-blocks-cse]] (where this measurement came from, and +what the real optimization lever turned out to be). diff --git a/src/jitv2/codegen.rs b/src/jitv2/codegen.rs index 0362ea4e..b9c37352 100644 --- a/src/jitv2/codegen.rs +++ b/src/jitv2/codegen.rs @@ -232,8 +232,12 @@ struct EmitCtx<'a, 'b> { /// no call site ever pays a runtime check for something that's actually /// fixed for that site. exception_call_block: Block, - exception_entry_word_block: Block, - exception_other_word_block: Block, + /// Shared exception-raise block: `(core_ptr, status, fault_pc, bd)`. + /// Both stage blocks that used to sit in front of this (one writing + /// compile-time word/bd into `core`, one trusting the live values) are + /// gone — `emit_exception_exit_const`/`_live` now pass those two values + /// as block args instead of materializing them into memory. See + /// `mips_core::deliver_exception_at`. /// Compile-time-only running total of retired-but-not-yet-stored /// instructions since the last `core.hot.cycles` flush — see the /// analyzer's `CompiledInstr::cycles_delta`/`cycles_flush` doc comments @@ -321,8 +325,7 @@ pub struct BlockSkeleton { /// Left unsealed for the same reason `exit_block` is — the caller must /// seal them once every exception-exit site has been emitted. pub exception_call_block: Block, - pub exception_entry_word_block: Block, - pub exception_other_word_block: Block, + /// (word offset, allocated block) for every instruction in the region, /// in ascending word-offset order (mirrors `instrs_linear`'s order). pub instr_blocks: Vec<(WordOffset, Block)>, @@ -760,29 +763,20 @@ impl Codegen { // Not sealed: predecessors are every bail site across the whole // function, established incrementally as later passes emit them. - // Shared exception-raise machinery (see BlockSkeleton's own doc - // comment for the two-stage rationale) — same block-param pattern as - // exit_block above, split into three blocks so no call site ever - // pays a runtime word==entry_word check. + // Shared exception-raise machinery — ONE block now, taking + // `(core_ptr, status, fault_pc, bd)`. The two outer stages this used + // to have (one storing compile-time word/bd into `core`, one + // trusting whatever the interpreter left live) are gone: + // `emit_exception_exit_const`/`_live` pass those two values as block + // args instead, so nothing has to be materialized into memory on the + // way to the call. Same block-param pattern as exit_block above. let exception_call_block = builder.create_block(); let call_core_ptr = builder.append_block_param(exception_call_block, ptr_ty); let call_status_param = builder.append_block_param(exception_call_block, ir::types::I32); + let call_fault_pc_param = builder.append_block_param(exception_call_block, ir::types::I64); + let call_bd_param = builder.append_block_param(exception_call_block, ir::types::I8); builder.switch_to_block(exception_call_block); - emit_exception_call_block_body(&mut self.module, &mut builder, &jit_consts, call_core_ptr, call_status_param); - - let exception_other_word_block = builder.create_block(); - let other_core_ptr = builder.append_block_param(exception_other_word_block, ptr_ty); - let other_word_param = builder.append_block_param(exception_other_word_block, ir::types::I64); - let other_bd_param = builder.append_block_param(exception_other_word_block, ir::types::I8); - let other_status_param = builder.append_block_param(exception_other_word_block, ir::types::I32); - builder.switch_to_block(exception_other_word_block); - emit_exception_other_word_block_body(&mut builder, other_core_ptr, other_word_param, other_bd_param, other_status_param, exception_call_block); - - let exception_entry_word_block = builder.create_block(); - let entry_exc_core_ptr = builder.append_block_param(exception_entry_word_block, ptr_ty); - let entry_exc_status_param = builder.append_block_param(exception_entry_word_block, ir::types::I32); - builder.switch_to_block(exception_entry_word_block); - emit_exception_entry_word_block_body(&mut builder, entry_exc_core_ptr, entry_exc_status_param, exception_call_block); + emit_exception_call_block_body(&mut self.module, &mut builder, &jit_consts, call_core_ptr, call_status_param, call_fault_pc_param, call_bd_param); // None of the three sealed here: predecessors (every emit_exception_exit // call site, plus the two outer stages' own jumps into // exception_call_block) are established incrementally as later @@ -802,7 +796,7 @@ impl Codegen { // once all of a block's predecessors are known. drop(builder); - BlockSkeleton { entry_block, exit_block, exception_call_block, exception_entry_word_block, exception_other_word_block, instr_blocks } + BlockSkeleton { entry_block, exit_block, exception_call_block, instr_blocks } } /// Signature for `JitFn` (`jitv2/jitv2.rs`): `extern "C" fn(*mut MipsCore) -> ExecStatus`. @@ -1063,8 +1057,6 @@ impl Codegen { trust_live_pc_bd_on_exc: true, exit_block: dead, exception_call_block: dead, - exception_entry_word_block: dead, - exception_other_word_block: dead, cycles_pending: &mut unused_cycles, }; @@ -1271,19 +1263,14 @@ impl Codegen { let word_offset_param = builder.append_block_param(exit_block, ir::types::I64); let exit_status_param = builder.append_block_param(exit_block, ir::types::I32); + // One shared exception block, `(core_ptr, status, fault_pc, bd)` — + // see the matching comment in `build_block_skeleton` for why the two + // outer stages are gone. let exception_call_block = builder.create_block(); let call_core_ptr = builder.append_block_param(exception_call_block, ptr_ty); let call_status_param = builder.append_block_param(exception_call_block, ir::types::I32); - - let exception_other_word_block = builder.create_block(); - let other_core_ptr = builder.append_block_param(exception_other_word_block, ptr_ty); - let other_word_param = builder.append_block_param(exception_other_word_block, ir::types::I64); - let other_bd_param = builder.append_block_param(exception_other_word_block, ir::types::I8); - let other_status_param = builder.append_block_param(exception_other_word_block, ir::types::I32); - - let exception_entry_word_block = builder.create_block(); - let entry_exc_core_ptr = builder.append_block_param(exception_entry_word_block, ptr_ty); - let entry_exc_status_param = builder.append_block_param(exception_entry_word_block, ir::types::I32); + let call_fault_pc_param = builder.append_block_param(exception_call_block, ir::types::I64); + let call_bd_param = builder.append_block_param(exception_call_block, ir::types::I8); // §13.4 internal dispatch head: this page's one compiled function may // cover several external entry points, so the function itself must @@ -1318,7 +1305,7 @@ impl Codegen { // instruction's cycles_delta/cycles_flush bookkeeping begins, // so a throwaway local is correct here (never read back). let mut unused_cycles_pending = 0u32; - let mut guard_ctx = EmitCtx { builder: &mut builder, module: &mut self.module, jit_consts, mem_helpers, core_ptr, raw: 0, word: 0, dc_geometry, bd: false, trust_live_pc_bd_on_exc: true, exit_block, exception_call_block, exception_entry_word_block, exception_other_word_block, cycles_pending: &mut unused_cycles_pending }; + let mut guard_ctx = EmitCtx { builder: &mut builder, module: &mut self.module, jit_consts, mem_helpers, core_ptr, raw: 0, word: 0, dc_geometry, bd: false, trust_live_pc_bd_on_exc: true, exit_block, exception_call_block, cycles_pending: &mut unused_cycles_pending }; emit_fr_mode_guard(&mut guard_ctx, live_entry_offset, compiled_for_fr1); } @@ -1375,7 +1362,7 @@ impl Codegen { builder.switch_to_block(stub); let raw = instrs[w as usize].raw; let mut unused_cycles_pending = 0u32; - let mut trace_ctx = EmitCtx { builder: &mut builder, module: &mut self.module, jit_consts, mem_helpers, core_ptr, raw, word: w, dc_geometry, bd: false, trust_live_pc_bd_on_exc: true, exit_block, exception_call_block, exception_entry_word_block, exception_other_word_block, cycles_pending: &mut unused_cycles_pending }; + let mut trace_ctx = EmitCtx { builder: &mut builder, module: &mut self.module, jit_consts, mem_helpers, core_ptr, raw, word: w, dc_geometry, bd: false, trust_live_pc_bd_on_exc: true, exit_block, exception_call_block, cycles_pending: &mut unused_cycles_pending }; emit_dev_trace_bp(&mut trace_ctx, origin); builder.ins().jump(real_target, &[]); builder.seal_block(stub); @@ -1399,15 +1386,9 @@ impl Codegen { // Left unsealed until every bail site below has been emitted. builder.switch_to_block(exception_call_block); - emit_exception_call_block_body(&mut self.module, &mut builder, &jit_consts, call_core_ptr, call_status_param); - - builder.switch_to_block(exception_other_word_block); - emit_exception_other_word_block_body(&mut builder, other_core_ptr, other_word_param, other_bd_param, other_status_param, exception_call_block); - - builder.switch_to_block(exception_entry_word_block); - emit_exception_entry_word_block_body(&mut builder, entry_exc_core_ptr, entry_exc_status_param, exception_call_block); - // None left sealed until every emit_exception_exit call site below - // has been emitted — same reasoning as exit_block above. + emit_exception_call_block_body(&mut self.module, &mut builder, &jit_consts, call_core_ptr, call_status_param, call_fault_pc_param, call_bd_param); + // Left unsealed until every emit_exception_exit call site below has + // been emitted — same reasoning as exit_block above. for &(word, block) in &instr_blocks { instrs[word as usize].block_id = Some(block.as_u32()); @@ -1449,7 +1430,7 @@ impl Codegen { // the right exception outer stage. let is_entry_point = instrs[word as usize].is_entry_point; let trust_live_pc_bd_on_exc = is_entry_point || instrs[word as usize].is_branch_fallback_successor; - let mut ctx = EmitCtx { builder: &mut builder, module: &mut self.module, jit_consts, mem_helpers, core_ptr, raw, word, dc_geometry, bd: false, trust_live_pc_bd_on_exc, exit_block, exception_call_block, exception_entry_word_block, exception_other_word_block, cycles_pending: &mut cycles_pending }; + let mut ctx = EmitCtx { builder: &mut builder, module: &mut self.module, jit_consts, mem_helpers, core_ptr, raw, word, dc_geometry, bd: false, trust_live_pc_bd_on_exc, exit_block, exception_call_block, cycles_pending: &mut cycles_pending }; if is_entry_point && entry_body_blocks.contains_key(&word) { // This entry word's ordinary block is reached only by @@ -1825,8 +1806,6 @@ impl Codegen { } builder.seal_block(exit_block); builder.seal_block(exception_call_block); - builder.seal_block(exception_other_word_block); - builder.seal_block(exception_entry_word_block); builder.finalize(self.module.target_config()); // Anonymous, not named: this module never looks a compiled region @@ -3098,6 +3077,7 @@ fn core_offset_of_write64_fn() -> i32 { std::mem::offset_of!(MipsCore, write64_f fn core_offset_of_write64_masked_fn() -> i32 { std::mem::offset_of!(MipsCore, write64_masked_fn) as i32 } #[cfg(feature = "jitv2")] fn core_offset_of_handle_exception_fn() -> i32 { std::mem::offset_of!(MipsCore, handle_exception_fn) as i32 } +fn core_offset_of_handle_exception_at_fn() -> i32 { std::mem::offset_of!(MipsCore, handle_exception_at_fn) as i32 } fn core_offset_of_interp_fallback_fn() -> i32 { std::mem::offset_of!(MipsCore, interp_fallback_fn) as i32 } fn core_offset_of_kill_entry_fn() -> i32 { std::mem::offset_of!(MipsCore, kill_entry_fn) as i32 } #[cfg(feature = "developer")] @@ -4958,94 +4938,34 @@ fn emit_exception_call_block_body( consts: &JitConsts, core_ptr: Value, status: Value, + fault_pc: Value, + bd: Value, ) { - let mem = MemFlagsData::trusted(); let ptr_ty = module.target_config().pointer_type(); - + // `handle_exception_at_fn`, not `handle_exception_fn`: EPC and Cause.BD + // are passed as arguments rather than left for the callee to read out of + // `core.pc`/`core.in_delay_slot`. See `mips_core::deliver_exception_at` + // for the full rationale — compiled code knows both at compile time + // almost everywhere, and materializing them into memory purely to have + // them read straight back was ~29% of all emitted store traffic. let callee = emit_hook_callee_raw(builder, consts, core_ptr, ptr_ty, - core_offset_of_handle_exception_fn()); + core_offset_of_handle_exception_at_fn()); let mut sig = module.make_signature(); sig.params.push(AbiParam::new(ptr_ty)); // core_ptr sig.params.push(AbiParam::new(ir::types::I32)); // status + sig.params.push(AbiParam::new(ir::types::I64)); // fault_pc + sig.params.push(AbiParam::new(ir::types::I8)); // bd sig.returns.push(AbiParam::new(ir::types::I32)); // ExecStatus (== status, unused) let sig_ref = builder.import_signature(sig); let core_arg = builder.ins().iadd_imm_s(core_ptr, CALLOUT_CORE_BIAS); - builder.ins().call_indirect(sig_ref, callee, &[core_arg, status]); + builder.ins().call_indirect(sig_ref, callee, &[core_arg, status, fault_pc, bd]); let ret_status = builder.ins().iconst(ir::types::I32, EXEC_COMPLETE as i64); builder.ins().return_(&[ret_status]); } -/// Outer stage for every non-entry-word `emit_exception_exit` call site, -/// shared across all of them (`word`/`bd` are genuine runtime params here, -/// unlike the entry-word stage — this one block really does serve many -/// different words, both delay-slot and non-delay-slot alike). Unconditionally -/// writes `core.pc = vbase | (word * 4)` and `core.in_delay_slot = bd` from -/// its own params — never trusts either field's live value on entry, so no -/// call site needs to rely on any upstream code having left them correct -/// (see `emit_exception_exit`'s doc comment for why leaving `in_delay_slot` -/// to inheritance was fragile) — then falls into the inner stage. Every call -/// site's `bd` is a compile-time-known literal (`emit_exception_exit` reads -/// `ctx.bd`): `true` only while inlined inside a delay slot's own semantics -/// (`emit_slot_semantics` sets `ctx.bd = true` before calling in), `false` -/// for every ordinary, non-slot head — including one that's independently -/// reachable as this same region's own branch/jump target (§6.1.4 dual -/// semantics), which is always a *different*, freshly-constructed `ctx` with -/// its own default `bd = false`. -fn emit_exception_other_word_block_body( - builder: &mut FunctionBuilder, - core_ptr: Value, - word: Value, - bd: Value, - status: Value, - call_block: Block, -) { - let mem = MemFlagsData::trusted(); - let i64t = ir::types::I64; - let pc_off = ir::immediates::Offset32::new(core_offset_of_pc()); - let flag_off = ir::immediates::Offset32::new(core_offset_of_in_delay_slot()); - - let pc = builder.ins().load(i64t, mem, core_ptr, pc_off); - let vbase = builder.ins().band_imm_s(pc, !(PAGE_SIZE as i64 - 1)); - let byte_offset = builder.ins().imul_imm_s(word, 4); - let fault_pc = builder.ins().iadd(vbase, byte_offset); - builder.ins().store(mem, fault_pc, core_ptr, pc_off); - builder.ins().store(mem, bd, core_ptr, flag_off); - builder.ins().jump(call_block, &[ir::BlockArg::Value(core_ptr), ir::BlockArg::Value(status)]); -} - -/// Outer stage for entry-word `emit_exception_exit` call sites — see -/// `BlockSkeleton::exception_entry_word_block`'s doc comment for why -/// `entry_word` is baked in as a compile-time constant here rather than -/// threaded as a block param (there's only ever one per region). -/// -/// Unconditional, same shape as `emit_exception_other_word_block_body` — no -/// runtime check. A runtime check *here* cannot work: by the time control -/// reaches this block, `core.in_delay_slot`'s live value no longer -/// distinguishes "external interpreter dispatch landed on entry_word" -/// (state already correct) from "an internal in-region branch landed on -/// entry_word" (state stale) — both are just some bit pattern in `core`, -/// with no third signal available here to tell them apart. The -/// disambiguation has to happen at the branch site instead: entry_word_block -/// (the target of every *internal* edge into entry_word, per its own doc -/// comment in `compile_region_uncommitted`) unconditionally forces -/// `core.in_delay_slot = false` and `core.pc = vbase | entry_word*4` before -/// falling into entry_word_body_block — internal edges into entry_word are -/// always ordinary fallthrough/taken-branch edges (`emit_target_edge`'s -/// `None` arm), never a delay-slot transfer, so `in_delay_slot` is always -/// `false` on that path. That leaves this block free to just assume state is -/// already correct unconditionally, exactly like the non-entry-word stage. -fn emit_exception_entry_word_block_body( - builder: &mut FunctionBuilder, - core_ptr: Value, - status: Value, - call_block: Block, -) { - builder.ins().jump(call_block, &[ir::BlockArg::Value(core_ptr), ir::BlockArg::Value(status)]); -} - /// Jump to the region's shared exception-raise machinery instead of emitting /// a fresh copy of the whole delay-slot-check-and-raise sequence at every /// call site — the exception-exit counterpart of `emit_bail`. Picks @@ -5055,28 +4975,63 @@ fn emit_exception_entry_word_block_body( /// this avoids the runtime check a single fully-shared block would need). fn emit_exception_exit(ctx: &mut EmitCtx, status: Value) { if ctx.trust_live_pc_bd_on_exc { - // entry word (state set by the interpreter dispatch that reached it) - // or a branch-fallback successor (state set by the BC1 fallback's - // interpreter run) — `core.pc`/`core.in_delay_slot` are already - // correct and must NOT be overwritten from the compile-time word/bd - // (which would clobber a slot's BD=true), so route through - // exception_entry_word_block, which trusts the live values. - ctx.builder.ins().jump(ctx.exception_entry_word_block, &[ - ir::BlockArg::Value(ctx.core_ptr), - ir::BlockArg::Value(status), - ]); + emit_exception_exit_live(ctx, status); } else { - let word_val = ctx.builder.ins().iconst(ir::types::I64, ctx.word as i64); - let bd_val = ctx.builder.ins().iconst(ir::types::I8, ctx.bd as i64); - ctx.builder.ins().jump(ctx.exception_other_word_block, &[ - ir::BlockArg::Value(ctx.core_ptr), - ir::BlockArg::Value(word_val), - ir::BlockArg::Value(bd_val), - ir::BlockArg::Value(status), - ]); + emit_exception_exit_const(ctx, status); } } +/// `emit_exception_exit` for a fault site whose faulting word and +/// delay-slot-ness are known at **compile time** — every ordinary in-region +/// instruction, including an inlined delay slot. +/// +/// `fault_pc` is `vbase + word*4` (`emit_word_addr` — position-independent, +/// §2.2) and `bd` is `ctx.bd`'s literal. Nothing is stored to `core`: both +/// values go straight into the call as arguments, which is the whole point +/// of the `handle_exception_at_fn` ABI. +/// +/// `ctx.bd` is `true` only while emitting inside a delay slot's own +/// semantics (`emit_slot_semantics` sets it before calling in) and `false` +/// for every ordinary head — including one independently reachable as a +/// branch target (§6.1.4 dual semantics), which is always a different, +/// freshly-constructed `ctx` with its own default `bd = false`. +fn emit_exception_exit_const(ctx: &mut EmitCtx, status: Value) { + let fault_pc = emit_word_addr(ctx, ctx.word); + let bd_val = ctx.builder.ins().iconst(ir::types::I8, ctx.bd as i64); + ctx.builder.ins().jump(ctx.exception_call_block, &[ + ir::BlockArg::Value(ctx.core_ptr), + ir::BlockArg::Value(status), + ir::BlockArg::Value(fault_pc), + ir::BlockArg::Value(bd_val), + ]); +} + +/// `emit_exception_exit` for a fault site that **inherited** its +/// `core.pc`/`core.in_delay_slot` from outside this compiled unit: the entry +/// word (state set by the interpreter dispatch that reached it) and a +/// branch-fallback successor (state set by the BC1 fallback's interpreter +/// run). +/// +/// Here the compile-time word/bd would be *wrong* — the entry word can be +/// arrived at as some other branch's delay slot, with `in_delay_slot` true +/// and a pending transfer armed, which `ctx.bd == false` would clobber. So +/// this one loads both from memory, where the interpreter left them, and +/// passes the loaded values as arguments. Two loads on a cold path, versus +/// the stores every *other* site would otherwise have to pay on the hot one. +fn emit_exception_exit_live(ctx: &mut EmitCtx, status: Value) { + let mem = MemFlagsData::trusted(); + let pc_off = ir::immediates::Offset32::new(core_offset_of_pc()); + let flag_off = ir::immediates::Offset32::new(core_offset_of_in_delay_slot()); + let fault_pc = ctx.builder.ins().load(ir::types::I64, mem, ctx.core_ptr, pc_off); + let bd_val = ctx.builder.ins().load(ir::types::I8, mem, ctx.core_ptr, flag_off); + ctx.builder.ins().jump(ctx.exception_call_block, &[ + ir::BlockArg::Value(ctx.core_ptr), + ir::BlockArg::Value(status), + ir::BlockArg::Value(fault_pc), + ir::BlockArg::Value(bd_val), + ]); +} + /// Exit stub for a runtime-computed target address (JR/JALR — §2.3, the /// target is a register value, not a compile-time word offset, so this /// can't go through the shared `exit_block`/`emit_bail`, which only knows @@ -5573,11 +5528,36 @@ fn emit_foreign_page_slot_exit(ctx: &mut EmitCtx, word: WordOffset, target_addr: /// word 4) and exit. No `emit_set_jit_trigger` either, matching /// `handle_branch_likely_skip` itself, which doesn't set it. Terminates the /// current block. -fn emit_foreign_page_annulled_not_taken_exit(ctx: &mut EmitCtx, word: WordOffset) { +fn emit_foreign_page_annulled_not_taken_exit(ctx: &mut EmitCtx, word: WordOffset, pending_outer_transfer: bool) { let mem = MemFlagsData::trusted(); let pc_off = ir::immediates::Offset32::new(core_offset_of_pc()); + let flag_off = ir::immediates::Offset32::new(core_offset_of_in_delay_slot()); let next_pc = emit_word_addr(ctx, word + 2); ctx.builder.ins().store(mem, next_pc, ctx.core_ptr, pc_off); + // Write `in_delay_slot` explicitly rather than inheriting it. The two + // callers need OPPOSITE values, which is exactly why this can't be left + // to whatever happened to be in the field: + // + // - **head-level** branch-likely at 0xFFC (`emit_branch_or_jump`): the + // annulled slot never runs and there is no outer branch, so nothing is + // pending — `false`. + // - **nested** branch-likely at 0xFFC (`emit_nested_foreign_page_slot_branch`): + // the *outer* branch's transfer is still live (its `branch_delay` armed + // it a dispatch ago, and `handle_branch_likely_skip` only does + // `pc += 8`), so it must stay pending across the page boundary — + // `true`, paired with the `delay_slot_target` store at that call site. + // + // Both used to be inherited from `emit_slot_semantics`' unconditional + // `in_delay_slot = 1` bracket (nested) or from it never having run + // (head). That bracket is now lockstep/developer-only — its other + // reader, `deliver_exception`, takes BD as an argument — so this exit + // owns the state it depends on. Exactly the fragility + // `rules/jitv2/emit_absolute_pc_exit-in_delay_slot-followup.md` flagged + // for the mirror-image case. Caught by + // `nested_likely_at_last_word_not_taken_annuls_foreign_slot_like_interpreter` + // and `beql_not_taken_at_0xffc_skips_slot_and_lands_on_fallthrough_directly`. + let bd_val = ctx.builder.ins().iconst(ir::types::I8, pending_outer_transfer as i64); + ctx.builder.ins().store(mem, bd_val, ctx.core_ptr, flag_off); let status = ctx.builder.ins().iconst(ir::types::I32, EXEC_COMPLETE as i64); ctx.builder.ins().return_(&[status]); } @@ -5778,7 +5758,7 @@ fn emit_branch_or_jump( // there's no pending transfer to defer onto the next page's // dispatch, matching handle_branch_likely_skip exactly // (direct pc+=8, no in_delay_slot involvement at all). - emit_foreign_page_annulled_not_taken_exit(ctx, word); + emit_foreign_page_annulled_not_taken_exit(ctx, word, false); } else { let fallthrough_word = word + 2; emit_target_edge(ctx, exit_block, block_for_word, instrs[word as usize].fallthrough_exit, fallthrough_word); @@ -6010,11 +5990,39 @@ fn emit_slot_semantics(ctx: &mut EmitCtx, instrs: &[CompiledInstr; ENTRIES_PER_P // restored back to false afterward: ctx is not reused for anything else // once this returns (the pass-2 loop constructs a fresh ctx per head). ctx.bd = true; + // ...and for the same reason, this slot's fault state is now fully + // compile-time known (`slot_word`, `bd = true`), so it must NOT be taken + // from live memory even when the *branch* it belongs to is an entry word + // / branch-fallback successor (which sets `trust_live_pc_bd_on_exc` for + // its own sake). Inheriting that flag into the slot sends + // `emit_exception_exit` down `emit_exception_exit_live`, which loads + // `core.in_delay_slot` — a field the bracket below no longer writes + // outside lockstep/developer, so BD came back false and every + // delay-slot fault delivered with Cause.BD clear. (Caught by the six + // equiv_test delay-slot exception tests; EPC was already correct, + // because `emit_word_addr` derives it from the page base either way.) + ctx.trust_live_pc_bd_on_exc = false; let mem = MemFlagsData::trusted(); let flag_off = ir::immediates::Offset32::new(core_offset_of_in_delay_slot()); let pc_off = ir::immediates::Offset32::new(core_offset_of_pc()); - let one = ctx.builder.ins().iconst(ir::types::I8, 1); - ctx.builder.ins().store(mem, one, ctx.core_ptr, flag_off); + // `core.in_delay_slot = true` for the duration of the slot — needed + // ONLY by `jitv2_lockstep` (whose compare reads it as the slot's + // post-state) and `developer` (dt tagging). + // + // It used to be unconditional, because `deliver_exception` read this + // field out of memory to decide `Cause.BD`. It no longer does: the + // exception path now calls `handle_exception_at_fn`, which takes BD as + // an *argument*, and `emit_exception_exit_const` passes `ctx.bd` — a + // compile-time literal that is `true` for exactly this case. See + // `mips_core::deliver_exception_at`. + // + // Worth ~3,574 stores per 300 real IRIX pages (measured), on the hot + // path, plus the CSE those stores were blocking. + #[cfg(any(feature = "jitv2_lockstep", feature = "developer"))] + { + let one = ctx.builder.ins().iconst(ir::types::I8, 1); + ctx.builder.ins().store(mem, one, ctx.core_ptr, flag_off); + } // jitv2_lockstep only: arm core.delay_slot_target with the branch's real // destination (already resolved by the caller — a register read for // RegJump, the branch-target/fallthrough Value for a conditional/J/JAL) @@ -6036,43 +6044,31 @@ fn emit_slot_semantics(ctx: &mut EmitCtx, instrs: &[CompiledInstr; ENTRIES_PER_P } #[cfg(not(feature = "jitv2_lockstep"))] let _ = delay_slot_target; - // Save the region's real entry pc before overwriting it — every later - // exit in this same compiled unit (emit_exit_block_body's `vbase = pc & - // !(PAGE_SIZE-1)`, emit_bail's retry word, an outer branch/jump's own - // link-register write) needs core.pc to still reflect the entry - // instruction's real page once the slot completes normally, not - // whatever the slot's own address was. Restored below on the - // slot-completed-without-trapping path only — if the slot itself raises - // an exception, control never returns here (emit_exception_exit is a - // block terminator), so there's nothing to restore on that path: the - // slot's `core.pc` write is exactly what deliver_exception needs to see - // in that case. + // Save the region's real entry pc, then point `core.pc` at the slot's + // own address for the duration of its semantics — like the + // `in_delay_slot` store above, now needed ONLY by `jitv2_lockstep` (the + // compare reads `core.pc` as post-state) and `developer` + // (`emit_dev_trace_bp` reports the slot's own pc). // - // That last sentence is the load-bearing one, and it is easy to miss: - // this store is NOT removable. `deliver_exception` (mips_core.rs) reads - // live `core.pc` to compute `cp0_epc = pc - 4` for a delay-slot fault, - // and the JIT reaches it through `emit_exception_call_block_body`, which - // passes only `(core_ptr, status)` — the faulting word is not an - // argument. Nor is `Cause.BD`: the `in_delay_slot = 1` store above is - // read out of memory by the same function. `ctx.bd` only selects which - // exception *stage block* runs, not what the callee sees. + // `deliver_exception` used to read live `core.pc` to compute + // `cp0_epc = pc - 4`, which made this mandatory; the exception path now + // passes the faulting word explicitly (`emit_exception_exit_const` -> + // `handle_exception_at_fn`), so nothing reads it back. // - // The "only bits 12..63 of core.pc matter in-region" argument (true for - // addressing — every in-region address is emit_vbase + a compile-time - // word) does NOT apply here: EPC needs the exact word. A 2026-09-01 - // attempt to cfg-gate this whole bracket down to - // jitv2_lockstep/developer was reverted after six equiv_test delay-slot - // exception failures (Cause differing by exactly bit 31). cpu-tests and - // a full IRIX boot both passed the broken build — equiv_test is the only - // suite that covers this. See - // rules/jitv2/inlined-slot-pc-bd-bracket-is-dead.md. The slot's address itself is derived from this same - // live pc load (emit_word_addr's vbase, §2.2 position independence) — - // never from compile-time page_base, which is a physical address in - // production (`comp.rs`'s `phys_base`) and would be wrong to bake into - // a value written into core.pc (a virtual address). + // Everything else in a region only ever consumes `core.pc`'s *page + // base* (`emit_vbase`, `pc & !0xFFF`, plus a compile-time word offset), + // and an inlined slot is always on its branch's own page — the 0xFFC + // cross-page slot is never inlined (`is_inlinable` rejects + // `word >= ENTRIES_PER_PAGE`; it exits via + // `emit_foreign_page_slot_exit`). So dropping this leaves every address + // computation in the region unaffected. + #[cfg(any(feature = "jitv2_lockstep", feature = "developer"))] let saved_pc = ctx.builder.ins().load(ir::types::I64, mem, ctx.core_ptr, pc_off); - let slot_addr_val = emit_word_addr(ctx, slot_word); - ctx.builder.ins().store(mem, slot_addr_val, ctx.core_ptr, pc_off); + #[cfg(any(feature = "jitv2_lockstep", feature = "developer"))] + { + let slot_addr_val = emit_word_addr(ctx, slot_word); + ctx.builder.ins().store(mem, slot_addr_val, ctx.core_ptr, pc_off); + } // The delay slot always executes exactly once here (§6.1.4 — never // conditional, never skippable), so this is unconditional too, unlike // the head-instruction loop's post-preamble placement — a slot has no @@ -6185,9 +6181,13 @@ fn emit_slot_semantics(ctx: &mut EmitCtx, instrs: &[CompiledInstr; ENTRIES_PER_P ctx.builder.seal_block(continue_block); } - let zero = ctx.builder.ins().iconst(ir::types::I8, 0); - ctx.builder.ins().store(mem, zero, ctx.core_ptr, flag_off); - ctx.builder.ins().store(mem, saved_pc, ctx.core_ptr, pc_off); + // Restore only what was actually saved above. + #[cfg(any(feature = "jitv2_lockstep", feature = "developer"))] + { + let zero = ctx.builder.ins().iconst(ir::types::I8, 0); + ctx.builder.ins().store(mem, zero, ctx.core_ptr, flag_off); + ctx.builder.ins().store(mem, saved_pc, ctx.core_ptr, pc_off); + } false } @@ -6405,7 +6405,7 @@ fn emit_nested_foreign_page_slot_branch( let target_off = ir::immediates::Offset32::new(core_offset_of_delay_slot_target()); ctx.builder.ins().store( MemFlagsData::trusted(), outer_delay_slot_target, ctx.core_ptr, target_off); - emit_foreign_page_annulled_not_taken_exit(ctx, word); + emit_foreign_page_annulled_not_taken_exit(ctx, word, true); } } } @@ -9166,16 +9166,8 @@ mod tests { let exception_call_block = builder.create_block(); let call_core_ptr = builder.append_block_param(exception_call_block, ptr_ty); let call_status_param = builder.append_block_param(exception_call_block, ir::types::I32); - - let exception_other_word_block = builder.create_block(); - let other_core_ptr = builder.append_block_param(exception_other_word_block, ptr_ty); - let other_word_param = builder.append_block_param(exception_other_word_block, ir::types::I64); - let other_bd_param = builder.append_block_param(exception_other_word_block, ir::types::I8); - let other_status_param = builder.append_block_param(exception_other_word_block, ir::types::I32); - - let exception_entry_word_block = builder.create_block(); - let entry_exc_core_ptr = builder.append_block_param(exception_entry_word_block, ptr_ty); - let entry_exc_status_param = builder.append_block_param(exception_entry_word_block, ir::types::I32); + let call_fault_pc_param = builder.append_block_param(exception_call_block, ir::types::I64); + let call_bd_param = builder.append_block_param(exception_call_block, ir::types::I8); { // Test harness for preamble emitters only (see this @@ -9187,7 +9179,7 @@ mod tests { // baked — `JitConsts::default()` is exactly that fallback. let jit_consts = JitConsts::default(); let mem_helpers = [None; MEM_HELPER_COUNT]; - let mut ctx = EmitCtx { builder: &mut builder, module: &mut codegen.module, jit_consts, mem_helpers, core_ptr, raw: 0, word: word_offset, dc_geometry, bd: false, trust_live_pc_bd_on_exc: true, exit_block, exception_call_block, exception_entry_word_block, exception_other_word_block, cycles_pending: &mut unused_cycles_pending }; + let mut ctx = EmitCtx { builder: &mut builder, module: &mut codegen.module, jit_consts, mem_helpers, core_ptr, raw: 0, word: word_offset, dc_geometry, bd: false, trust_live_pc_bd_on_exc: true, exit_block, exception_call_block, cycles_pending: &mut unused_cycles_pending }; emit(&mut ctx, exit_block, word_offset); } // Not-fired/not-pending path continues here (the preamble leaves @@ -9205,22 +9197,9 @@ mod tests { builder.seal_block(exit_block); // only predecessor in this harness is the preamble's bail site builder.switch_to_block(exception_call_block); - emit_exception_call_block_body(&mut codegen.module, &mut builder, &jit_consts, call_core_ptr, call_status_param); - - builder.switch_to_block(exception_other_word_block); - emit_exception_other_word_block_body(&mut builder, other_core_ptr, other_word_param, other_bd_param, other_status_param, exception_call_block); - - builder.switch_to_block(exception_entry_word_block); - emit_exception_entry_word_block_body(&mut builder, entry_exc_core_ptr, entry_exc_status_param, exception_call_block); + emit_exception_call_block_body(&mut codegen.module, &mut builder, &jit_consts, call_core_ptr, call_status_param, call_fault_pc_param, call_bd_param); - // Sealed together, after every predecessor edge into any of the - // three (the two outer stages' jumps into exception_call_block, - // above) has been emitted — never actually jumped to *into* from - // outside this trio in this harness, but exception_call_block's - // own in-trio predecessors must still be established first. builder.seal_block(exception_call_block); - builder.seal_block(exception_other_word_block); - builder.seal_block(exception_entry_word_block); builder.finalize(codegen.module.target_config()); } diff --git a/src/jitv2/mod.rs b/src/jitv2/mod.rs index e555d74b..93e9e3f3 100644 --- a/src/jitv2/mod.rs +++ b/src/jitv2/mod.rs @@ -216,3 +216,18 @@ mod zz_constdedup { } } } + +#[cfg(test)] +mod zz_offsets { + #[test] + fn zz_print_offsets() { + if std::env::var("IRIS_PRINT_OFFSETS").is_err() { return; } + use crate::mips_core::MipsCore; + println!("OFF gpr = {:#x}", std::mem::offset_of!(MipsCore, gpr)); + println!("OFF pc = {:#x}", std::mem::offset_of!(MipsCore, pc)); + println!("OFF hot = {:#x}", std::mem::offset_of!(MipsCore, hot)); + println!("OFF fpr = {:#x}", std::mem::offset_of!(MipsCore, fpr)); + println!("OFF nutlb = {:#x}", std::mem::offset_of!(MipsCore, nutlb)); + println!("SIZE core = {:#x}", std::mem::size_of::()); + } +} diff --git a/src/mips_core.rs b/src/mips_core.rs index 4684e5dc..890025a7 100644 --- a/src/mips_core.rs +++ b/src/mips_core.rs @@ -465,6 +465,19 @@ pub struct MipsCore { /// interpreter loop — pc is already at the vector, nothing left to do. #[cfg(feature = "jitv2")] pub handle_exception_fn: unsafe extern "C" fn(*mut core::ffi::c_void, u32) -> u32, + /// [`Self::handle_exception_fn`] with the EPC/BD inputs passed + /// explicitly: `(ctx, status, fault_pc, bd)`. See + /// [`deliver_exception_at`] for the full rationale — in short, compiled + /// code knows the faulting word and its delay-slot-ness as compile-time + /// constants at nearly every fault site, and materializing them into + /// `core.pc`/`core.in_delay_slot` purely so the callee could read them + /// back was ~29% of all emitted store traffic. + /// + /// `bd` is `u8` rather than `bool` because this is a C ABI boundary and + /// `bool`'s representation is not something to rely on across one; + /// nonzero means "in a delay slot". + #[cfg(feature = "jitv2")] + pub handle_exception_at_fn: unsafe extern "C" fn(*mut core::ffi::c_void, u32, u64, u8) -> u32, /// Fetch, decode, and execute exactly one instruction at the current /// `core.pc` through the real interpreter dispatch (`MipsExecutor::step`'s /// own fetch+exec_decoded path) and return its `ExecStatus`. Exists so @@ -1024,6 +1037,10 @@ unsafe extern "C" fn jit_hooks_not_installed_exception(_ctx: *mut core::ffi::c_v panic!("jitv2: exception hook called before MipsExecutor::install_jit_hooks"); } #[cfg(feature = "jitv2")] +unsafe extern "C" fn jit_hooks_not_installed_exception_at(_ctx: *mut core::ffi::c_void, _status: u32, _pc: u64, _bd: u8) -> u32 { + panic!("jitv2: exception_at hook called before MipsExecutor::install_jit_hooks"); +} +#[cfg(feature = "jitv2")] unsafe extern "C" fn jit_hooks_not_installed_interp_fallback(_ctx: *mut core::ffi::c_void) -> u32 { panic!("jitv2: interp_fallback hook called before MipsExecutor::install_jit_hooks"); } @@ -1149,6 +1166,7 @@ impl MipsCore { write64_masked_fn: jit_hooks_not_installed_write64_masked, #[cfg(feature = "jitv2")] handle_exception_fn: jit_hooks_not_installed_exception, + handle_exception_at_fn: jit_hooks_not_installed_exception_at, #[cfg(feature = "jitv2")] interp_fallback_fn: jit_hooks_not_installed_interp_fallback, #[cfg(feature = "jitv2")] @@ -2191,6 +2209,36 @@ impl MipsCore { /// values (`1 << 28`, `1 << 29`) are inlined below; their canonical /// definitions and doc comments live in `mips_exec.rs`. pub fn deliver_exception(core: &mut MipsCore, status: u32) { + // The interpreter maintains `pc`/`in_delay_slot` live on every dispatch, + // so reading them here is exactly right for its callers. Compiled code + // does not (or rather: would rather not — see + // `deliver_exception_at`'s doc comment), and passes them explicitly. + deliver_exception_at(core, status, core.pc, core.in_delay_slot) +} + +/// [`deliver_exception`] with the two EPC/BD inputs supplied explicitly +/// instead of read out of `core`. +/// +/// **Why this exists.** `Cause.BD` and `cp0_epc` are derived from exactly two +/// pieces of state: the faulting instruction's own address, and whether it +/// sat in a branch delay slot. The interpreter keeps both live in `MipsCore` +/// as a matter of course, so the plain [`deliver_exception`] reading them +/// back is free there. Compiled code is the opposite case: it knows both as +/// *compile-time constants* at almost every fault site (`ctx.word`, +/// `ctx.bd`), and had to spend real stores materializing them into memory +/// purely so this function could read them back — ~29% of all emitted store +/// traffic, measured over 300 real IRIX pages, most of it the +/// `in_delay_slot=1`/`pc=slot_addr` bracket `emit_slot_semantics` wraps +/// around every inlined delay slot. +/// +/// `fault_pc` is the faulting instruction's virtual address — NOT the delay +/// slot's branch, and not pre-decremented: the `- 4` for the BD case happens +/// here, exactly as it does for the interpreter. `bd` is whether that +/// instruction was in a delay slot. +/// +/// Both are ignored when `Status.EXL` was already set (a nested exception +/// leaves EPC/BD untouched), same as before. +pub fn deliver_exception_at(core: &mut MipsCore, status: u32, fault_pc: u64, bd: bool) { const EXEC_IS_TLB_REFILL: u32 = 1 << 28; const EXEC_IS_XTLB_REFILL: u32 = 1 << 29; @@ -2203,12 +2251,12 @@ pub fn deliver_exception(core: &mut MipsCore, status: u32) { cause = (cause & !CAUSE_EXCCODE_MASK) | (status & CAUSE_EXCCODE_MASK); if !was_exl { - if core.in_delay_slot { + if bd { cause |= CAUSE_BD; - core.cp0_epc = core.pc.wrapping_sub(4); + core.cp0_epc = fault_pc.wrapping_sub(4); } else { cause &= !CAUSE_BD; - core.cp0_epc = core.pc; + core.cp0_epc = fault_pc; } } core.cp0_cause = cause; diff --git a/src/mips_exec.rs b/src/mips_exec.rs index a060541d..801695d2 100644 --- a/src/mips_exec.rs +++ b/src/mips_exec.rs @@ -1647,6 +1647,17 @@ unsafe extern "C" fn jit_handle_exception(ctx: *mut core::f exec.handle_exception(status) } +/// [`jit_handle_exception`] with the EPC/BD inputs passed explicitly — see +/// `MipsCore::handle_exception_at_fn` and +/// `mips_core::deliver_exception_at` for why. Unlike its sibling, this does +/// NOT read `core.pc`/`core.in_delay_slot`: compiled code supplies both, so +/// neither has to be live in memory at the fault site. +#[cfg(feature = "jitv2")] +unsafe extern "C" fn jit_handle_exception_at(ctx: *mut core::ffi::c_void, status: u32, fault_pc: u64, bd: u8) -> u32 { + let exec = unsafe { &mut *exec_from_core::(ctx) }; + exec.handle_exception_at(status, fault_pc, bd != 0) +} + /// Force one instruction's worth of real forward progress through the /// interpreter, bypassing the JIT dispatch gate — see /// `MipsCore::interp_fallback_fn`'s doc comment for why this needs to exist @@ -2755,6 +2766,7 @@ impl MipsExecutor { self.core.write64_fn = jit_write64::; self.core.write64_masked_fn = jit_write64_masked::; self.core.handle_exception_fn = jit_handle_exception::; + self.core.handle_exception_at_fn = jit_handle_exception_at::; self.core.interp_fallback_fn = jit_interp_fallback::; self.core.kill_entry_fn = jit_kill_entry::; #[cfg(feature = "developer")] @@ -3835,6 +3847,56 @@ va={:#018x} phys={:#010x} (code pfn {:#x}, page {:#010x}, word {}/{})", status } + /// [`Self::handle_exception`] with the EPC/BD inputs passed explicitly + /// rather than read from `core.pc`/`core.in_delay_slot` — see + /// [`crate::mips_core::deliver_exception_at`] for why compiled code wants + /// this (it knows both as compile-time constants and would otherwise + /// have to store them to memory purely to have them read straight back). + /// + /// Identical in every other respect, including the `developerx` + /// bus/address-error monitor break, the LLBit clear, `syscall_pending`, + /// the nutlb flush, and the trailing `in_delay_slot = false`. + #[cfg(feature = "jitv2")] + fn handle_exception_at(&mut self, status: ExecStatus, fault_pc: u64, bd: bool) -> ExecStatus { + #[cfg(feature = "developerx")] + { + let was_exl = (self.core.cp0_status & STATUS_EXL) != 0; + let epc = if was_exl { + self.core.cp0_epc + } else if bd { + fault_pc.wrapping_sub(4) + } else { + fault_pc + }; + let exc_code = (status & CAUSE_EXCCODE_MASK) >> 2; + if exc_code == EXC_IBE || exc_code == EXC_DBE { + eprintln!("BUS ERROR ({}) at PC={:#010x} EPC={:#010x}", + if exc_code == EXC_IBE { "IBE" } else { "DBE" }, + fault_pc, epc); + return EXEC_BREAKPOINT; + } + if exc_code == EXC_ADEL || exc_code == EXC_ADES { + eprintln!("ADDRESS ERROR ({}) at PC={:#010x} EPC={:#010x} BadVAddr={:#010x}", + if exc_code == EXC_ADEL { "ADEL" } else { "ADES" }, + fault_pc, epc, self.core.cp0_badvaddr); + return EXEC_BREAKPOINT; + } + if (exc_code == EXC_TLBL || exc_code == EXC_TLBS) && (self.core.cp0_badvaddr as u32 == 0xFF800000) { + eprintln!("ADDRESS ERROR ({}) at PC={:#010x} EPC={:#010x} BadVAddr={:#010x}", + if exc_code == EXC_TLBL { "TLBL" } else { "TLBS" }, + fault_pc, epc, self.core.cp0_badvaddr); + return EXEC_BREAKPOINT; + } + } + + self.cache.set_llbit(false); + self.core.syscall_pending = false; + crate::mips_core::deliver_exception_at(&mut self.core, status, fault_pc, bd); + self.nanotlb_invalidate(); + self.core.in_delay_slot = false; + status + } + /// `handle_exception`'s exact twin, used only by `exec_syscall`: sets /// `syscall_pending` instead of clearing it (see that field's doc /// comment). A dedicated copy rather than a parameter on the shared From 5a1d7c15d5f511de1f4d9da665ad8a4a38e6b522 Mon Sep 17 00:00:00 2001 From: technomancer Date: Wed, 2 Sep 2026 22:21:28 -0700 Subject: [PATCH 3/3] unify last instruction on page and excluded instruction handling since they really are the same situation --- rules/jitv2/block-fragmentation-blocks-cse.md | 6 +- rules/jitv2/deferred-delay-slots-unified.md | 91 +++++++++++++ ...absolute_pc_exit-in_delay_slot-followup.md | 21 ++- .../inlined-slot-pc-bd-bracket-is-dead.md | 112 ---------------- rules/jitv2/inlined-slot-pc-bd-bracket.md | 122 ++++++++++++++++++ rules/jitv2/jit-v2-design.md | 8 +- src/jitv2/analyzer.rs | 89 +++++++++---- src/jitv2/codegen.rs | 13 +- src/jitv2/equiv_test.rs | 84 ++++++++++++ 9 files changed, 398 insertions(+), 148 deletions(-) create mode 100644 rules/jitv2/deferred-delay-slots-unified.md delete mode 100644 rules/jitv2/inlined-slot-pc-bd-bracket-is-dead.md create mode 100644 rules/jitv2/inlined-slot-pc-bd-bracket.md diff --git a/rules/jitv2/block-fragmentation-blocks-cse.md b/rules/jitv2/block-fragmentation-blocks-cse.md index 95c40784..4755e355 100644 --- a/rules/jitv2/block-fragmentation-blocks-cse.md +++ b/rules/jitv2/block-fragmentation-blocks-cse.md @@ -96,8 +96,10 @@ of 3 instructions. but it is the *enabler* for (1): merging pass-1 blocks without hoisting the preamble buys little, since the preamble re-splits every instruction. Must stay per-instruction under `jitv2_lockstep`. -3. **`pc`/`in_delay_slot` traffic** — partly addressed, see - [[inlined-slot-pc-bd-bracket-is-dead]]. +3. **`pc`/`in_delay_slot` traffic** — **done** (2026-09-02): the exception + ABI now passes EPC/BD as arguments, so the per-slot bracket is + lockstep/developer-only. Total emitted stores across this same 300-page + corpus went 24,222 -> 12,431. See [[inlined-slot-pc-bd-bracket]]. 4. **GPR load/store traffic** — smaller than expected, and partly fixed for free by (1), since Cranelift already CSEs these within a block. diff --git a/rules/jitv2/deferred-delay-slots-unified.md b/rules/jitv2/deferred-delay-slots-unified.md new file mode 100644 index 00000000..e822e74d --- /dev/null +++ b/rules/jitv2/deferred-delay-slots-unified.md @@ -0,0 +1,91 @@ +# A branch whose delay slot can't be inlined: one path, not two + +**2026-09-02.** Until this change, jitv2 had two unrelated answers to the same +question, and one of them was a leftover from before the answer was known. + +## The question + +A branch/jump's delay slot is architecturally indivisible from it (§6.1.4): +the slot always executes exactly once, so codegen inlines it into the +branch's own compiled unit. Sometimes it can't: + +- the slot is on the **next physical page** (branch at offset 0xFFC), or +- the slot is an **`Excluded`** instruction (COP0/MTC0, `cache`, `eret`, + `syscall`, `break`, LL/SC, BC1, CP2, any unimplemented opcode), which by + definition has no native emitter and must run through the interpreter in + *head* position, or +- the slot is otherwise unvisited. + +## The two old answers + +| slot | branch | mechanism | +|---|---|---| +| off-page (0xFFC) | **compiled**, slot deferred | `is_0xffc_branch` skips `visit_slot`; `taken_exit: ForeignPageSlot`; codegen's `emit_foreign_page_slot_exit` arms the pending transfer | +| excluded | **declined outright** | `visit_slot` returns `false`, branch never marked visited, falls out of the region | + +The second is what the first used to do, before the foreign-page case was +worked out. The analyzer's own comment gave it away — *"a slot that can't +complete disqualifies the outermost branch exactly like an excluded slot +always did"* — describing inertia, not a reason. + +`is_inlinable`'s doc comment had *already* asserted the unification: all +three ways a slot can fail to inline "collapse to the same analyzer-side fact +and the same codegen-side consequence: deferred to the next dispatch." The +analyzer just didn't act on it. + +## The unified rule + +**If the slot can't be inlined, compile the branch and hand the interpreter a +pending transfer** — arm `core.delay_slot_target`, set `core.in_delay_slot`, +land `core.pc` on the slot word, return `EXEC_COMPLETE`. The interpreter runs +the slot and retires the transfer. It does not care *why* the slot was +deferred. + +The slot's address is derivable identically in both cases: +`emit_word_addr(ctx, word + 1)`. At word 1023 that is `vbase + 1024*4` = +`vbase + 0x1000` — the next page's word 0 — because it is an `iadd`, so the +carry into bit 12 just works. (An on-page excluded slot is the easier case: +no carry at all.) There is no address asymmetry between the two; that was +the one thing that made them *look* like different problems. + +## What changed + +- **analyzer `visit`**: a failed `visit_slot` now sets `deferred_slot` + instead of `return false`. The branch is visited with + `has_inline_slot = false` and both edges forced to + `StopReason::ForeignPageSlot` (whose doc comment now says it covers both + causes — the name is historical). +- **`is_inlinable` (codegen)**: now also rejects `is_fallback` heads. + **Necessary, not cosmetic**: an `Excluded` word *can* be `visited` — as a + fallback head admitted by some other path — which would otherwise make it + look inlinable purely because the `visited` bit was set. +- **codegen otherwise unchanged.** It was already keyed on `is_inlinable` + rather than on `word == 1023`, so the deferred case flows through the + existing foreign-slot emitters untouched. That is the payoff: one predicate, + one path. + +## Payoff: simplification, not speed + +Measured honestly, on the 300-page IRIX corpus: **22 branch-with-excluded-slot +occurrences across 300 pages**, and emitted instruction count identical +(480,780 before and after). The shape is rare, and where it occurs the region +often ended nearby anyway. + +Do not expect a benchmark to move. The reason to have done it is that jitv2 +is hairy enough already, and this removes a special case that existed only +because of the order things were figured out in. A delay slot at a page break +is genuinely plausible in real code; a weird excluded instruction in one +mostly is not. + +## Tests + +- `analyzer::tests::walk_excluded_delay_slot_defers_the_slot_and_still_compiles_the_branch` + (rewritten — it previously asserted the old decline-the-branch contract). +- `equiv_test::tests::{branch_taken,branch_not_taken,jump}_with_excluded_delay_slot_defers_like_the_interpreter` + — execution-level, checking the JIT reproduces the interpreter's + `pc`/`in_delay_slot`/`delay_slot_target` exactly. Analyzer-level tests alone + would only prove the region compiles, not that it *runs* right. + +Related: [[inlined-slot-pc-bd-bracket]] — same area, same session; that change +is what made `emit_foreign_page_annulled_not_taken_exit`'s inherited +`in_delay_slot` an explicit parameter. diff --git a/rules/jitv2/emit_absolute_pc_exit-in_delay_slot-followup.md b/rules/jitv2/emit_absolute_pc_exit-in_delay_slot-followup.md index 2597a264..9c9299fa 100644 --- a/rules/jitv2/emit_absolute_pc_exit-in_delay_slot-followup.md +++ b/rules/jitv2/emit_absolute_pc_exit-in_delay_slot-followup.md @@ -11,12 +11,21 @@ because something upstream already cleared the flag first: `emit_branch_taken_edge`/`emit_nested_branch_slot`) runs after `emit_slot_semantics`'s non-terminating tail, which unconditionally clears the flag and restores `saved_pc` before returning. - **(2026-09-01)** An attempt to cfg-gate this clear (and the whole - `in_delay_slot=1`/`pc` save/restore bracket) down to - `jitv2_lockstep`/`developer` was made and **reverted** — `deliver_exception` - reads both fields out of memory, so they are load-bearing on every config. - See [[inlined-slot-pc-bd-bracket-is-dead]]. This bullet's guarantee is - intact and unconditional, exactly as originally written. + **(2026-09-02: this guarantee is GONE.)** The bracket is now + `#[cfg(any(feature = "jitv2_lockstep", feature = "developer"))]` — the + exception ABI passes `Cause.BD` and EPC as arguments, so nothing reads + those fields back for an inlined slot. See [[inlined-slot-pc-bd-bracket]]. + Compiled code no longer *sets* `in_delay_slot` for an inlined slot either, + so these call sites remain correct — the flag is simply never true there to + begin with — but they are now correct **by luck of what runs before them**, + not by an upstream guarantee. Which is exactly what this note warned about: + the removal immediately broke + `emit_foreign_page_annulled_not_taken_exit`, which had been silently + inheriting `in_delay_slot = 1` from the bracket. That one now takes an + explicit `pending_outer_transfer` parameter (its two callers need opposite + values). The general fix below — move the clear *inside* + `emit_absolute_pc_exit` — is still not done, and is now more clearly worth + doing rather than less. - The annulling-Likely not-taken arm never sets the flag in the first place (the slot is skipped entirely, mirroring `handle_branch_likely_skip`). diff --git a/rules/jitv2/inlined-slot-pc-bd-bracket-is-dead.md b/rules/jitv2/inlined-slot-pc-bd-bracket-is-dead.md deleted file mode 100644 index ce76d096..00000000 --- a/rules/jitv2/inlined-slot-pc-bd-bracket-is-dead.md +++ /dev/null @@ -1,112 +0,0 @@ -# The inlined delay slot's `core.pc` / `in_delay_slot` bracket looks dead. It is not. - -**Status: a removal was attempted on 2026-09-01 and reverted the same day.** -This note exists so the next person who spots this "obviously redundant" -bracket does not repeat it. - -`emit_slot_semantics` (src/jitv2/codegen.rs) wraps every **inlined** delay -slot in six memory operations: - -``` -in_delay_slot = 1 -saved_pc = load core.pc ; save -core.pc = - ... the slot instruction's real semantics ... -in_delay_slot = 0 ; restore -core.pc = saved_pc ; restore -``` - -Measured on 300 real IRIX corpus pages (`zz_corpus_sizes`, -`IRIS_JIT_DISASM=1`, `opt_level=speed`, no `developer`), that is 3,574 `pc` -stores + 3,574 `in_delay_slot` stores + 1,741 `pc` loads out of 24,222 total -emitted stores — about 29% of all store traffic, all inline hot-path. It is -genuinely expensive, and it is genuinely necessary. - -## Why it looks dead - -Every argument below is *true* and still leads to the wrong conclusion: - -- An in-region branch edge is a plain `jump` to the target's block - (`emit_target_edge`) — it writes neither field. -- A region-leaving exit writes its own final `core.pc` (`emit_bail`, - `emit_absolute_pc_exit`, `emit_runtime_pc_exit`). -- Only bits 12..63 of `core.pc` are used for in-region *addressing*: every - address is `emit_vbase` (`pc & !0xFFF`) plus a compile-time word offset, - and an inlined slot is always on its branch's own page (the 0xFFC - cross-page slot is never inlined — `is_inlinable` rejects - `word >= ENTRIES_PER_PAGE`). So the low bits look irrelevant. -- `emit_exception_exit` picks its outer stage from the **compile-time** - `ctx.bd`, and `exception_other_word_block` stores both `core.pc` and - `core.in_delay_slot` itself — so the inline pair looks overwritten before - anything reads it. - -## Why it is actually live - -**`deliver_exception` (mips_core.rs) reads both fields straight out of -memory**, and the JIT's exception path gives it no other channel: - -```rust -if core.in_delay_slot { - cause |= CAUSE_BD; - core.cp0_epc = core.pc.wrapping_sub(4); -} else { - cause &= !CAUSE_BD; - core.cp0_epc = core.pc; -} -``` - -`emit_exception_call_block_body` calls `handle_exception` with **only -`(core_ptr, status)`** — `Cause.BD` is not an argument. `ctx.bd` selects -*which block runs*, not what the callee sees; `exception_other_word_block` -stores `ctx.bd` into `core.in_delay_slot` precisely *because* the callee is -about to read it back out of memory. - -And EPC needs the **exact word**, not the page: `cp0_epc = core.pc - 4` for a -delay-slot fault. The "only bits 12..63 matter" argument is correct about -in-region addressing and irrelevant here. - -So a faulting inlined delay slot needs, live in memory at the moment it -faults: `in_delay_slot = true` and `core.pc` = the slot's own address. -Removing either produces `BD=0, EPC=pc` where the interpreter produces -`BD=1, EPC=pc-4`. - -## How the removal was caught - -`cargo test --release --features jitv2 --lib jitv2::` — six `equiv_test` -failures, all delay-slot exception shapes: - -``` -adel_in_delay_slot_epc_and_bd_match_interpreter -ades_in_delay_slot_epc_and_bd_match_interpreter -overflow_in_delay_slot_traps_with_epc_and_bd_matching_interpreter -overflow_in_delay_slot_not_taken_traps_with_epc_and_bd_matching_interpreter -nested_likely_at_last_word_not_taken_annuls_foreign_slot_like_interpreter -word_both_inlined_delay_slot_and_independent_branch_target_faults_on_slot_pass_matches_interpreter -``` - -with `cp0_cause` differing by exactly bit 31 (`0x30` vs `0x8000_0030`). - -**`cpu-tests` did NOT catch it** — it ran 2101 passed / 61 failed, byte-identical -to baseline, because its coverage of delay-slot exception BD/EPC is thin. -IRIX also booted fine with the broken build. Neither is sufficient -validation for a codegen change in this area; `equiv_test` is the suite that -covers it, and it must be run before believing any change to -`emit_slot_semantics`, the exception stages, or anything else touching -`core.pc`/`in_delay_slot`. - -## If you want this traffic back - -The stores are only observable through `handle_exception`. To remove them you -would have to change the JIT→Rust exception ABI so BD and the faulting word -are passed as *arguments* (they are already compile-time constants at every -inlined-slot call site — `ctx.bd` and `ctx.word`), and have the callee use -those instead of reading `core.in_delay_slot`/`core.pc`. That is a real -design change to `emit_exception_call_block_body` + -`deliver_exception`, not a local cleanup, and it has to keep working for the -interpreter's own callers of `deliver_exception`, which genuinely do maintain -those fields live. - -Related: [[emit_absolute_pc_exit-in_delay_slot-followup]] (the clear belongs -inside `emit_absolute_pc_exit`, not in each caller), and -[[block-fragmentation-blocks-cse]] (where this measurement came from, and -what the real optimization lever turned out to be). diff --git a/rules/jitv2/inlined-slot-pc-bd-bracket.md b/rules/jitv2/inlined-slot-pc-bd-bracket.md new file mode 100644 index 00000000..ebd0b657 --- /dev/null +++ b/rules/jitv2/inlined-slot-pc-bd-bracket.md @@ -0,0 +1,122 @@ +# The inlined delay slot's `core.pc` / `in_delay_slot` bracket + +**History, in order — read all three parts before touching this.** + +1. It looked dead. It was not. +2. It was made dead, deliberately, by changing the exception ABI. +3. Two other things silently depended on it. Both are now explicit. + +## What it is + +`emit_slot_semantics` (src/jitv2/codegen.rs) wraps every **inlined** delay +slot: + +``` +in_delay_slot = 1 +saved_pc = load core.pc +core.pc = + ... the slot instruction's real semantics ... +in_delay_slot = 0 +core.pc = saved_pc +``` + +As of 2026-09-02 all six of those memory operations are +`#[cfg(any(feature = "jitv2_lockstep", feature = "developer"))]`. + +## Part 1 — why it was NOT removable (2026-09-01, reverted) + +A first attempt cfg-gated the bracket on the argument that nothing in a +compiled region reads either field back: an in-region branch edge is a plain +`jump` (`emit_target_edge`), a region-leaving exit writes its own `core.pc`, +and only bits 12..63 of `core.pc` matter for in-region addressing. + +All true, and all beside the point. **`deliver_exception` (mips_core.rs) read +both fields straight out of memory:** + +```rust +if core.in_delay_slot { + cause |= CAUSE_BD; + core.cp0_epc = core.pc.wrapping_sub(4); +} +``` + +and `emit_exception_call_block_body` called `handle_exception` with only +`(core_ptr, status)`. `ctx.bd` selected *which stage block ran*, not what the +callee saw. EPC also needs the **exact word**, not the page, so the +"low bits are dead" argument did not apply. + +Caught by six `equiv_test` delay-slot exception tests, `cp0_cause` differing +by exactly bit 31. **`cpu-tests` passed the broken build (2101/61, identical +to baseline) and IRIX booted fine** — the common case self-heals, because +delivering with the stale pc resumes at the branch and simply re-executes it. +It breaks only where a handler *inspects* rather than retries (reading +`Cause.BD` to find the faulting instruction, or a non-restartable +trap/overflow/breakpoint in a slot, which would loop). + +## Part 2 — how it was actually removed (2026-09-02) + +Not by deleting the stores, but by removing the reason they existed: **pass +EPC and BD as arguments instead of through memory.** + +- `mips_core::deliver_exception_at(core, status, fault_pc, bd)` holds the + logic; `deliver_exception(core, status)` is now a two-line wrapper reading + the fields, so interpreter and `jitv2_verify` callers are untouched. +- `MipsExecutor::handle_exception_at`, and a new + `MipsCore::handle_exception_at_fn` FFI hook `(ctx, status, fault_pc, bd)`. +- Codegen splits the exit into two wrappers over **one** shared call block + (the two outer stage blocks are deleted): + - `emit_exception_exit_const` — `emit_word_addr(ctx.word)` + `iconst(ctx.bd)`. + Every ordinary in-region instruction, **including an inlined slot**. + - `emit_exception_exit_live` — two loads. Only for the entry word and + branch-fallback successor, which inherit state from outside the region. + +Measured over 300 real IRIX corpus pages (`zz_corpus_sizes`, +`IRIS_JIT_DISASM=1`, `opt_level=speed`, no `developer`): + +| | before | after | +|---|---|---| +| `pc` stores | 5,884 | 1,243 | +| `in_delay_slot` stores | 4,900 | 259 | +| `pc` loads | 3,959 | 1,967 | +| `gpr` stores | 6,790 | 6,790 | +| **total stores** | **24,222** | **12,431** | + +Half of all emitted store traffic. (`gpr` unchanged is the correctness +check — that traffic is architectural and must not move.) + +## Part 3 — the two hidden dependencies it was masking + +Both were silent inheritances of the bracket's unconditional writes, and both +now set what they need explicitly: + +**`trust_live_pc_bd_on_exc` leaked into slots.** When a branch is itself an +entry word (or branch-fallback successor) that flag is set on its `ctx`, and +`emit_slot_semantics` inherited it — routing the *slot's* fault down +`emit_exception_exit_live`, which loads a flag the bracket no longer writes. +Fixed by clearing it alongside `ctx.bd = true`: a slot's fault state is +always compile-time known. + +**`emit_foreign_page_annulled_not_taken_exit` inherited `in_delay_slot`** — +and its two callers need **opposite** values. Head-level branch-likely at +0xFFC: nothing pending, `false`. Nested: the *outer* branch's transfer is +still live, `true`. Now a `pending_outer_transfer` parameter. This is exactly +the fragility [[emit_absolute_pc_exit-in_delay_slot-followup]] flagged for the +mirror-image case, hit from the other direction. + +## What still needs the bracket + +- **`jitv2_lockstep`** — the compare reads `core.pc`/`in_delay_slot` as the + slot's post-state (and `delay_slot_target` as the expected pc, since a slot + retires *from* `in_delay_slot = true`). +- **`developer`** — `emit_dev_trace_bp` reports the slot's own pc for `dt`. + +## Testing rule + +**`equiv_test` is the only suite that covers delay-slot exception BD/EPC.** +cpu-tests and a full IRIX boot both passed the broken build. Run +`cargo test --release --features jitv2 --lib jitv2::` before believing any +change to `emit_slot_semantics`, the exception path, or anything touching +`core.pc`/`core.in_delay_slot`. + +Related: [[block-fragmentation-blocks-cse]] (where the measurement came from), +[[deferred-delay-slots-unified]] (the sibling cleanup in the same area). diff --git a/rules/jitv2/jit-v2-design.md b/rules/jitv2/jit-v2-design.md index 1ebdcdd4..08a38e3e 100644 --- a/rules/jitv2/jit-v2-design.md +++ b/rules/jitv2/jit-v2-design.md @@ -152,7 +152,7 @@ Under decline-and-defer, every stub does one thing: write the interpreter's nati > **(as-built, 2026-09-01)** For in-region *addressing* only **bits 12..63** of `core.pc` are consumed: every in-region address is `emit_vbase` (`pc & !0xFFF`) plus a *compile-time* word offset — `emit_word_addr`, `emit_write_link_register`, `emit_jump_target_addr` take the word as an argument; `emit_exit_block_body` takes it as a **block parameter**; `emit_bail` passes an `iconst`. Every store that moves `core.pc` off-page (`emit_absolute_pc_exit`, `emit_runtime_pc_exit`, `emit_foreign_page_slot_exit`, the Likely-skip arms) is immediately followed by `return_`, so no `emit_vbase` can observe one. Memory callouts don't read `core.pc` at all — `jit_read*`/`jit_write*` take the VA as an explicit argument. > -> **This does NOT make the low bits dead.** `deliver_exception` (mips_core.rs) computes `cp0_epc` from live `core.pc` — the exact word, not the page — and the JIT reaches it via `emit_exception_call_block_body`, which passes only `(core_ptr, status)`. So `core.pc` must be the *faulting instruction's own* address whenever an exception can be raised, which includes inside an inlined delay slot. A 2026-09-01 attempt to drop `emit_slot_semantics`' per-slot pc save/restore on the "low bits are dead" argument was reverted after six `equiv_test` delay-slot exception failures. See `rules/jitv2/inlined-slot-pc-bd-bracket-is-dead.md`. +> **(as-built, 2026-09-02)** The remaining exception to this rule is gone. `deliver_exception` used to compute `cp0_epc` from live `core.pc` — the exact word, not the page — which forced `emit_slot_semantics` to keep `core.pc` pointing at each inlined delay slot. The exception ABI now passes EPC and `Cause.BD` as **arguments** (`handle_exception_at_fn` -> `deliver_exception_at`), so no fault site has to materialize either into memory first. Per-slot `pc`/`in_delay_slot` stores are now `jitv2_lockstep`/`developer`-only, and total emitted store traffic across 300 real IRIX pages fell from 24,222 to 12,431. See `rules/jitv2/inlined-slot-pc-bd-bracket.md`. ### 3.5 Region exits @@ -187,11 +187,11 @@ Both arms that need "run the interpreter for real" (FR-mismatch's fallback, and - 0xFFC rule (§2.3). - BD stub variants (§3.3). -> **(as-built, 2026-09-01) An inlined slot MUST maintain live `core.pc`/`in_delay_slot`, and this is the one real exception to §3.4's "no per-instruction PC store on the fast path".** `emit_slot_semantics` brackets every inlined slot with `in_delay_slot = 1` / slot-address into `core.pc`, restoring both afterward. It is tempting to call this dead — an in-region branch edge is a plain `jump` (`emit_target_edge`) writing neither field, and `exception_other_word_block` stores both itself — but `deliver_exception` reads **both fields out of memory** to decide `Cause.BD` and `cp0_epc = pc - 4`, and `emit_exception_call_block_body` passes it only `(core_ptr, status)`. `ctx.bd` selects which exception stage block runs; it is not an argument to the callee. Dropping either store yields `BD=0, EPC=pc` where the interpreter yields `BD=1, EPC=pc-4`. +> **(as-built, 2026-09-02) An inlined slot maintains no live `core.pc`/`in_delay_slot` outside `jitv2_lockstep`/`developer`.** It used to have to: `deliver_exception` read both fields out of memory to decide `Cause.BD` and `cp0_epc = pc - 4`. The JIT→Rust exception ABI now passes both as arguments (`emit_exception_exit_const` supplies `emit_word_addr(ctx.word)` and `iconst(ctx.bd)`, both compile-time known at every in-region fault site; `emit_exception_exit_live` loads them only for the entry word / branch-fallback successor, which inherit state from outside). The two exception stage blocks collapsed into one shared call block as a result. A 2026-09-01 attempt to drop the stores *without* the ABI change was reverted after six `equiv_test` failures — see `rules/jitv2/inlined-slot-pc-bd-bracket.md`, which also records the two silent inheritances the bracket was masking. > -> Removing this traffic (~29% of all emitted stores) would require changing the JIT→Rust exception ABI to pass BD and the faulting word as arguments. See `rules/jitv2/inlined-slot-pc-bd-bracket-is-dead.md`. +> `in_delay_slot` is still read by the entry word's foreign-slot check (§6.1.4) and `jitv2_lockstep`'s compare — both head-instruction concerns, not slot concerns. > -> `in_delay_slot` is additionally read by the entry word's foreign-slot check (§6.1.4) and `jitv2_lockstep`'s compare. +> **(as-built, 2026-09-02) A slot that cannot be inlined defers, whatever the reason.** Off-page (0xFFC) and `Excluded` slots are now one path: the branch is compiled, `delay_slot_target`/`in_delay_slot` are armed, `pc` lands on the slot word, and the interpreter runs it. The excluded case previously declined the branch outright — a leftover from before the 0xFFC handling existed. See `rules/jitv2/deferred-delay-slots-unified.md`. - Branch-likely: annul semantics compiled explicitly; annulled slot still charges its interpreter-equivalent cycles. `[Q4.1]` confirm interpreter's cycle charge for annulled slots and mirror it. ### 4.4 Excluded instructions (interpreter-only, end region) diff --git a/src/jitv2/analyzer.rs b/src/jitv2/analyzer.rs index 1759e197..5847b0b8 100644 --- a/src/jitv2/analyzer.rs +++ b/src/jitv2/analyzer.rs @@ -335,6 +335,15 @@ pub enum StopReason { /// isn't there. The entry-side counterpart is `exec_decoded`'s /// `entry_offset == 0` always-probe, which already consumes this /// runtime state correctly regardless of which page armed it. + /// + /// **Not only the 0xFFC case.** Any branch whose mandatory delay slot + /// cannot be inlined ends this way — the slot being on the next physical + /// page (offset 0xFFC) and the slot being an `Excluded` instruction are + /// the same situation from the branch's point of view, and get the same + /// treatment: compile the branch, hand the interpreter a pending + /// transfer, let it run the slot. (The excluded case used to *decline the + /// branch outright* — a leftover from before the foreign-page handling + /// was worked out. Unified 2026-09-02.) ForeignPageSlot, /// The walk's instruction budget (`Analyzer::walk_bounded`) ran out /// before this edge's target could be visited. Test/tooling scaffolding @@ -1010,22 +1019,36 @@ fn visit(instrs: &mut [CompiledInstr; ENTRIES_PER_PAGE], page: &[u32; ENTRIES_PE // first, atomically (visit_slot, not visit — the slot is never itself // recursed into as a branch/jump target, only as a slot — see // visit_slot's doc comment for the nested-branch-in-slot case it does - // handle). If the slot (or its own nested slot-chain) comes back - // excluded or runs off the page, the branch/jump can't be compiled - // either — neither gets marked visited. Not charged against `budget` — - // a delay slot was never a truncation candidate (§6.1.4). Skipped - // entirely for a 0xFFC branch/jump/regjump — there is no slot to walk. + // handle). Not charged against `budget` — a delay slot was never a + // truncation candidate (§6.1.4). Skipped entirely for a 0xFFC + // branch/jump/regjump — there is no slot on *this* page to walk. + // + // If the slot can't be inlined — `Excluded`/`RegionBoundary`, or a + // nested slot-chain that hits one — the branch is NOT declined. It is + // compiled with a deferred slot, exactly like the 0xFFC case: codegen + // arms `core.delay_slot_target`, sets `core.in_delay_slot`, lands + // `core.pc` on the slot word, and returns, leaving the interpreter to + // run the slot and retire the transfer. The two situations are + // identical from the branch's point of view ("my mandatory slot is not + // something I can emit inline"), and the interpreter handles the + // resulting pending-transfer state the same way regardless of *why*. + // + // This used to `return false`, dropping the branch out of the region + // entirely — a leftover from before the foreign-page case was worked + // out, and a real cost: every branch whose slot happens to be a + // syscall/cache/eret/MFC0/LL/SC/BC1/unimplemented opcode ended its + // region one instruction early. let slot = offset + 1; - if !is_0xffc_branch && !matches!(class, Classify::Sequential) && !visit_slot(instrs, page, page_base, slot, budget) { - return false; - } + let deferred_slot = !is_0xffc_branch + && !matches!(class, Classify::Sequential) + && !visit_slot(instrs, page, page_base, slot, budget); // This word has a real, on-page inline slot iff it's a Branch/Jump/ - // RegJump AND not the 0xFFC foreign-slot case (whose mandatory slot is - // on the next, unwalkable page — nothing at `instrs[offset+1]` on this - // page belongs to it). See `has_inline_slot`'s doc comment for why - // `compute_cycles_flush` needs this. - let has_inline_slot = !is_0xffc_branch && !matches!(class, Classify::Sequential); + // RegJump whose slot codegen can actually emit — neither the 0xFFC case + // (slot on the next, unwalkable page) nor a deferred one (slot excluded). + // See `has_inline_slot`'s doc comment for why `compute_cycles_flush` + // needs this. + let has_inline_slot = !is_0xffc_branch && !deferred_slot && !matches!(class, Classify::Sequential); // Re-derive `is_branch_fallback_successor` from the page's own bytes // instead of relying on some earlier walk having visited the fallback @@ -1045,7 +1068,14 @@ fn visit(instrs: &mut [CompiledInstr; ENTRIES_PER_PAGE], page: &[u32; ENTRIES_PE budget.remaining -= 1; budget.mark_visited(offset); - if is_0xffc_branch { + if is_0xffc_branch || deferred_slot { + // Both hand the interpreter a pending transfer and exit, so every + // edge is a region exit regardless of where it points. For the 0xFFC + // case the on-page target arithmetic would additionally be *wrong* + // (see finish_visit_foreign_page_slot's doc comment); for a deferred + // slot it would merely be pointless, since codegen never consults a + // taken/fallthrough block for a branch that returns to the + // interpreter. Same reason either way, so same handling. return finish_visit_foreign_page_slot(instrs, offset, class); } finish_visit(instrs, page, page_base, offset, class, budget) @@ -1157,7 +1187,7 @@ fn finish_visit_foreign_page_slot(instrs: &mut [CompiledInstr; ENTRIES_PER_PAGE] Classify::Jump { .. } | Classify::RegJump => { instrs[offset as usize].taken_exit = Some(StopReason::ForeignPageSlot); } - Classify::Sequential | Classify::Excluded | Classify::RegionBoundary => unreachable!("is_0xffc_branch guarantees a branch/jump/regjump class"), + Classify::Sequential | Classify::Excluded | Classify::RegionBoundary => unreachable!("is_0xffc_branch/deferred_slot guarantee a branch/jump/regjump class"), } true } @@ -1480,11 +1510,17 @@ mod tests { } #[test] - fn walk_excluded_delay_slot_disqualifies_the_branch_too() { - // A branch whose delay slot is excluded can't be compiled either - // (§6.1.4: slot is an indivisible part of the branch unit) -- the - // instruction that led into the branch must record the exit instead, - // and neither the branch nor its slot may be visited. + fn walk_excluded_delay_slot_defers_the_slot_and_still_compiles_the_branch() { + // A branch whose delay slot is Excluded is compiled with a DEFERRED + // slot, exactly like a branch at 0xFFC whose slot is on the next + // page: both mean "my mandatory slot is not something codegen can + // emit inline", and both are handled by arming + // `core.delay_slot_target`/`core.in_delay_slot`, landing `core.pc` + // on the slot word, and letting the interpreter run it. + // + // This used to decline the branch outright (the region ended at + // word 0) — a leftover from before the 0xFFC foreign-slot handling + // existed. See `StopReason::ForeignPageSlot`'s doc comment. let mut page = [0u32; ENTRIES_PER_PAGE]; page[0] = 0; // nop, falls through to word 1 page[1] = i_type(OP_BEQ, 1, 2, 3); // branch at word 1, target = 1+1+3 = 5 @@ -1493,10 +1529,17 @@ mod tests { let (result, non_empty) = a.walk(&page, 0, 0); assert!(non_empty); let v: Vec<_> = instrs_linear(result).collect(); - assert_eq!(v.len(), 1, "only word 0 should be in the region"); + assert_eq!(v.len(), 2, "word 0 and the branch itself are both in the region"); assert_eq!(v[0].word, 0); - assert_eq!(v[0].fallthrough_exit, Some(StopReason::Excluded)); - assert!(!result[1].visited, "branch with an excluded delay slot must not be visited"); + assert_eq!(v[0].fallthrough_exit, None, "word 0 now continues into the branch"); + assert!(result[1].visited, "the branch is compiled, with its slot deferred"); + // Every edge exits: the branch hands the interpreter a pending + // transfer rather than resolving a taken/fallthrough block. + assert_eq!(result[1].taken_exit, Some(StopReason::ForeignPageSlot)); + assert_eq!(result[1].fallthrough_exit, Some(StopReason::ForeignPageSlot)); + assert!(!result[1].has_inline_slot, "the slot is deferred, not inlined"); + // The slot itself is NOT walked into the region — the interpreter + // runs it, and it must stay in head position for that. assert!(!result[2].visited); } diff --git a/src/jitv2/codegen.rs b/src/jitv2/codegen.rs index b9c37352..f7a8b82b 100644 --- a/src/jitv2/codegen.rs +++ b/src/jitv2/codegen.rs @@ -5407,7 +5407,18 @@ fn emit_vbase(ctx: &mut EmitCtx) -> Value { /// for an annulled Likely slot that must never run. Never a NOP, and never /// silently skipped: the instruction still executes, just on the next dispatch. fn is_inlinable(instrs: &[CompiledInstr; ENTRIES_PER_PAGE], word: WordOffset) -> bool { - (word as usize) < ENTRIES_PER_PAGE && instrs[word as usize].visited + if (word as usize) >= ENTRIES_PER_PAGE { + return false; + } + let slot = &instrs[word as usize]; + // `is_fallback`: an analyzer-`Excluded` word kept in the region as an + // interpreter-fallback *head*. It is `visited`, but it has no native + // emitter by definition, so it can never be inlined as somebody else's + // delay slot — the branch above it defers instead (analyzer's + // `deferred_slot`). Without this check a word that some *other* path + // admitted as a fallback head would look inlinable here purely because + // it happens to be marked visited. + slot.visited && !slot.is_fallback } /// `vbase | (word * 4)` as a runtime `Value` — the in-page address of diff --git a/src/jitv2/equiv_test.rs b/src/jitv2/equiv_test.rs index eacd06b2..21ad33e5 100644 --- a/src/jitv2/equiv_test.rs +++ b/src/jitv2/equiv_test.rs @@ -3403,6 +3403,90 @@ mod tests { std::mem::forget(codegen); } + /// A branch whose delay slot is an **`Excluded`** instruction (here + /// MTC0) is compiled with a *deferred* slot, exactly like a branch at + /// 0xFFC whose slot is on the next page: the JIT arms + /// `delay_slot_target`/`in_delay_slot`, lands `pc` on the slot word, and + /// returns, leaving the interpreter to run the slot and retire the + /// transfer. + /// + /// Before 2026-09-02 the analyzer declined such a branch outright (see + /// `analyzer::tests::walk_excluded_delay_slot_defers_the_slot_and_still_compiles_the_branch`), + /// so this whole shape fell out of every region — a leftover from before + /// the 0xFFC foreign-slot handling existed. This test is the execution- + /// level counterpart of that analyzer test: it checks the JIT reproduces + /// the interpreter's pending-transfer state exactly, not merely that the + /// region compiles. + fn check_excluded_delay_slot_deferred(branch_raw: u32, gpr: [u64; 32]) { + // Mid-page, so this is unambiguously about the slot being Excluded + // rather than about any page-boundary arithmetic. + let head_word: u16 = 4; + let slot_word = head_word + 1; + // MTC0 r1, $12 — Classify::Excluded, so it can never be inlined. + let excluded = make_r(crate::mips_isa::OP_COP0, crate::mips_isa::RS_MTC0, 1, 12, 0, 0); + let page = [(head_word, branch_raw), (slot_word, excluded)]; + + let page_base = 0xFFFF_FFFF_9FC0_F000u64; + let pc = page_base + (head_word as u64) * 4; + + // One dispatch: the branch itself, which arms its slot. + let (int_pc, int_bd, int_target) = interp_delay_state(&page, gpr, pc, 1); + + let mut page_words = [0u32; ENTRIES_PER_PAGE]; + for &(word, raw) in &page { page_words[word as usize] = raw; } + let mut analyzer = Analyzer::new(); + let (walked, non_empty) = analyzer.walk_bounded(&page_words, head_word, page_base as u32, usize::MAX); + assert!(non_empty); + assert!(walked[head_word as usize].visited, + "the branch must be compiled even though its slot is Excluded"); + assert!(!walked[head_word as usize].has_inline_slot, + "its slot must be deferred, not inlined"); + let mut instrs_owned = *walked; + let mut codegen = Codegen::new(); + let jit_fn: JitFn = codegen.compile_region(&mut instrs_owned, head_word, true, false) + .expect("a branch with a deferred slot must compile"); + + let (exec, _mem) = seeded_executor(gpr, pc); + let mut exec = Box::new(exec); + let status = unsafe { jit_fn(&mut exec.core as *mut MipsCore) }; + assert_eq!(status, crate::mips_exec::EXEC_COMPLETE); + assert_eq!(exec.core.pc, int_pc, "pc must match the interpreter"); + assert_eq!(exec.core.in_delay_slot, int_bd, "in_delay_slot must match the interpreter"); + if int_bd { + assert_eq!(exec.core.delay_slot_target, int_target, + "the armed transfer target must match the interpreter"); + } + std::mem::forget(codegen); + } + + #[test] + fn branch_taken_with_excluded_delay_slot_defers_like_the_interpreter() { + // BEQ r0,r0,+3 — always taken. + check_excluded_delay_slot_deferred( + make_i(crate::mips_isa::OP_BEQ, 0, 0, 3), + [0u64; 32], + ); + } + + #[test] + fn branch_not_taken_with_excluded_delay_slot_defers_like_the_interpreter() { + // BNE r0,r0,+3 — never taken; the slot still executes (non-annulling), + // so the pending transfer is the fallthrough. + check_excluded_delay_slot_deferred( + make_i(crate::mips_isa::OP_BNE, 0, 0, 3), + [0u64; 32], + ); + } + + #[test] + fn jump_with_excluded_delay_slot_defers_like_the_interpreter() { + // Unconditional J — single-edge shape. + check_excluded_delay_slot_deferred( + make_j(crate::mips_isa::OP_J, 0x3F0_F008 >> 2), + [0u64; 32], + ); + } + #[test] fn nested_branch_at_last_word_with_foreign_page_slot_matches_interpreter() { // BEQ r0,r0,+4 at 1022 -> nested BEQ r0,r0,+2 at 1023 (always taken).