feat(cpu): the instruction-granular execution path (ADR 0013), default-off - #232
Merged
Conversation
…t-off Step 2 of task #64. `Pipeline::step_instruction` runs ONE instruction to completion -- fetch, gate, execute, memory, commit -- and returns what it cost in PCycles, instead of advancing four latches once per PClock. It executes the SAME instruction stream through the SAME semantics. decode, exec::execute, alu, cop0, tlb, addr, softfloat, the caches and the exception model are shared, and the commit runs through the accurate path's own `wb_stage` by staging the instruction into `dc_wb` first. Nothing about what an instruction computes is reimplemented; only the timing model is relaxed, which is exactly and only what ADR 0013 authorizes. THE COST MODEL IS THE ACCURATE PATH'S OWN STALL REQUESTS. This is the part worth reading twice: `alu::muldiv_stall_cycles`, `fpu::stall_cycles`, `tlb::ITLB_MISS_PCYCLES`, M_RCP_REGISTER, M_ICACHE_FILL/M_DCACHE_FILL and `exception::EPILOGUE_STALL` already exist, and the accurate path SPENDS them as cycles. The fast path drains `Pipeline::stall` after each phase and CHARGES them. So there is no second table to keep in step and no constant is invented - a number wrong here is wrong in the accurate path too, which is the only arrangement under which the two can be compared at all. The cache fills are fitted rather than measured (ledger C-1) and the module says so. A branch and its delay slot execute in the SAME CALL, which is what lets this add no field to `Pipeline`: the save-state layout is untouched and ADR 0011 section 4's mode marker is still not owed. A delay slot that is itself a branch continues the loop rather than recursing. Not modeled, each deliberately: the bypass network (sequential execution makes operands current), the load interlock (a load's value is always ready), the `prev_was_run` interrupt gate (an instruction boundary is always legal), and the flush cascade (nothing younger has been fetched). Every one is a timing structure -- the layer ADR 0013 relaxes. Also extracted `apply_cop0_read` from `dc_stage`, the fourth latch-independent primitive. PR #231 deliberately left it alone because it had one caller and a seam guessed without a second caller is a guess; this is that caller. THE GATE, and two findings from building it: - The FAST path sets the boundaries and the accurate path follows. One call can retire two instructions, so the oracle is advanced to meet the fast path rather than the reverse, which it structurally could not do. - NEITHER PC-LIKE QUANTITY IS COMPARABLE. `Cpu::pc` is the IC fetch pointer and sits up to four instructions ahead of the retiring one; `dc_wb.pc` is off by exactly one, because the reverse cascade has already moved the next instruction in by the end of the tick that retired. Both were tried and each reported a divergence on the first boundary of a run that agrees on everything else. The architectural PC of a retiring instruction is not observable from outside the pipeline in the accurate mode; EPC is where it surfaces, and that IS compared as part of COP0. What carries the weight instead: every program writes a distinct value to a distinct register per instruction, so a skipped, repeated or reordered instruction shows in the GPRs within one boundary. The cost half is an A/B DIFFERENCE between two programs of the same length differing only in MULT/DIV versus NOP. The first version used a threshold on a single run and PASSED WITH THE CHARGE DELETED, because eight cold I-cache fills cleared the bar on their own -- the converging-test hazard, caught by mutation rather than by review. Mutation-checked, both restored: skipping the delay slot fails three tests; deleting the muldiv charge fails the A/B (54 with, 54 without) where the threshold version had passed. CI gains four entries for the new feature (test + clippy on the light leg, no_std build), mirroring `fast-scheduler`, because CI runs clippy exactly once and feature-gated code is invisible to every other job. Gates: fmt, clippy (workspace + the feature), test --workspace, the new gate, rustdoc, no_std for both crates, en-US, markdownlint, and the oracle -- 0 failing in the Phase 1 categories, 90 suite-wide, unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe CPU crate adds an opt-in ChangesFast execution path
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Cpu
participant Pipeline
participant Bus
Cpu->>Pipeline: step_instruction
Pipeline->>Bus: Fetch instruction and access memory
Pipeline-->>Cpu: Return instruction cost and updated state
Possibly related PRs
🚥 Pre-merge checks | ✅ 7 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (7 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/cpu.md`:
- Around line 711-716: Add the missing comma in the documentation sentence
following “MIPS calls that UNPREDICTABLE” so the two independent clauses are
correctly joined, without changing the surrounding explanation.
- Around line 732-747: Remove the commas before the essential trailing “because”
clauses in the documentation sentences describing “dc_wb.pc” and the deleted
charge passing the test. Preserve commas before trailing “because” clauses when
the main clause is negated, as in the surrounding “Neither” sentence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 03dad8b6-49b7-4329-a4d9-ee54e109a8d6
📒 Files selected for processing (7)
.github/workflows/ci.ymlcrates/rustyn64-cpu/Cargo.tomlcrates/rustyn64-cpu/src/lib.rscrates/rustyn64-cpu/src/pipeline.rscrates/rustyn64-cpu/src/pipeline/fastexec.rscrates/rustyn64-cpu/tests/fast_exec_differential.rsdocs/cpu.md
Four review findings. `Flow::Redirected` no longer carries a target. Everything that produces it has already written `next_pc` -- that is what redirecting IS -- so the payload was a second copy of the same fact and a place for the two to disagree. ERET wrote it twice; now once. The branch-in-delay-slot loop gains a documented `EXPECTED_CHAIN` bound with a debug_assert. It is NOT a safety net against an infinite loop: the loop terminates by construction, since every iteration retires an instruction. It bounds how long one call runs without returning to the scheduler, which is a SCHEDULING concern rather than a correctness one -- a chain of jumps each in the previous one's delay slot starves the RCP for its duration. A release build continues past it rather than truncating, because stopping mid-chain would execute the wrong instructions, and a wrong answer is worse than a long call. The real cap belongs in the wiring slice, where the scheduler is the thing that cares. The test budget's 400x multiplier gains its derivation: the accurate path spends one tick_at per PCycle, and the worst single instruction here is roughly DDIV 69 + an I-cache fill 46 + a D-cache fill 40 + the 5-stage fill and epilogue -- under 200. Doubling leaves room for a stall nobody remembers to account for here. Three prose fixes in docs/cpu.md, one of them applying this repo's own comma-before-because convention -- the one I cited to REJECT a suggestion on #230 -- back at two sentences where the main clause is not negated. Re-verified after the refactor: the delay-slot mutation still fails three tests. Gates: fmt, clippy (workspace + feature), test --workspace, the fast-exec gate, rustdoc, no_std, en-US, markdownlint. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
Adjudication of the Antigravity (Gemini) review — no blocking issues raised; three suggestions and one nitpick, answered in Suggestion 1 — redundant Suggestion 2 — bound the delay-slot chain. Adopted, with the reasoning made explicit because the framing matters: The loop terminates by construction — every iteration retires an instruction — so So it is a Suggestion 3 — derive the Nitpick — drop the |
Antigravity review (Gemini via Ultra)This PR adds an instruction-granular execution path ( Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/rustyn64-cpu/src/pipeline/fastexec.rs (1)
132-205: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftBound branch-delay chains before exposing instruction stepping.
A self-referential branch-delay chain can execute indefinitely inside one
Pipeline::step_instructioncall. The debug-only assertion does not provide a release-build bound, and the differential-test budget cannot run until that call returns.
crates/rustyn64-cpu/src/pipeline/fastexec.rs#L132-L205: retain resumable chain state or return at a caller-visible work quantum instead of executing unbounded branch-delay links.crates/rustyn64-cpu/tests/fast_exec_differential.rs#L253-L280: add a self-referential branch-delay regression after the implementation can return control to the test harness.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/rustyn64-cpu/src/pipeline/fastexec.rs` around lines 132 - 205, The fast-execution loop in Pipeline::step_instruction can remain inside a self-referential branch-delay chain indefinitely; add a caller-visible work quantum by preserving resumable chain state or returning after a bounded number of links, rather than relying only on the debug_assert! at crates/rustyn64-cpu/src/pipeline/fastexec.rs:132-205. Add a regression covering a self-referential branch-delay chain at crates/rustyn64-cpu/tests/fast_exec_differential.rs:253-280, verifying control returns to the test harness and execution can resume correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/rustyn64-cpu/src/pipeline/fastexec.rs`:
- Around line 132-205: The fast-execution loop in Pipeline::step_instruction can
remain inside a self-referential branch-delay chain indefinitely; add a
caller-visible work quantum by preserving resumable chain state or returning
after a bounded number of links, rather than relying only on the debug_assert!
at crates/rustyn64-cpu/src/pipeline/fastexec.rs:132-205. Add a regression
covering a self-referential branch-delay chain at
crates/rustyn64-cpu/tests/fast_exec_differential.rs:253-280, verifying control
returns to the test harness and execution can resume correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 69525817-d2af-44bc-849a-8d32d373c5ce
📒 Files selected for processing (3)
crates/rustyn64-cpu/src/pipeline/fastexec.rscrates/rustyn64-cpu/tests/fast_exec_differential.rsdocs/cpu.md
This was referenced Jul 31, 2026
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
1 participant
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.
Motivation
Step 2 of task #64, following #231's extraction.
Pipeline::step_instructionruns one instruction to completion — fetch, gate, execute, memory, commit — and returns what it cost inPCycles, instead of advancing four inter-stage latches once perPClock.pipeline.rsis 36.1% of a rendering frame; this is the change that addresses it.Behind the default-off
fast-execfeature, authorized by ADR 0013.It executes the same instruction stream
decode,exec::execute,alu,cop0,tlb,addr,softfloat, the caches, the TLB, and the exception model are all shared — and the commit runs through the accurate path's ownwb_stage, by staging the instruction intodc_wbfirst. Nothing about what an instruction computes is reimplemented. Only the timing model is relaxed, which is exactly and only what ADR 0013 authorizes.The cost model is the accurate path's own stall requests
This is the design decision the rest follows from. Every documented cost already exists in the tree, and the accurate path spends them as cycles:
MULT5,DIV37,DMULT8,DDIV69alu::muldiv_stall_cycles(UM Table 3-12)fpu::stall_cycles(UM Table 7-14)tlb::ITLB_MISS_PCYCLES(UM §4.6.2)M_RCP_REGISTER— measuredexception::EPILOGUE_STALL(UM §4.7 p. 114)M_ICACHE_FILL/M_DCACHE_FILL— fitted, not measured (ledger C-1)So the fast path drains
Pipeline::stallafter each phase and charges it, rather than carrying a table of its own. There is nothing to keep in step, and no constant is invented — a number wrong here is wrong in the accurate path too, which is the only arrangement under which the two are comparable at all. The last row is flagged in the module docs: inheriting fitted anchors is right, but a timing result from this mode is no more trustworthy thanM(RDRAM)is.No new state, deliberately
A branch and its delay slot execute in the same call. The alternative — a
pending_redirectfield onPipeline— would be state the fast path owns, which makes ADR 0011 §4's save-state mode marker fall due and breaks the layout. Executing the pair together costs nothing and owes nothing. A delay slot that is itself a branch continues the loop rather than recursing; MIPS calls that UNPREDICTABLE and the loop reproduces the cascade's sequence.Not modeled, each deliberately, and each a timing structure rather than a semantic one: the bypass network (sequential execution makes operands current), the load interlock (a load's value is always ready), the
prev_was_runinterrupt gate (an instruction boundary is always legal), the flush cascade (nothing younger has been fetched).Also extracted
apply_cop0_readfromdc_stage— the fourth latch-independent primitive. #231 left it alone on the grounds that a seam guessed without a second caller is a guess; this is that caller.Two findings from building the gate
The fast path sets the boundaries and the accurate path follows. One call can retire two instructions, so the oracle is advanced to meet the fast path — the reverse is structurally impossible.
Neither PC-like quantity is comparable across the modes. Both were tried, and each reported a divergence on the first boundary of a run that agrees on everything else:
Cpu::pcis theICfetch pointer, up to four instructions ahead of the retiring one;dc_wb.pcis off by exactly one, because the reverse cascade has already moved the next instruction in by the end of the tick that retired.The architectural PC of a retiring instruction is simply not observable from outside the pipeline in the accurate mode.
EPCis where it surfaces, and that is compared, as part of COP0. What carries the weight instead: every test program writes a distinct value to a distinct register per instruction, so a skipped, repeated, or reordered instruction shows in the GPRs within one boundary.The cost assertion had to become an A/B
The first version asserted
cost >= 8 + 5 + 37on the multiply program, and passed with the charge deleted — eight cold I-cache fills cleared the bar on their own. That is the converging-test hazard, and mutation caught it, not review.It is now the difference between two programs of the same length differing only in
MULT/DIVversusNOP, which cancels every shared cost. Asserted as==(42) rather than>=, since anything else in the difference would be an operand-dependent cost this model does not have.Mutation-checked, both restored afterwards: skipping the delay slot fails three tests; deleting the muldiv charge fails the A/B (
54 with them, 54 without) where the threshold version had passed.Disclosure: a fifth doc-comment theft, shipped in #231
Adding
sample_interrupt_linesabovefn dc_stagein the previous PR put it betweendc_stage's doc comment and the function, sodc_stagehas been undocumented onmainsince #231 and its/// \DC` — the data-cache access…block merged ontosample_interrupt_lines. The bot caught theex_gateandfetch_wordinstances in that PR; this one slipped through. Fixed here, and I sweptpipeline.rsfor others — the only remaining undocumented items inimpl` blocks are two test helpers.Verification
One guarded conditional, no pipes:
cargo fmt --all --check;cargo clippy --workspace --all-targets -- -D warnings;cargo clippy -p rustyn64-cpu --features fast-exec --all-targets -- -D warnings;cargo test --workspace;cargo test -p rustyn64-cpu --features fast-exec;RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps; theno_stdbuild for both crates;scripts/check_en_us.sh;pre-commit run markdownlint --all-files.The oracle, on the accurate path, which this must not move:
CI gains four entries for the new feature (test + clippy on the light leg, plus the
no_stdbuild), mirroringfast-scheduler— CI runs clippy exactly once and feature-gated code is invisible to every other job, so without them the gate would be a gate that never runs.What this PR does not do
It does not wire
fast-execintoSystemor the scheduler, so nothing in a shipped build reaches it and there is no throughput number yet — deliberately, so the execution path and its wiring are reviewable separately. Wiring, the core-level differential predicate on the ADR 0012 machinery, and the A-B-A measurement are the next slice.🤖 Generated with Claude Code