feat(cpu): raise unimplemented operation for subnormals (1,098 → 584) - #29
Merged
Merged
Conversation
n64-systemtest: 1,098 -> 584. `ADD.S`, `SUB.S`, `ADD.D`, `DIV.D`, `ABS.*` and `NEG.*` are at zero failures; the `CVT.W`/`CVT.L` families and `CVT.D.fmt` fell off the list entirely. The VR4300 has no subnormal datapath. Rather than producing a subnormal, or silently flushing one, it raises the unmaskable unimplemented-operation cause (`FCSR.Cause.E`, bit 17) and traps. Four occasions, and they are not interchangeable: a subnormal operand; a subnormal result with `FCSR.FS` clear; a subnormal result with `FS` set but underflow or inexact ENABLED (it cannot deliver a trapped underflow's defined result either -- the suite's own comment on this case reads "(wow)"); and an MSB-clear NaN operand, quiet by this processor's inverted convention (C-12). Only with `FS` set and both enables clear does it flush -- and where it flushes to depends on the rounding mode: +/-0 under nearest and toward-zero, but the smallest NORMAL of that sign under a mode that rounds away from zero, because zero is on the wrong side of the true result. Out-of-range float-to-integer conversions now raise unimplemented rather than the IEEE Invalid that `fpu::to_i32` reports. The translation happens at the call site so the IEEE answer stays available. Three things this surfaced, all now recorded in ledger C-13: - `MOV` is not `ABS`/`NEG`. All three look like sign-or-bit manipulation and only `MOV` is: `ABS`/`NEG` classify their operand, raise Invalid on a signalling NaN, and REPLACE `FCSR.Cause`, while `MOV` transports the bits and leaves `FCSR` alone. The oracle settles it by construction -- `MOV.S` runs through the cause-preserving harness and `ABS.S`/`NEG.S` through the ordinary one. Worth 52 assertions. The earlier finding that `MOV` must not touch `Cause` stands; it simply does not generalise to its neighbours. - Compares are EXEMPT. "This FPU cannot do subnormals" sounds like it should be universal and is not: `C.cond.fmt` compares a subnormal as an ordinary number and raises nothing. Applying the rule there would have regressed all sixteen compare tests, which had just reached zero. - The subnormal-result policy is shared by the arithmetic and by the narrowing `CVT.S.D`, so it lives in one helper rather than being written twice. Seven tests, each mutation-checked: removing the operand check, disabling the result policy, ignoring `FS`, ignoring the rounding mode in the flush, keeping the IEEE Invalid on conversions, and folding `ABS`/`NEG` back into `MOV` each turn a specific test red. Still open here: `CVT.S.D` does not honour `FCSR.RM` and computes its flags by hand (21 assertions) -- the conversions' version of C-11, wanting a narrowing `softfloat::convert`. `SQRT` remains neither decoded nor implemented (58). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesThe VR4300 FPU now classifies subnormals and selected NaNs as unimplemented operations, applies FCSR-dependent flushing, separates MOV from ABS/NEG semantics, and rejects affected conversions. Tests and documentation record the behaviour and updated system-test results. VR4300 FPU behaviour
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant COP1Pipeline
participant FPUHelpers
participant FCSR
COP1Pipeline->>FPUHelpers: classify operands and conversion sources
FPUHelpers-->>COP1Pipeline: return unimplemented-operation decision
COP1Pipeline->>FCSR: replace Cause and raise FloatingPoint exception
FCSR-->>COP1Pipeline: apply trap or flush state
Possibly related PRs
🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR refines the VR4300 COP1 model to correctly raise the unmaskable unimplemented-operation cause (FCSR.Cause.E, bit 17) for subnormal-related cases (and related edge conditions), aligning the emulator’s behavior with n64-systemtest expectations and significantly reducing remaining assertion failures.
Changes:
- Implement unimplemented-operation behavior for subnormal operands/results (including FS + enable interactions) and MSB-clear NaN operands across relevant COP1 ops, with shared helpers to avoid drift.
- Adjust
ABS/NEGhandling to classify operands (unlikeMOV), including correct Cause replacement and trap behavior. - Add targeted regression tests (plus documentation/ledger/status/changelog updates) capturing the rule details and the new systemtest delta (1,098 → 584).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| docs/STATUS.md | Updates COP1 status to reflect that unimplemented-operation handling is now implemented and updates the n64-systemtest failure count. |
| docs/accuracy-ledger.md | Adds ledger entry C-13 documenting subnormal/unimplemented-operation rules and observed effects. |
| crates/rustyn64-cpu/src/pipeline.rs | Implements the new COP1 behavior (subnormal policy, operand checks, ABS/NEG classification) and adds regression tests. |
| crates/rustyn64-cpu/src/fpu.rs | Adds helpers for subnormal detection, “unimplemented NaN” classification, operand gating, and rounding-mode-sensitive flush behavior. |
| CHANGELOG.md | Documents the new unimplemented-operation behavior and the corrected ABS/NEG vs MOV semantics under Unreleased. |
| AGENTS.md | Updates project status notes and adds a new “no subnormal datapath” rule summary referencing ledger C-13. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The new paragraph explaining that ABS and NEG classify their operand was added directly BELOW the old one saying all three raise nothing, so the two sat adjacent and contradicted each other. The stale half is gone and the surviving half keeps the still-true point about `ft` never being read. Found by Copilot on PR #29 -- the same class of finding as the last one, and the reason this repo treats comment and code as independent claims. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![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
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.rs (1)
1504-1510: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winStale doc block:
fp_arithnow produces exactly what this says it does not.The
# Still not handledsection asserts the unimplemented-operation cause "is not produced by the arithmetic here ... which this FPU computes normally instead." This PR routesfp_binary/fp_convert/fp_sign_opinto precisely that cause, so the comment now contradicts the code directly beneath it. Per path instructions, comment and code are independent claims and a comment asserting behaviour the code contradicts is a flagged defect.Suggested removal/rewrite of the stale section
- /// # Still not handled - /// - /// The **unimplemented-operation** cause (bit 17) is unmaskable and is not - /// produced by the arithmetic here — the VR4300 raises it for subnormal - /// operands and results, which this FPU computes normally instead. That is - /// a separate body of work from the maskable enables, and the suite's - /// `expected_unimplemented` cases still fail. + /// # Unimplemented operation + /// + /// The **unimplemented-operation** cause (bit 17) is unmaskable and IS + /// raised here: subnormal operands/results and MSB-clear NaNs route through + /// `fp_binary`/`fp_convert`/`fp_sign_op` to the unimplemented path.🤖 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.rs` around lines 1504 - 1510, Update the stale “Still not handled” documentation above fp_arith to match the current behavior of fp_binary, fp_convert, and fp_sign_op. Remove the claim that unimplemented-operation is not produced by this arithmetic and that expected_unimplemented cases still fail; document the actual routing to that cause instead.Source: Path instructions
🤖 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/accuracy-ledger.md`:
- Around line 700-712: Update C-12’s closing note to mark the MSB-clear NaN
issue resolved, adding a forward reference to C-13 and removing the outdated
“still open” and failing-tests assertions. Preserve the superseded claim in the
append-mostly ledger by flagging the edited-in-place text rather than silently
deleting it.
---
Outside diff comments:
In `@crates/rustyn64-cpu/src/pipeline.rs`:
- Around line 1504-1510: Update the stale “Still not handled” documentation
above fp_arith to match the current behavior of fp_binary, fp_convert, and
fp_sign_op. Remove the claim that unimplemented-operation is not produced by
this arithmetic and that expected_unimplemented cases still fail; document the
actual routing to that cause instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 31c9e8be-a695-4fa4-8642-b7a7b4b943bf
📒 Files selected for processing (6)
AGENTS.mdCHANGELOG.mdcrates/rustyn64-cpu/src/fpu.rscrates/rustyn64-cpu/src/pipeline.rsdocs/STATUS.mddocs/accuracy-ledger.md
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-cpu/src/pipeline.rs`:
- Around line 1531-1540: Update the preceding FPU documentation block near the
arithmetic and conversion handling to remove the stale claim that
unimplemented-operation is not produced and that those cases remain unhandled.
Make the comments reflect that arithmetic and conversion paths now raise
unimplemented-operation, while preserving the accurate behavior described around
the funct 5/6/7 handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6cddb305-3a44-43c4-a8bc-90c5d28674db
📒 Files selected for processing (1)
crates/rustyn64-cpu/src/pipeline.rs
C-12 closed with a 'still open, and adjacent' note about MSB-clear NaN operands raising unimplemented operation. C-13 implements exactly that, so the note was asserting something false -- including that the arithmetic tests still fail on NaN inputs, which they no longer do. Marked resolved with a forward reference rather than rewritten, matching how C-10 was handled and this file's own append-mostly rule: what each entry believed when it was written is the record worth keeping. Found by CodeRabbit, citing the ledger path instruction it was given. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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/accuracy-ledger.md`:
- Line 697: Update the sentence in the accuracy ledger entry by adding a comma
after “written,” while preserving the rest of the wording.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0a98b1fb-dde9-4bcd-9de0-d24070911ec5
📒 Files selected for processing (1)
docs/accuracy-ledger.md
| **Adjacent, and since RESOLVED in C-13:** an **IEEE-signalling / VR4300-quiet** | ||
| NaN operand (MSB clear) to an arithmetic operation raises **unimplemented | ||
| operation** rather than nothing — the VR4300 cannot propagate one in hardware. | ||
| When this entry was written that was still open and the arithmetic tests failed |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the missing comma after “written”.
Use: “When this entry was written, that was still open …”.
🧰 Tools
🪛 LanguageTool
[typographical] ~697-~697: Consider adding a comma here.
Context: ...te one in hardware. When this entry was written that was still open and the arithmetic ...
(IF_THAT_S_NOT_POSSIBLE_COMMA)
🤖 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 `@docs/accuracy-ledger.md` at line 697, Update the sentence in the accuracy
ledger entry by adding a comma after “written,” while preserving the rest of the
wording.
Source: Linters/SAST tools
This was referenced Jul 21, 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
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
After #28, the dominant remaining n64-systemtest block was the unmaskable
unimplemented-operation cause. The VR4300 has no subnormal datapath: rather than producing a
subnormal, or silently flushing one, it raises
FCSR.Cause.E(bit 17) and traps.Result
n64-systemtest: 1,098 → 584.
ADD.S,SUB.S,ADD.D,DIV.D,ABS.*andNEG.*are now at zero failures; theCVT.W/CVT.Lfamilies andCVT.D.fmtfell off the list entirely. What remains is dominated bythe LLE RSP, cart DMA and SP registers — i.e. Phase 2 work rather than COP1.
The rule
Four occasions, and they are not interchangeable:
FCSR.FSclearFSset and underflow or inexact enabledFSset, both enables clearThe third row is the interaction of two features that each work on their own: flushing works, and
enabled traps work, but together they do not — the processor cannot deliver a trapped underflow's
defined result. n64-systemtest's own comment on this case reads "(wow)".
Where it flushes to depends on the rounding mode:
±0under nearest and toward-zero, but thesmallest normal of that sign under a mode that rounds away from zero, because zero is on the
wrong side of the true result. Getting that wrong gives
-0in all four cases, which looksentirely reasonable.
Out-of-range float-to-integer conversions now raise unimplemented rather than the IEEE Invalid
that
fpu::to_i32reports. The translation happens at the call site, so the IEEE answer staysavailable to anything that wants it.
Three things this surfaced
MOVis notABS/NEG. All three look like sign-or-bit manipulation and onlyMOVis:ABS/NEGclassify their operand, raise Invalid on a signalling NaN, and replaceFCSR.Cause, whileMOVtransports the bits and leavesFCSRalone. The oracle settles this byconstruction rather than description —
MOV.Sis driven throughtest_floating_point_f32_which_preserves_cause_bits,ABS.S/NEG.Sthrough the ordinary harnessthat asserts
Causewas cleared. Worth 52 assertions. The earlier finding thatMOVmust nottouch
Cause(ledger C-10) stands; it simply does not generalise to its neighbours.Compares are exempt. "This FPU cannot do subnormals" sounds like it should be universal and is
not:
C.cond.fmtcompares a subnormal as an ordinary number and raises nothing. Applying the rulethere would have regressed all sixteen compare tests, which had just reached zero in #28.
The subnormal-result policy is shared by the arithmetic and the narrowing
CVT.S.D, so itlives in one helper rather than being written twice and drifting.
Testing
355 passing. Seven new tests, each mutation-checked — removing the operand check, disabling
the result policy, ignoring
FS, ignoring the rounding mode in the flush, keeping the IEEEInvalid on conversions, and folding
ABS/NEGback intoMOVeach turn a specific test red.The subnormal-result test deliberately uses two normal operands, so it exercises the result
path and not the operand one — a test using a subnormal input would pass with the result check
deleted.
Not in scope
Phase 1 is not complete and v0.2.0 is not cut.
CVT.S.Ddoes not honourFCSR.RMand computes its flags by hand rather than throughsoftfloat(21 assertions). This is the conversions' version of ledger C-11 and wants the samefix: a narrowing
softfloat::convertthat rounds once.SQRT(funct 4) remains neither decoded nor implemented (58 assertions).