docs(perf): the VU family hoist — built, accurate, and measured neutral - #239
Merged
Conversation
The experiment the SIMD section asked for, run against the RSP gate added in #238. It reverts under this document's own rule, and what it settles is worth more than the change would have been. WHICH FAMILY, MEASURED RATHER THAN GUESSED. An opcode histogram in vu_compute over 900 frames of Super Mario 64: 125 M VU compute ops, ~139 k per frame. The MULTIPLY FAMILY (0x00..=0x0F) is ~61% of them — 0x0e alone is 17.8%, 0x0f 15.2%. It is also the only family that dispatched TWICE per lane (outer match, then multiply_lane's own), so the opcode was decoded sixteen times per instruction. If dispatch-in-the-loop were the constraint, that is where it would show. THE CHANGE WAS CORRECT. A dedicated eight-lane loop for op <= 0x0F, returning directly since the multiplies write no VCC, no VCO, and fall outside the 0x28..=0x2D accumulator range. Accuracy held on the new gate: RSP 0 failing across 224 tests started, suite-wide unchanged at 90. AND IT IS WORTH NOTHING. Six readings a side, one sitting: A 64.114 64.303 64.183 64.110 63.744 63.899 (mean 64.059) B 63.840 64.182 64.040 63.473 63.775 63.517 (mean 63.804) The ranges overlap heavily. Means differ by 0.40% in the hoist's favour while the conservative pairing says it is 0.68% SLOWER. When the sign depends on which pairing you quote, the result is neutral — the same standard applied to the rsp_tick early-out, and the same conclusion. Reverted per the standing rule. WHAT IT SETTLES. This was the strongest available test of "the per-lane dispatch is what stops the VU going fast", on the family that is 61% of the work and carries double the dispatch. It moved nothing. With the decode sizing (0.29% for a perfect cache), the VU's 8.54% of a frame is THE ARITHMETIC ITSELF, not the dispatch around it. No amount of restructuring the interpreter reaches it. What remains is genuine SIMD — eight u16 lanes per operation, which is what the hardware is — or nothing. That makes the unsafe/nightly decision the real gate on further VU work rather than something to defer behind a restructure. This measurement does not make that call; it removes the cheaper alternative that was standing in front of it. Recorded under Ruled out: do not hoist VU opcode families for performance. The remaining families are smaller and singly-dispatched, so they can only measure smaller. All instrumentation was scratch: vu.rs snapshotted, restored, tree verified clean against the snapshot between every leg. Gates: check_en_us.sh, markdownlint. Docs-only — the hoist itself is not landed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 3 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 (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe performance documentation records a VU multiply-family dispatch-hoisting experiment, RSP accuracy results, A-B-A timing measurements, and the decision to revert the neutral optimisation. ChangesVU multiply-family hoist
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
🚥 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: 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/performance.md`:
- Around line 1653-1677: Revise the “What it settles” and “Ruled out”
conclusions to cover only the measured multiply-family dispatch hoist for this
workload. Remove definitive claims that interpreter restructuring cannot help,
SIMD is the only improvement, or other opcode families can only have smaller
effects; label those broader points as unverified hypotheses requiring separate
measurements.
- Around line 1629-1647: Complete the VU-hoist benchmark documentation around
the displayed A/B measurements by recording the actual A–B–A run order, host,
operating system, toolchain, revision, and exact command, or link to an existing
source containing all of that provenance. Preserve the current measurements and
conclusion while satisfying the §Measured requirement for a reproducible method.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 292e3e7d-ce21-49bd-ab74-aa926c004b8d
📒 Files selected for processing (1)
docs/performance.md
Two review findings. I OVER-GENERALISED FROM ONE FAMILY, AGAIN. The section claimed that no interpreter restructuring reaches the VU, that SIMD or nothing is what remains, and that the other families can only measure smaller. None of that was measured. One family was hoisted on one workload. Split into what IS established — hoisting the multiply family's dispatch is neutral on SM64's render phase, and with the 0.29% decode sizing that is two of the three things a pre-decoded threaded interpreter would do — and what is HYPOTHESIS, each labelled and each needing its own measurement: that the 8.54% is predominantly arithmetic (neither experiment decomposed the per-lane body); that no restructuring helps (a full per-opcode specialisation, which also hoists multiply_lane's inner match, is a DIFFERENT change); that smaller families measure smaller (a reason to expect it, not a measurement); and that SIMD is what remains (obvious candidate, untried). The Ruled-out entry now rules out REPEATING THIS EXPERIMENT rather than the different change it was being read to cover. What survives is narrower and still useful: the cheapest candidate is gone from the front of the queue, so the unsafe/nightly decision is no longer deferrable behind "restructure first, it is free" — the free restructure was tried on its best case and did nothing. PROVENANCE ADDED for the benchmark: the actual A-B-A order (A x3 -> B x3 -> B x3 -> A x3, the B blocks contiguous because the rebuild is what costs time), the revision, and the exact command; host, OS, toolchain, profile and ROM hash are the ones recorded once in section Method rather than duplicated. Gates: check_en_us.sh, markdownlint. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Antigravity review (Gemini via Ultra)This documentation-only PR records opcode analysis and benchmark results for a vector unit (VU) multiply opcode dispatch hoisting experiment, documenting that the change yielded neutral performance results in Super Mario 64 and justifying its reversion. Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
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.
Result: built, accurate, neutral — reverted
The single-family hoist experiment, run against the RSP gate #238 added for it. Docs-only; the hoist itself is not landed.
Which family — measured, not guessed
An opcode histogram in
Rsp::vu_computeover 900 frames of Super Mario 64: 125 M VU compute ops, ~139 k per frame.0x0e0x0f0x0d0x10VADD0x040x00The multiply family (
0x00..=0x0F) is ~61% of all VU compute ops — and it is the only family that dispatched twice per lane (outer match, thenmultiply_lane's own), so the opcode was decoded sixteen times per instruction. If dispatch-in-the-loop were the constraint, this is where it would show.The change was correct
A dedicated eight-lane loop for
op <= 0x0F, returning directly — the multiplies write noVCC, noVCO, and fall outside the0x28..=0x2Daccumulator range, so none of the shared tail applies.Accuracy held, on the gate built for exactly this:
And it is worth nothing
Six readings a side,
frame_bench, one sitting:Ranges: A
63.744–64.303, B63.473–64.182— heavy overlap. The means differ by 0.40% in the hoist's favor; the conservative pairing (best A against worst B) says it is 0.68% slower.When the sign of the result depends on which pairing you quote, the result is neutral. Same standard as the
rsp_tickearly-out, same conclusion. Reverted per this document's standing rule — revert anything neutral or worse, as was done for thenext_edgehoist, thevi_divotreorder,target-cpu=nativeand PGO.What it settles — the reason this was worth doing
This was the strongest available test of "the per-lane dispatch is what stops the VU going fast": run on the family that is 61% of the work and carries double the dispatch. It moved nothing.
Taken with the decode sizing (0.29% for a perfect decode cache):
What remains is genuine SIMD — eight
u16lanes per operation, which is what the hardware is — or nothing.That makes the
unsafe/nightly decision the real gate on further VU work, rather than something to defer behind a restructure. This measurement doesn't make that call; it removes the cheaper alternative that was standing in front of it.Ruled out
Do not hoist VU opcode families out of the lane loop for performance. Measured neutral on the largest family, with accuracy verified. The remaining families are smaller and singly-dispatched, so they can only measure smaller.
Verification
bash scripts/check_en_us.sh;pre-commit run markdownlint --all-files. All instrumentation was scratch —vu.rssnapshotted, restored, and the tree verified clean against the snapshot between every leg.🤖 Generated with Claude Code