feat(cpu): implement the ADR 0007 five-stage pipeline - #2
Merged
Conversation
Second half of T-11-001, and the last structural decision in Phase 1 that
cannot be retrofitted. Structure only: the stages move latches and account for
time, they do not decode or execute yet (T-11-002 onward). What is real here is
the shape, the stall mechanism, the delay-slot carriage and the interrupt gate.
Latches, not stages. Five stages have four boundaries, so the state lives on the
boundaries: ic_rf, rf_ex, ex_dc, dc_wb, each carrying pc, word, occupied,
in_delay_slot and abort.
in_delay_slot rides IN the latch rather than in a global CPU flag, which is the
whole point. A multi-cycle stall between a branch and its delay slot
desynchronises a global flag -- the classic bug in this area -- whereas a flag
attached to the instruction makes Cause.BD and EPC fall out for free.
Pipeline::advance runs WB -> DC -> EX -> RF -> IC. Each stage reads its input
latch before any upstream stage writes it, so no value can move two stages in
one cycle and no double buffering is needed: the reverse order IS the latching.
Reversing it silently makes the pipeline one cycle too fast, which is why it has
a guard rather than a comment.
Interlocks are Stall { cycles, resume, cause } with an Interlock enum naming all
eight documented cases (LDI, DCB, DCM, ICB, ITM, MCI, CP0I) so a stall is always
attributable from a trace rather than being an anonymous cycle count. CP0I is
marked undocumented in place, pointing at accuracy-ledger C-3.
Exception is deliberately NOT named Fault. UM 4.5 defines a fault as the union
of interlocks and exceptions (Figure 4-11: Faults = Interlocks + Exceptions,
split Stalls vs Abort) and CEN64 follows that wider usage; only the aborting
subset rides in a latch, so it carries the narrower name. Using "fault" for the
narrow meaning would contradict the manual in the same crate that corrects
IF/DF to IC/DC.
Interrupts are sampled once per PClock in DC -- documented, not inherited from a
reference implementation: UM Figure 4-12 places INTR in the DC column and 4.7.6
lists it among the DC-stage priorities. Accepted only if the previous PCycle was
a run cycle (4.7.1). Exactly one recognition predicate exists in the tree;
carrying two subtly different ones is a known source of one-cycle discrepancies
elsewhere.
load_interlocks reproduces the hardware's IMPRECISION, which is the spec here:
it matches the load's rt against the next instruction's rs or rt encoded field
whether or not that field is used as a source, exempts $zero, and does not cross
the GPR/FPR boundary. Emulating precise behaviour would be the bug. The
parameters are named rs/rt after the manual's own field names for the same
reason -- calling them "operands" would imply a semantics the check lacks.
Seven pipeline tests. The two structural guards were MUTATION-TESTED rather than
assumed to work:
- a_value_advances_exactly_one_stage_per_cycle: reversing the cascade to run
forwards fails it.
- delay_slot_flag_survives_a_multi_cycle_stall (the Phase 1 exit criterion):
dropping the flag in transit fails it with the intended message. A global
in_delay_slot bool passes a naive test and fails this one.
Plus: a stall freezes every latch; an interrupt is refused on the cycle after a
stall; an abort kills its own stage and everything younger but nothing older; an
aborted instruction does not retire; and the load-interlock imprecision cases
including the LUI false positive and the $zero exemption.
Two existing tests had their premises invalidated and were CORRECTED rather than
patched around:
- Cpu::tick no longer retires an instruction per call. It takes 5 PCycles to
fill the pipeline (UM 4.1: "at least 5 PCycles are required to execute an
instruction"), so the test now asserts nothing retires for four cycles and the
first retires on the fifth. That is a better test than the one it replaces.
- The scheduler's step count derives from cpu_cycles() instead of Cpu::retired,
because retirement now lags stepping by the pipeline depth and the two are no
longer interchangeable. The residue invariant's third term moved to an
inter-domain CPU-vs-RCP comparison for the same reason -- it should be a
property of the clock, not of the CPU.
Gate: fmt, clippy -D warnings (0 issues), 54 tests passing (was 47), rustdoc
-D warnings, no_std thumbv7em cross-build.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the ADR 0007 five-stage pipeline (IC -> RF -> EX -> DC -> WB) for the VR4300 CPU, transitioning from a single-cycle instruction retirement model to a cycle-accurate pipeline where instructions take 5 cycles to retire. It introduces inter-stage latches, stall/interlock handling, and interrupt gating, and updates existing scheduler tests to derive CPU cycles rather than relying on instruction retirement. However, a critical correctness issue was identified in the exception propagation logic: because stages are executed in reverse order (WB -> DC -> EX -> RF -> IC), any abort/exception stamped onto upstream latches during a stage's execution is immediately overwritten and lost in the same cycle. Additionally, exceptions are misaligned because they are not stamped onto the latch containing the instruction that actually caused the exception.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Pull request overview
Implements the structural model of the VR4300 five-stage pipeline per ADR 0007, integrating it into Cpu::tick and updating scheduler invariants/tests to decouple “CPU stepping” from “instruction retirement” now that retirement lags behind execution by pipeline depth.
Changes:
- Added
Pipelinewith four inter-stage latches, stall/abort mechanics, delay-slot carriage, and DC-stage interrupt sampling/gating. - Wired
Cpu::tickto advance the pipeline once perPClockand mirror pipeline retirement intoCpu::retired. - Updated scheduler tests/invariants to use derived CPU position (
cpu_cycles()) instead of retired-work counters; documented the change in the changelog.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| crates/rustyn64-cpu/src/pipeline.rs | New pipeline model: latches, stall/abort flow, interrupt gate, and unit tests for structural invariants. |
| crates/rustyn64-cpu/src/lib.rs | Integrates the pipeline into Cpu state and tick, updating the CPU retirement semantics. |
| crates/rustyn64-core/src/scheduler.rs | Adjusts scheduler tests/residue invariant to measure CPU stepping via derived cycle position rather than retirement. |
| CHANGELOG.md | Documents the ADR 0007 pipeline landing and related test/invariant changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
PR #2 review. All four adopted; two were genuine correctness issues I had missed, and one is the same class of defect this project already wrote a lesson about. Gemini (critical) -- aborts and the reverse cascade. Its central claim was that `abort_from` gets overwritten by the cascade. Tracing it, the abort-before-move ordering is in fact correct for the only stage that currently aborts, and the flags do survive the latch moves. But two of the three sub-points were real: - The instruction fetched by `ic_stage` LATER IN THE SAME CYCLE escaped the flush entirely and would have executed down the wrong path. Fixed: an abort now raises `flush_pending` and `IC` inserts a bubble for that cycle. Bubbling rather than redirecting is the honest interim behaviour -- the exception vector is T-11-002, and declining to execute beats executing the wrong thing. - The test was shallow. It asserted latch state immediately after `abort_from` without ever advancing, so it could not distinguish a real flush from one overwritten in the same cycle. Replaced with `an_abort_survives_the_cascade`, which steps the pipeline and follows the consequences. The ordering that makes it correct was implicit and easy for a future stage to get backwards, so it is now a documented contract on `abort_from`: a stage must stamp the abort BEFORE moving its latch, because the instruction executing in stage S sits in S's INPUT latch until the move. Stamping after would flag the younger instruction and let the causing one escape -- a misalignment no single-cycle assertion catches, which is exactly why the deepened test matters. Copilot -- `Stall.resume` was stored and never read. Sharp, and the consistent call is removal rather than documentation: `advance` always runs the full cascade, so the field carried no information while looking wired. That is the precise hazard `Bus::poll_irq_at_phase` was deleted for two commits ago (engineering-lessons 3.2), and keeping it here while citing that lesson elsewhere would be incoherent. ADR 0007 does specify `(cycles, resume_stage)`; the field returns in T-11-002 when stages can stall independently and a partial resume becomes meaningful. Recorded in the type's doc so the divergence from the ADR is deliberate and visible rather than an omission. Copilot -- `stall_for(0)` silently burned a cycle: it recorded a stall, consumed a cycle via `saturating_sub`, and marked it not-a-run-cycle, which ALSO suppressed interrupt acceptance on the following cycle (UM 4.7.1). A one-cycle timing error with no visible cause. Now ignored, with a test. Copilot -- the CHANGELOG said "eight documented interlocks" and listed seven. Verified against the manual directly (mutool extraction of UM Table 4-3): the count was right and the list was incomplete. `COp` (cache operation) was missing from the enum entirely and is now added. Also caught by the deepened test rather than by inspection: my first version asserted the bubble lands in `rf_ex`, when it lands in `ic_rf` -- `rf_ex` holds the previous cycle's fetch, which has moved down. The test failing on a wrong assertion about my own latch semantics is the argument for writing it that way. Both new guards mutation-tested: removing the flush fails `an_abort_survives_the_cascade`; the zero-stall path is covered directly. Gate: fmt, clippy -D warnings (0), 56 tests passing (was 54), rustdoc -D warnings, no_std cross-build. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The rustdoc gate (RUSTDOCFLAGS=-D warnings) rejects an intra-doc link from public documentation to a private item. Caught after pushing rather than before -- I read the gate output as clean when it reported 3 issues. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
doublegate
added a commit
that referenced
this pull request
Jul 20, 2026
PR #5 review. All three adopted; Copilot's pair are a real defect in the one function whose entire purpose was preventing it. Copilot -- Regs::read and Regs::write checked the $zero rule BEFORE masking the index to 5 bits. So `write(32, v)` fell through the `i != 0` guard and landed in `gpr[32 & 31]`, which is gpr[0]: it corrupts the architectural zero, in the module that exists specifically so "$zero's hardwiring lives in exactly one place". `read(32)` had the milder version, returning gpr[0] rather than a guaranteed zero. Not reachable from decode, which masks to 5 bits already -- but `Regs` is public API and the module's whole justification is that no call site can get this wrong. A function that can be made to violate the invariant it enforces makes that promise false. Both now mask first and apply the rule to the masked index, with a test sweeping out-of-range indices and asserting that even a deliberately corrupted gpr[0] is never observable through `read`. Gemini (high) -- an unaligned instruction fetch must raise AdEL rather than fetching. Correct, and now implemented: the bus is not touched at all, because the access itself is what is invalid. Adopted with one deliberate deviation. The suggestion included `*next_pc = pc & !3; // Redirect PC or insert a bubble`, and that would be actively harmful: silently rounding the faulting address down "fixes" it and lets execution continue on a path hardware never takes, converting a raised exception into a wrong answer. Hardware goes to the exception vector. The PC is therefore left exactly as it was, with a TODO for the vector redirect in T-11-004, and the test asserts the PC is NOT realigned so a future change cannot quietly reintroduce it. The suggestion's other point -- that IC must not let a newly fetched instruction escape the flush -- is already handled by `flush_pending` from PR #2, and calling `abort_from` here routes through the same path. Worth noting the case is not reachable from straight-line execution, which advances by 4 from an aligned reset vector. It becomes reachable with the branch/jump family (T-11-004) where a computed target can be unaligned, and it is already reachable today through the public `Cpu::set_pc` that the golden-log harness uses. Gate: fmt, clippy -D warnings (0), 94 tests (was 90), rustdoc -D warnings, no_std cross-build. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
doublegate
added a commit
that referenced
this pull request
Jul 22, 2026
Adjudicating the PR #56 bot review: - **Wait for the whole command** (CodeRabbit #2, Antigravity blocking #2): `Rdp::tick` now returns without advancing when `DPC_END - DPC_CURRENT` is less than the decoded length. The rdpq microcode advances `DPC_END` incrementally as it fills the buffer, so it can land mid-command; consuming a partial multi-word primitive would decode unwritten RDRAM. New test drives a 22-word triangle with `DPC_END` first at word 10 (stalls) then at word 22 (consumed whole). Mutation-checked. - **XBUS stalls the decoder** (CodeRabbit #3): with `DPC_STATUS.XBUS` set the command source is DMEM, which is not yet wired, so the decoder must not fall back to reading RDRAM (that would decode parameter data as opcodes and desync). New test asserts no advance under XBUS. Mutation-checked. - **Independent decoder test** (CodeRabbit #1): the FIFO-walk fixture now states each command's word count explicitly rather than calling `command_len_words`, so the walk is a genuine check of the decoder against the N64brew map, not a tautology built from it. Rejected: Antigravity blocking #1 (texture rectangle is 3 words). The N64brew command map shows 0x24/0x25 as two 64-bit words (Word 0 coords + Word 1 s/t/dsdx/dtdy); there is no third dsdy/dtdx word — texrect is axis-aligned and carries only dsdx and dtdy. The decoder's 2 stands. Docs: docs/rdp.md gains the two stall conditions; CHANGELOG updated. Gates: fmt, clippy --workspace -D warnings, cargo test --workspace, rustdoc -D warnings, thumbv7em no_std, markdownlint — all green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
doublegate
added a commit
that referenced
this pull request
Jul 24, 2026
Adjudicates the Stage A review. CodeRabbit correctly found a stale scalar-only claim, but its suggested direction (revert to scalar-only) is wrong: the VU runs. Rsp::tick -> su_step dispatches COP2 (cop2 -> vu_compute/vu_single_lane) and the vector load/store family, and the n64-systemtest RSP category is Failed: 0. Fixed the stale comments to match (the correct direction): - rsp/src/lib.rs Rsp::tick: reworded to state su_step runs the full scalar+vector engine (was 'the vector unit is Sprint 2; the scalar unit runs today'). - su.rs: the COP2 dispatch comment + the cop2() doc no longer say 'computational instructions to come' -- they dispatch to the VU. - STATUS.md: cite the systemtest runner on the COP1 Failed:0 claim (CR #2). - VERSION-PLAN.md: 'seven feature releases (plus v0.4.1 doc patch)' (CR #3). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
doublegate
added a commit
that referenced
this pull request
Jul 25, 2026
Adopt Antigravity review suggestion #2: on a hung ROM or wrong entry point the sentinel-poll loop exits on the step cap, after which assert_eq!(n, 8192) fails with an opaque mismatch. An explicit non-timeout assert makes that failure mode self-explanatory. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
doublegate
added a commit
that referenced
this pull request
Jul 25, 2026
* feat(harness): author I-cache-fill hardware timing ROM Companion to the D-cache M(RDRAM) timing ROM (tools/mrdram-timing-rom/): a bare-metal N64 ROM that measures the VR4300 I-cache line-fill cost on real hardware. It runs a straight-line block of N=8192 addiu instructions (32 KiB, larger than the 16 KiB I-cache) so every 32-byte fetch line misses, times it with COP0 Count, and reports fill = (delta*2 - N)/(N/8) after subtracting the verified 1-PClock-per-instruction base. Results go to fixed RDRAM words (phys 0x10000, past the code block) and the ISViewer text channel for a flashcart to read -- the same output path as the D-cache ROM. This makes the eventual hardware measurement of M_ICACHE_FILL one console-run away, replacing the value currently FITTED from ares/cen64 (ledger C-1). - tools/mrdram-timing-rom/icache_timing.asm + .z64 (bass, ARM9 fork; blank IPL3, no Nintendo code; MIT OR Apache-2.0). - build.sh now assembles both ROMs; README documents the I-cache variant. - crates/rustyn64-test-harness/tests/icache_timing_rom.rs boots the ROM through load_direct and asserts it reads back the charged M_ICACHE_FILL (measures 46.09 in-emulator, the charged 46) -- proof the measurement path is correct end-to-end and a guard tying the ROM to the constant. - .gitignore re-includes icache_timing.z64 by exact filename; ledger C-1 and CHANGELOG note both authored hardware ROMs. Gates: cargo fmt --check, clippy -D warnings (test-harness), both ROM runners green, markdownlint on the touched docs, check_no_roms clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(harness): assert the I-cache timing ROM did not hit the step cap Adopt Antigravity review suggestion #2: on a hung ROM or wrong entry point the sentinel-poll loop exits on the step cap, after which assert_eq!(n, 8192) fails with an opaque mismatch. An explicit non-timeout assert makes that failure mode self-explanatory. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Second half of T-11-001, and the last structural decision in Phase 1 that cannot be retrofitted.
Scope
Structure, not instructions. The stages move latches and account for time; decode and execute are T-11-002 onward. What is real here is the shape — the part that would require rewriting every consumer if it were changed later.
What landed
ic_rf,rf_ex,ex_dc,dc_wb.in_delay_slotrides in the latch, not in a global CPU flag. A multi-cycle stall between a branch and its delay slot desynchronises a global flag — the classic bug here. Attached to the instruction,Cause.BD/EPCfall out for free.Interlocknames all eight documented cases so a stall is attributable from a trace, not an anonymous cycle count.CP0Iis marked undocumented in place (accuracy-ledger C-3).Exception, deliberately notFault— UM §4.5 defines a fault as interlocks ∪ exceptions; only the aborting subset rides in a latch.PCyclebeing a run cycle (§4.7.1). Exactly one recognition predicate exists.load_interlocksreproduces the hardware's imprecision — matches on thersorrtencoded field whether or not used as a source, exempts$zero, doesn't cross GPR/FPR. Emulating precise behaviour would be the bug.Tests: 54 passing (was 47)
The two structural guards were mutation-tested, not assumed to work:
a_value_advances_exactly_one_stage_per_cycledelay_slot_flag_survives_a_multi_cycle_stallPlus: stall freezes every latch; interrupt refused on the cycle after a stall; abort kills younger-only; aborted instructions don't retire; load-interlock imprecision including the
LUIfalse positive and the$zeroexemption.Two tests corrected rather than patched around
Cpu::tickno longer retires per call — it takes 5PCycles to fill the pipeline (UM §4.1). The test now asserts nothing retires for four cycles and the first retires on the fifth.cpu_cycles()instead ofCpu::retired, since retirement now lags stepping by the pipeline depth. The residue invariant's third term moved to an inter-domain CPU-vs-RCP comparison — it should be a property of the clock, not the CPU.Gate
fmt,clippy -D warnings(0 issues), 54 tests,rustdoc -D warnings,no_stdthumbv7em cross-build.