fix(rsp): BREAK in a taken branch delay slot halts at the branch target - #43
Merged
Conversation
When a BREAK sits in the delay slot of a *taken* branch, the RSP halts with the PC at the branch target, not the sequential address -- the redirect the branch already latched still wins. `su_step` was discarding the pending `after_delay` target on the halt path and using BREAK's own sequential result, so n64-systemtest's `RSP BREAK (within delay slot)` (beq r0,r0,+6 -> break) stopped at 0x8 instead of 0x1C. The halt path now uses `after_delay.unwrap_or(next)`, so a delay-slot BREAK follows the branch while a plain BREAK, and a BREAK after an *untaken* branch, still halt sequentially. Two regressions cover both directions; the taken case reddens under the old `set_pc(next)`. Suite-wide n64-systemtest failures drop 97 -> 96 (the RSP category is now down to the two RDP DPC-register `start-valid` tests). Phase 1 categories still 0. Gates: fmt, clippy, workspace test, rustdoc, no_std, markdownlint green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe scalar unit now halts at a taken branch’s redirect target when ChangesBREAK delay-slot behaviour
Estimated code review effort: 2 (Simple) | ~10 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: 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 `@docs/rsp.md`:
- Around line 161-166: Add provenance for the BREAK delay-slot behavior
described in docs/rsp.md: either record the measurement, including its source,
in docs/accuracy-ledger.md and link the exact n64-systemtest case from this
passage, or replace it with a citation to the authoritative manual/wiki section
stating the rule. Ensure the SPEC text no longer presents the invariant without
a verifiable reference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8df951d2-e2a2-4ff6-b48c-27da785e0635
📒 Files selected for processing (2)
crates/rustyn64-rsp/src/su.rsdocs/rsp.md
Adopts CodeRabbit's review comment on PR #43. The rule is an oracle-pinned behaviour, not a measured timing constant, so it does not belong in docs/accuracy-ledger.md; instead the SPEC text now names the exact n64-systemtest cases (`RSP BREAK (within delay slot)` and its untaken sibling in src/tests/rsp/op_break.rs) and the `su::tests` regressions that pin both directions. 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
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
Continues Phase 2 (
§v0.3.0, RSP categoryFailed: 0). After the vector ISAand the reserved VZERO family landed (#41, #42), the RSP category had three
non-vector failures left. This fixes one of them —
RSP BREAK (within delay slot).The bug
When a
BREAKexecutes in the delay slot of a taken branch, the RSP haltswith the PC at the branch target, not the sequential address: the redirect the
branch already latched still wins.
su_stepwas dropping the pendingafter_delaytarget on the halt path and usingBREAK's own sequential result,so the suite's
beq r0,r0,+6->breakstopped at0x8instead of0x1C.The fix
The halt path uses
after_delay.unwrap_or(next). A delay-slotBREAKafter ataken branch follows the branch; a plain
BREAK, and aBREAKafter anuntaken branch, still halt sequentially. Two regressions cover both
directions — the taken case reddens under the old
set_pc(next).Result
Suite-wide n64-systemtest failures drop 97 -> 96. The RSP category is now
down to the two
RSP STATUS: start-validtests, which are really RDP DPCregister-file tests (FREEZE bit, START/END/CURRENT, START_VALID state machine) —
substantial RDP work that belongs with the "RDP command list" criterion, tracked
as follow-up. Phase 1 categories remain at 0.
Gates run locally
cargo fmt --all --check,cargo clippy --workspace --all-targets -D warnings,cargo test --workspace,RUSTDOCFLAGS=-D warnings cargo doc --workspace --no-deps, the no_std build, andpre-commit run markdownlint --all-files— allgreen. The n64-systemtest runner confirms the 97->96 drop and Phase 1 at 0.
🤖 Generated with Claude Code