perf(rsp): census which COP2 ops actually run — 62% is one dispatch function - #250
Merged
Conversation
…unction
`vu.rs` is 143 functions and ~8.5% of a frame, and vectorizing it means writing
`unsafe` intrinsics. Hand-vectorizing 143 functions to recover the cost of the
few that matter is the expensive way round, so: count first.
`work-counters` gains a 64-slot histogram of COP2 computational `funct` values,
reported by `work_bench`. Super Mario 64, 120 frames, 14,577,323 ops:
0x0e VMADN 17.60% -> 17.60% cumulative
0x0f VMADH 14.05% -> 31.65%
0x0d VMADM 8.93% -> 40.58%
0x04 VMUDL 8.60% -> 49.18%
... twelve ops reach 80.69%
Only 32 of the 64 possible `funct` values ever appear, and at ~121,478 per frame
against 294,983 RSP instructions, **41% of everything the RSP executes is a VU
computation**.
The result that decides the shape of the work: `funct 0x00..=0x0F` — the whole
multiply/multiply-accumulate family — is **61.64%**, and it is dispatched by ONE
function, `multiply_lane`. That function is also the natural SIMD target: eight
independent 16x16 lane products into a 48-bit accumulator.
It bounds the ambition too, which matters more. The VU is ~8.5% of a frame and
62% of it is ~5.3%; a perfect vectorization cannot exceed that, and the real
figure will be lower because dispatch, register reads and accumulator writeback
do not vanish. SIMD work here gets measured against 5.3%, not 8.5%.
`[u64; 64]` has no `Default`, so the field needs an explicit
`default = "zeroed_funct_histogram"` — the compiler catching, at the type level,
the same class of mistake #245 shipped at runtime.
|
Warning Review limit reached
Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe RSP now records COP2 function-code frequencies under ChangesCOP2 histogram instrumentation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant work_bench
participant Rsp
participant cop2
participant VU_dispatch
work_bench->>Rsp: run benchmark
cop2->>Rsp: record funct value
cop2->>VU_dispatch: dispatch COP2 instruction
work_bench->>Rsp: read vu_funct_histogram()
Rsp-->>work_bench: return operation counts
work_bench-->>work_bench: calculate and print workload shares
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 8 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (8 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: 3
🤖 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 `@crates/rustyn64-rsp/src/lib.rs`:
- Around line 146-149: Revise the hotspot-census wording to avoid presenting
unsafe RSP SIMD or vectorization of vu.rs as an available implementation path,
while preserving that the census identifies optimization hotspots for a future
policy-compliant design. Update the documentation at
crates/rustyn64-rsp/src/lib.rs lines 146-149,
crates/rustyn64-frontend/examples/work_bench.rs lines 173-175,
docs/performance.md lines 2189-2192, and CHANGELOG.md lines 97-100; reflect that
unsafe remains restricted to the frontend and FFI.
- Around line 277-306: Add a feature-gated execution test near the existing
deserialization/reset coverage that dispatches one COP2 computational
instruction, asserts the corresponding vu_funct_histogram bucket increments
exactly once, deserializes the state, and then verifies that bucket resets.
Ensure the test uses the real execution path and fails when count_vu_funct does
not update the histogram.
- Around line 143-155: Update docs/rsp.md to document the work-counters feature
as default-off, describe that vu_funct tracks executions for all 64 COP2
computational funct slots, and state that the histogram is skipped during
serialization and reset on deserialization. Keep the implementation unchanged
and do not use docs/performance.md as the feature contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3f0ea481-ef61-4dbe-816a-f788761b59c7
📒 Files selected for processing (5)
CHANGELOG.mdcrates/rustyn64-frontend/examples/work_bench.rscrates/rustyn64-rsp/src/lib.rscrates/rustyn64-rsp/src/su.rsdocs/performance.md
There was a problem hiding this comment.
Pull request overview
Adds a work-counters-gated COP2 “computational funct” histogram to the RSP so real workloads can be censused before attempting SIMD/vectorization work, and documents the resulting dispatch shape/upper-bound for expected speedups.
Changes:
- Add a 64-slot per-
functexecution histogram toRsp(behindwork-counters) and increment it on COP2 computational dispatch. - Expose the histogram via
Rsp::vu_funct_histogram()and report it fromexamples/work_bench.rs. - Document the census results and implications in
docs/performance.mdand theCHANGELOG.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/performance.md | Documents the COP2 funct census results and the implied SIMD ceiling. |
| crates/rustyn64-rsp/src/su.rs | Hooks the COP2 computational dispatch path to increment the histogram. |
| crates/rustyn64-rsp/src/lib.rs | Adds the histogram field/API and the counter increment helper under work-counters. |
| crates/rustyn64-frontend/examples/work_bench.rs | Prints the histogram ranking and a 0x00..=0x0F rollup in the bench output. |
| CHANGELOG.md | Records the new census feature and its headline findings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…fe is available Eight review findings. Two matter. THE MEASUREMENT BUG. The histogram was reported RAW while every other figure in `work_bench` is a delta over the timed window, so ~36 warm-up frames were folded into a table captioned "120 frames". Now snapshotted before the loop and subtracted. The effect is 0.06% (14,577,323 -> 14,569,003) because boot barely exercises the VU, and the family share moves 61.64% -> 61.62% — but a number that is right by accident is still not measured. THE POLICY CLAIM. `crates/rustyn64-rsp` is `#![forbid(unsafe_code)]`, so "vectorizing means unsafe intrinsics" presented a path that is not currently available. The maintainer has agreed in principle to a scoped exception by ADR; that ADR does not exist. The census identifies a hotspot; it does not authorize a technique. Corrected in the field doc, the example, docs/performance.md and the CHANGELOG. Also: - `docs/rsp.md` gains the work-counter contract — what each counts, why `retired` counts executed rather than stepped, the `#[serde(skip)]` reset semantics, and the forbid(unsafe_code) note. Chip changes update the chip doc. - `a_cop2_instruction_increments_its_own_bucket` executes a real `VMUDN` and asserts its OWN bucket moved and that exactly one bucket moved, then round trips a save-state. Two properties together, because an all-zero histogram passes a reset check and a wrong-bucket counter passes a total check. - `count_vu_funct` uses `wrapping_add`, matching the two counters standardized last PR. - The vacuity assert's wrapped-space run is gone. REJECTED: that `[u64; 64]` implements `Default` on 1.96.0 and the explicit serde default is unnecessary. It does not — `let _: [u64; 64] = Default::default();` fails to compile with E0277 on this toolchain. The doc now states the observed compiler behavior and gives that one-line reproduction, rather than asserting a rule about where the array impls stop.
The wrapped-space run in the vacuity assert was NOT fixed in 8416198 — the replacement anchor missed because rustfmt had already reflowed the literal, and I reported it adopted without re-reading the line. It rendered as "would be a table of zeros". Now fixed and verified by rendering the literal the way rustc does (backslash-newline eats the newline AND the leading whitespace) and asserting no run of three spaces survives. Also from the same review: `filter().map()` becomes `filter_map`, and `count_vu_funct` binds the masked index once instead of computing it twice.
Owner
Author
Adjudication — Antigravity reviewTwo nitpicks adopted, one suggestion adopted-late (I had reported it fixed and it was not), one suggestion rejected, and the blocking issue refuted by running the exact command. Blocking: build breaks with
|
Antigravity review (Gemini via Ultra)This PR adds an optional 64-slot COP2 Blocking issues
Suggestions
Nitpicks
Automated first-pass review by |
This was referenced Aug 1, 2026
doublegate
added a commit
that referenced
this pull request
Aug 1, 2026
"A design that checks a feature flag per instruction has spent the win before it starts" is a performance claim with no experiment behind it, inside a document whose entire verdict is arithmetic over measured shares. Nothing here has built or timed a dispatch shape. Restated as a named risk with its reasoning: the 5.3% ceiling is spread over ~121,478 VU operations per frame (#250), so the per-operation budget is small enough that a branch plus a non-inlinable call could plausibly consume it. Gate 3's A-B-A is what would settle it. The advice survives because hoisting the check costs nothing to prefer up front — which is the only defensible reason to state a preference before measuring it.
doublegate
added a commit
that referenced
this pull request
Aug 1, 2026
…1.056x) (#252) * docs(adr): ADR 0016 — a scoped unsafe exception for the RSP vector unit Three merged documents currently say the same thing: that vectorizing the VU needs an `unsafe` exception, that the maintainer agreed to one in principle, and that the ADR does not exist. This is that ADR, so the claim stops being a forward reference. It permits `core::arch` intrinsics in `vu.rs` ONLY — not raw pointers, not `transmute`, not `get_unchecked`, not FFI — with the crate-level `forbid` becoming `deny` and a single module-level `allow`. Every other chip crate is untouched. Four gates, and they are gates rather than aspirations: 1. A scalar/vector EQUIVALENCE test over the operand space, including saturation, the VMUDL/VMUDN signedness asymmetry and accumulator wrap. Conformance to the ROM suite is necessary and explicitly NOT sufficient: the suite exercises what microcode happens to use, and a divergence outside that set is exactly the bug intrinsics invite. 2. `rsp_categories_report_no_failures` stays at 0 failing. 3. A-B-A against the 5.3% ceiling the census established, neutral-or-worse reverted like the family hoist and PGO were. 4. A tested fallback — SSE2 is baseline on x86_64, SSSE3/SSE4.1 are not, and aarch64 is supported. It authorizes a technique; it does not schedule the work, and it does not pretend the trade is good. The costs are stated: the chip crates stop having zero `unsafe`, the VU acquires a portability surface, and two implementations must agree forever. Against a 5.3% ceiling that is marginal, and the ADR says so and names the comparison — the CPU is 32.29%, roughly six times larger, and anyone reaching for this should first have a reason not to spend the effort there instead. Implementation note recorded for whoever takes it: `multiply_lane` is PER-LANE, so this is a restructure rather than an annotation — the 48-bit accumulator has no native vector type and needs 16-bit planes, as parallel-rsp does. * docs(adr): state that restriction 1 is review-enforced, not compiler-enforced Seven review findings on ADR 0016, and one of them is the kind of error the ADR itself is about. `unsafe_code` is a BINARY lint. It cannot permit a `core::arch` call and reject a `transmute` in the same module, so "intrinsics only" is enforced by REVIEW, not by the compiler. Writing it as a scope restriction alongside two that the compiler does enforce made it read as mechanical. That is a rule stated as if implemented — at policy level, which is worse than in a comment, because the whole point of an ADR is to be trusted later without re-derivation. Now said outright, with the narrow module scope named as what makes the review tractable, and a CI grep suggested for when the exception is actually used. The scope item was also self-contradictory: "the rest of the crate keeps `forbid`" while the crate-level attribute becomes `deny` so `vu.rs` can opt out. `forbid` exists precisely so an inner `allow` cannot override it, so the two cannot both hold. Spelled out per-attribute, and the consequence promoted into its own cost bullet: the crate drops from `forbid` to `deny`, which is a strictly larger weakening than "vu.rs may use intrinsics" and the part most likely to be forgotten. Also adopted: `#[target_feature]`'s structural cost noted at gate 4 (those functions are unsafe to call and do not inline across the boundary, so the dispatch must be amortized above the hot loop — a per-instruction feature check has spent the win before it starts); `docs/rsp.md`'s heading no longer says "does not authorize a technique" when the section now says it does; the CHANGELOG census bullet no longer claims the ADR does not exist two entries below the ADR; and the bare issue numbers are links. * docs(adr,policy): sync AGENTS.md with ADR 0016, and correct a claim it had outrun A reviewer pointed out that the ADR's `Amends` line named two policy files while the PR touched neither, so the tree would carry the old blanket rule alongside the new exception. Chasing that turned up two errors. The `Amends` line named `docs/architecture.md`, which says NOTHING about `unsafe`. That was my invention. It now names `AGENTS.md` only, and says so. `AGENTS.md` claimed "there is zero `unsafe` in the tree today — keep it that way". That has been FALSE since #241: `rustyn64-rdp-gpu` has 12 `unsafe` blocks, the parallel-rdp FFI shim, quarantined there deliberately under ADR 0014. The policy line had outrun the tree by four PRs and nothing failed, because a statement about a repository is not checked by anything — the same decay this project has already recorded three times for "undocumented". `AGENTS.md` now states the rule, names the FFI shim as the existing exception, and names ADR 0016 as a conditional one that is UNUSED today. Three more from the same review: - The negative results carry provenance and status. "Measured neutral" cites its `docs/performance.md` section and is marked MEASURED; the decode cache's 0.29% is marked an INFERRED UPPER BOUND, because no implementation was ever built. - The `// SAFETY:` requirement covers "block OR OPERATION", quoting the repo rule in full. Block alone would leave an `unsafe` operation inside an `unsafe fn` unexplained — the likeliest case here, since intrinsics are usually called from `#[target_feature]` functions, which are `unsafe fn`. - Gate 4 is a named target matrix instead of "a tested fallback". The one that decides the design is `thumbv7em-none-eabihf --no-default-features`: the chip stack must keep building `no_std + alloc` there and it has no SIMD at all, so the scalar path is a first-class implementation a supported target depends on — which is why gate 1 is equivalence rather than conformance of the fast path. * docs(adr): the ADR repeated the invented citation it had just fixed The Amends line was corrected in 8848c84 to stop citing `docs/architecture.md`, which says nothing about `unsafe`. The Context section six lines below still asserted that same file "makes zero `unsafe` in the chip crates a property of the design" — the identical error, in the identical document, surviving the fix aimed at it. `grep -in "unsafe\|forbid" docs/architecture.md` returns nothing. The policy lives in `AGENTS.md`, and it is the crate attributes that enforce it; the Context now says so. The same sentence also claimed "the tree has never had any", which has been false since #241 (12 blocks in the parallel-rdp FFI shim, ADR 0014) and contradicted the AGENTS.md correction this very PR makes. Now split explicitly: the CHIP crates have never had any, the tree has. Also drops PR-process narrative from the Amends line. The load-bearing half — architecture.md must not be cited as a source for this policy — is kept and stated as a checkable fact rather than a story about a draft. * docs(adr): the dispatch-cost claim was asserted, not measured "A design that checks a feature flag per instruction has spent the win before it starts" is a performance claim with no experiment behind it, inside a document whose entire verdict is arithmetic over measured shares. Nothing here has built or timed a dispatch shape. Restated as a named risk with its reasoning: the 5.3% ceiling is spread over ~121,478 VU operations per frame (#250), so the per-operation budget is small enough that a branch plus a non-inlinable call could plausibly consume it. Gate 3's A-B-A is what would settle it. The advice survives because hoisting the check costs nothing to prefer up front — which is the only defensible reason to state a preference before measuring it.
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.
Motivation — scoping B2 before writing any
unsafevu.rsis 143 functions and ~8.5% of a frame, and vectorizing it means writingunsafeintrinsics under a scoped ADR exception. Hand-vectorizing 143 functions to recover the cost of the few that matter would be the expensive way round.So: count first, exactly as the Bus census did — and that one paid for itself immediately by surfacing the
read_u32asymmetry (#249, 1.32%).What this adds
A 64-slot histogram of COP2 computational
functvalues onRsp, behindwork-counters, reported byexamples/work_bench.rs.#[serde(skip)], retired-work tally, default-OFF.The census — Super Mario 64, 120 frames, 14,577,323 COP2 computational ops
That is ~121,478 per frame against 294,983 RSP instructions per frame, so 41% of everything the RSP executes is a VU computation. Only 32 of the 64 possible
functvalues ever appear.0x0eVMADN0x0fVMADH0x0dVMADM0x04VMUDL0x11VSUB0x06VMUDN0x15VSUBC0x32VRCPH/VRSQHclass0x10VADD0x05VMUDM0x33VMOV0x1dVSARFour operations are half the work. Twelve are 81%.
The result that decides the shape of B2
funct 0x00..=0x0F— the whole multiply / multiply-accumulate family — is 61.64%, and it is dispatched by a single function,multiply_lane.Vectorizing one function covers 62% of the VU's computational work. It is also the natural SIMD target on its own merits: eight independent 16×16 lane products accumulating into a 48-bit accumulator is precisely what a vector unit does.
And it bounds the ambition, which matters more
The VU is ~8.5% of a frame. 62% of that is ~5.3%. A perfect vectorization of
multiply_lanecannot exceed it, and the real figure will be lower because the dispatch, the register reads and the accumulator writeback do not vanish.Any SIMD work here gets measured against 5.3%, not 8.5%. Three figures in
docs/performance.mdhave already turned out to belong to configurations that were not being run — the VI's 4.64%, the RDP's 6.36%, and the shared-device plan's "double PCIe crossing". Each was arrived at by reading rather than counting. This is the same trap, and this PR is how it is avoided rather than repeated.One implementation note
[u64; 64]does not implementDefault— the standard impls stop at 32 — so#[serde(skip)]alone does not compile and the field needs an explicitdefault = "zeroed_funct_histogram". That is the compiler catching, at the type level, the same class of mistake #245 shipped at runtime: a skipped field that deserializes into something unusable.Gates run locally
cargo fmt --all --check·cargo clippy --workspace --all-targets -- -D warnings· the same forwork-counterson rsp and frontend ·cargo test --workspace·cargo test -p rustyn64-rsp --features work-counters·RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps·pre-commit run markdownlint --all-files·scripts/check_en_us.sh