test(harness): assert the RSP criterion, which nothing did - #238
Merged
Conversation
Phase 2's cut criterion is `Failed: 0` in n64-systemtest's RSP categories. It was verified by hand at v0.3.0 and NEVER ASSERTED: `phase_1_categories_report_no_ failures` excludes "RSP" and "SP " through its LATER_PHASES list, so it stays green through an arbitrarily broken vector unit. docs/STATUS.md's own evidence column said so out loud — "dump per-test to confirm none are RSP-prefixed" — and nobody noticed that a manual dump is not a gate. Found while sizing a vector-unit refactor: I cited that suite as the refactor's gate, and a reviewer checked. It is not one. `rsp_categories_report_no_failures` is the same shape as the Phase 1 assertion and for the same reasons: output exists, the suite reached xioctl(EXIT), and — the part that matters — the RSP category ITSELF started. That last witness is not redundant with the suite-wide count: a change that stopped RSP tests starting would leave zero RSP failures, which is the vacuous pass this project keeps re-encountering. BASELINE: 0 failing across 224 RSP tests started, suite to EXIT, 90 suite-wide across 950. MUTATION-CHECKED, and the result is the argument for the gate existing: making VAND compute VOR (`0x28 => s | t`) fails `rsp_categories_report_no_failures` while `phase_1_categories_report_no_failures` STILL REPORTS 0 FAILING. The old gate cannot see a broken VU; the new one can. Restored afterwards and the tree verified against the snapshot. The runner's five-tuple return became a `Report` struct on the way — it was gaining a sixth field, at which point every call site is a positional puzzle and a swapped pair compiles. docs/STATUS.md's Phase 2 row and docs/testing-strategy.md's oracle table now name the automated assertion instead of the manual procedure, and say why there are two tests rather than one. Gates: fmt, clippy, test --workspace, rustdoc, en-US, markdownlint, and both systemtest assertions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 2 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 system-test harness now separates RSP/SP results from Phase 1 results. It returns a structured ChangesRSP test reporting
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 10✅ Passed checks (10 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: 1
🤖 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-test-harness/tests/systemtest.rs`:
- Around line 65-67: Update the rustdoc immediately above the run function to
document that it returns a Report, removing the stale tuple description and
mentioning the Report return type directly. Keep the run signature and
implementation unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 012b3993-19bd-49e2-a959-96f17bca01c2
📒 Files selected for processing (3)
crates/rustyn64-test-harness/tests/systemtest.rsdocs/STATUS.mddocs/testing-strategy.md
…eturns Changing the return type to `Report` left its doc comment describing five positional values, omitting the RSP failures and the rsp_started witness entirely. A comment that asserts what the code does and disagrees with it is the exact failure mode this repo tracks, and I introduced one in the same commit that added the struct. Rewritten to point at the struct's own field docs, with the reason for the struct (a sixth field was arriving, and a swapped pair of positionals compiles) and the reason one run serves both assertions (~25 s in --release; two runs would double it for no added coverage). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Antigravity review (Gemini via Ultra)This pull request introduces the Blocking issues
Suggestions
Nitpicks
Automated first-pass review by |
doublegate
added a commit
that referenced
this pull request
Jul 31, 2026
…al (#239) * docs(perf): the VU family hoist — built, accurate, and measured NEUTRAL 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> * docs(perf): bound the hoist conclusion to what one family measured 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> --------- Co-authored-by: Claude Opus 5 <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
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.
Why this exists
I was asked for the single-family VU hoist experiment. #237 established — via review — that there is no gate for it to run against, so this is the prerequisite: an RSP-scoped n64-systemtest assertion.
Phase 2's cut criterion is
Failed: 0in the RSP categories. It was verified by hand at v0.3.0 and never asserted.phase_1_categories_report_no_failuresexcludes"RSP"and"SP "through itsLATER_PHASESlist, so it stays green through an arbitrarily broken vector unit.docs/STATUS.md's own evidence column said so out loud — "dump per-test to confirm none areRSP-prefixed" — and nobody noticed that a manual dump is not a gate. I then cited that suite as a refactor's gate, and a reviewer checked.Baseline
The mutation check is the argument for the gate
Making
VANDcomputeVOR(0x28 => s | t):rsp_categories_report_no_failuresphase_1_categories_report_no_failuresThe old gate cannot see a broken VU; the new one can. That is not a hypothetical about coverage — it is the same suite, the same run, one defect, and two different verdicts. Restored afterwards, tree verified against the snapshot.
Shape
Same as the Phase 1 assertion and for the same reasons: output exists, the suite reached
xioctl(EXIT), and — the part that matters — the RSP category itself started. That witness is not redundant with the suite-wide count: a change that stopped RSP tests starting would leave zero RSP failures, which is the vacuous-pass shape this project keeps re-encountering. The threshold is> 20against an observed 224, loose on purpose — it exists to catch a category that did not run, not to pin the suite's size.RSP_CATEGORIESis deliberately a narrower list thanLATER_PHASES:"RSP"and"SP "only. The RDP, MI, VI, AI, PI, SI and cart entries belong to other subsystems and are not asserted here.The runner's five-tuple return became a
Reportstruct on the way — it was gaining a sixth field, at which point every call site is a positional puzzle and a swapped pair compiles.Docs
docs/STATUS.md's Phase 2 row anddocs/testing-strategy.md's oracle table now name the automated assertion instead of the manual procedure, and say why there are two tests rather than one.Verification
One guarded conditional:
cargo fmt --all --check;cargo clippy --workspace --all-targets -- -D warnings;cargo test --workspace;RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps;scripts/check_en_us.sh;pre-commit run markdownlint --all-files; and both systemtest assertions green.Next
With the gate in place, the single-family hoist experiment can run against it. That is the following PR, not this one.
🤖 Generated with Claude Code