feat(cpu): decode COP1 compares and conversions, fix NaN convention - #28
Merged
Conversation
…NaN convention n64-systemtest: 2,682 -> 1,098. All sixteen `C.cond.fmt` tests now pass outright. Two changes, measured separately. **The compares and conversions (2,682 -> 1,468).** `C.cond.fmt`, the `CVT` family and `ROUND`/`TRUNC`/`CEIL`/`FLOOR` to `.W`/`.L` were implemented in `fpu.rs` all along but unreachable: decode admitted only `funct 0..=3` and `5..=7`, and never admitted the INTEGER source formats `.W`/`.L` at all, so every integer-to-float conversion was a silent no-op too. The same shape of gap as `MOV.fmt`, found by asking which neighbouring encodings shared the range. `ROUND`/`TRUNC`/`CEIL`/`FLOOR` take their rounding mode from the OPCODE and ignore `FCSR.RM`; `CVT.W`/`CVT.L` consult it. That is the entire difference between the two families, and it is invisible whenever `RM` happens to agree -- so the test sets `RM` to nearest and converts `-1.5`, where the two disagree. `fp_arith` is restructured around one commit-or-trap point: each family returns an `FpCommit` plus flags, and the trap check, the `Cause`-only write and the non-retirement all happen once rather than per family. **The NaN convention (1,468 -> 1,098).** The VR4300 classifies a NaN as signalling when the significand's MSB is SET -- the legacy MIPS convention, inverted from IEEE-754:2008. `0x7FC0_0000`, which Rust produces as `f32::NAN` and everything else calls quiet, raises Invalid here. Established from the oracle's own expectations, which name their constants the IEEE way and then assert the opposite: for a non-signalling compare it expects MSB-set to raise Invalid and MSB-clear to raise nothing. The signalling compare forms raise for both and so do not distinguish the conventions -- checking only those would have left it open. The corroboration that makes it more than a curve fit: the VR4300's own default NaN result is `0x7FBF_FFFF`, MSB clear. Read as IEEE that is a processor whose invalid-operation result is a signalling NaN, which would re-trap on first use. Read under this convention it is an ordinary quiet one. Two independent facts agreeing on the same inversion. Accuracy ledger C-12. Five existing tests asserted the IEEE convention and were updated; one now asserts `is_snan_f32(f32::NAN)` on purpose, because that is the case most likely to be "fixed" back by someone who has not read the ledger entry. All three guards mutation-checked. The decode arm initially had NO test -- reverting it broke nothing -- which is exactly the decoded-but-no-op blind spot AGENTS.md now warns about; the enumerated decode test and two execution tests were added until the revert goes red. Also adds `.coderabbit.yaml`, tuned to this project's decided rules rather than generic Rust style, so a second review bot does not spend its comments on things clippy already gates or flag deliberate deviations as defects. 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
WalkthroughCOP1 compare and conversion instructions now decode and execute through a refactored floating-point commit and trap flow. VR4300 NaN classification is updated across arithmetic and soft-float paths, with tests and project records revised. CodeRabbit review settings are added. ChangesCOP1 execution updates
Review configuration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant COP1Instruction
participant Decoder
participant fp_arith
participant FCSRAndFPR
COP1Instruction->>Decoder: decode funct and format
Decoder->>fp_arith: dispatch Op::FpArith
fp_arith->>FCSRAndFPR: commit FPR result or FCSR.C
fp_arith->>FCSRAndFPR: write Cause on floating-point trap
🚥 Pre-merge checks | ✅ 8 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (8 passed)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR wires up previously-implemented COP1 compare (C.cond.fmt) and conversion/rounding operations in the decoder/execute path, and corrects NaN signalling classification to match the VR4300’s legacy MIPS convention (significand MSB set ⇒ signalling). It reduces n64-systemtest failures from 2,682 → 1,098 and updates project docs/changelog to reflect the new accuracy milestone.
Changes:
- Expanded COP1 decode/execute to cover compares, conversions, and fixed-mode integer conversions (keeping
SQRTintentionally undecoded). - Updated NaN signalling detection (and related tests/docs) to the VR4300-inverted convention (ledger C-12).
- Updated status/ledger/changelog/agent guidance and added CodeRabbit configuration to align review automation with repo decisions.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/STATUS.md | Updates COP1 “partial” status and the current n64-systemtest failing count/next blocker. |
| docs/accuracy-ledger.md | Adds ledger entry C-12 documenting the inverted VR4300 NaN convention and its impact. |
| crates/rustyn64-cpu/src/softfloat.rs | Updates NaN signalling classification in unpacking logic and adjusts tests/comments accordingly. |
| crates/rustyn64-cpu/src/pipeline.rs | Refactors COP1 execution around an FpCommit to unify commit-or-trap handling and adds compare/conversion support. |
| crates/rustyn64-cpu/src/fpu.rs | Implements VR4300 NaN signalling classification and adds/updates corroborating tests. |
| crates/rustyn64-cpu/src/decode.rs | Broadens COP1 decode coverage (incl. integer source formats) and adds enumerated decode tests to prevent gaps. |
| CHANGELOG.md | Records the new COP1 decode coverage and NaN convention fix with updated n64-systemtest deltas. |
| AGENTS.md | Updates project guidance to include the inverted NaN convention and the new failure count/state. |
| .coderabbit.yaml | Adds repo-specific CodeRabbit review instructions emphasizing ADR/ledger consistency and known deliberate deviations. |
Comments suppressed due to low confidence (1)
docs/STATUS.md:131
- This paragraph still says the conversions and
C.cond.fmtcompares are “not yet decoded” and “unreachable”, which contradicts the updated COP1 status just above (and the PR’s stated result). It should be updated/removed sodocs/STATUS.mdremains the single source of truth.
`SQRT` (funct 4), the conversions and the `C.cond.fmt` compares are implemented
in `fpu.rs` but **not yet decoded**, so they remain unreachable. `ABS`, `MOV`
and `NEG` were in that list until they were found to be the cause of ~100
failures — a *decoded-but-no-op* instruction is invisible to `cargo test`, and
`MOV` in particular is emitted by the compiler for every FP call boundary.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…leared `CAUSE_MASK` covered only bits 16:12. Bit 17, `Unimplemented Operation`, is part of the `Cause` field even though it is not an IEEE exception and so has no `Enable` bit and no sticky `Flags` twin -- which means that mask is the ONLY thing that can ever clear it. Once raised it stayed set forever, and software reading `FCSR` after a perfectly successful conversion would still see the previous failure. Found by Copilot on PR #28. The suite could not have caught it: no test raised bit 17 and then ran another COP1 instruction. There is one now, mutation-checked against reverting the mask. Also adopts Copilot's second comment: the C-11 paragraph naming the undecoded funct space as "the dominant remaining block" is now false, and is rewritten in explicit past tense rather than back-edited. A ledger read top to bottom should show what was believed when each entry was written. Rewrites `.coderabbit.yaml` against the published schema (schema.v2.json) rather than from memory. All the original keys were valid but the file was thin: - `tools`: clippy and markdownlint OFF, because this repo already gates both harder than a bot will (pedantic + nursery at `-D warnings`, and a pinned markdownlint pre-commit hook). Leaving them on spends review comments on findings CI has already blocked. actionlint, yamllint, shellcheck and gitleaks stay on -- they cover ground no local gate does. - `finishing_touches`: generated docstrings and unit tests OFF. rustdoc is a blocking gate and every test here carries a rationale comment saying what it would catch, so generated stand-ins would have to be rewritten. - `pre_merge_checks`: Conventional Commits title, plus two custom checks -- that a behaviour change states its measured n64-systemtest delta, and that a chip change touches that chip's doc. - New path instructions for tests (flagging convergent success/failure paths) and for workflows (flagging a gate piped into tail/grep, which has hidden three real failures here). - `knowledge_base.code_guidelines` pointed at AGENTS.md, the accuracy ledger and engineering-lessons, with learnings scoped local. 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: 4
🤖 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 @.coderabbit.yaml:
- Around line 66-71: Expand the Markdown lint configuration in the
`.coderabbit.yaml` review instructions so the mandatory check applies to every
Markdown file, not only `docs/**/*.md`. Update the existing Markdown path rule
or add the appropriate repository-wide Markdown pattern, while preserving the
current documentation-specific guidance for files under `docs/`.
- Line 31: Update the ROM ignore pattern in the CodeRabbit configuration to
exclude only tests/roms/external/**, allowing the committed
tests/roms/n64-systemtest/ corpus to remain visible during review.
In `@docs/accuracy-ledger.md`:
- Around line 689-693: Correct the NaN classification in the “Still open, and
adjacent” entry: describe an MSB-clear NaN as IEEE-signalling and VR4300-quiet
under the documented inverted convention, while preserving the stated
unimplemented-operation behavior and its connection to the arithmetic tests.
In `@docs/STATUS.md`:
- Around line 105-125: Remove or update the stale COP1 status paragraph near the
referenced section that claims compares and conversions are not decoded or
reachable. Ensure docs/STATUS.md consistently states that compares and
conversions decode and execute, while preserving the current blocker
descriptions for unimplemented-operation causes and SQRT.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fb82f1cc-054b-4a7c-9509-c22e9687861c
📒 Files selected for processing (9)
.coderabbit.yamlAGENTS.mdCHANGELOG.mdcrates/rustyn64-cpu/src/decode.rscrates/rustyn64-cpu/src/fpu.rscrates/rustyn64-cpu/src/pipeline.rscrates/rustyn64-cpu/src/softfloat.rsdocs/STATUS.mddocs/accuracy-ledger.md
Owner
Author
|
@coderabbitai review Re-requesting after 3be3c85, which rewrote |
|
✅ Action performedReview finished.
|
…ertive All four were correct. - `.coderabbit.yaml` hid `tests/roms/**` from review, which over-reached: the `n64-systemtest/` corpus is committed (MIT, with its upstream LICENSE beside it) and `tests/roms/README.md` carries the licence-tiering rules. Narrowed to `!tests/roms/external/**`, the gitignored tier that was actually meant. - The markdownlint instruction covered only `docs/**/*.md`, leaving AGENTS.md and CHANGELOG.md -- edited on nearly every PR -- outside the one rule that mentions it. It is now a `**/*.md` entry, and says why it matters: markdownlint has no CI job here, so it is the single gate that can silently not run. - Ledger C-12 described the still-open case as an "IEEE-quiet NaN operand (MSB clear)", which is self-contradictory: under IEEE-754:2008 MSB *set* is quiet. The oracle settles it -- the ADD.S case expecting unimplemented-operation uses `SIGNALLING_NAN_START_64` (MSB clear) -- so it is IEEE-signalling and VR4300-quiet. Both readings are now named at every mention, here and in docs/STATUS.md, since C-12 exists precisely because they disagree. - docs/STATUS.md contradicted itself: a trailing paragraph still claimed the compares and conversions were undecoded. Rewritten to name only `SQRT`, and to keep the rule that paragraph produced -- when adding a decode arm, enumerate the neighbouring funct space rather than only the encoding that prompted it. Profile raised from `chill` to `assertive`, the most feedback CodeRabbit offers. Its docs warn that may feel nitpicky; that is the right trade here, because a missed defect on this project is measured in weeks of misdirected investigation (ledger C-10), every comment is adjudicated individually rather than skimmed, and the path instructions already list the deliberate deviations not to report -- which is what makes assertive affordable rather than noisy. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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)
1698-1764: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThread
FCSR.RMinto the inexact CVT helpers.
CVT.S.D,CVT.S.W,CVT.S.L, andCVT.D.Lstill go through mode-blind casts, so directed rounding is lost for non-exact conversions. That violatesdocs/accuracy-ledger.mdU-8 and the FPU rounding contract incrates/rustyn64-cpu/src/fpu.rs.CVT.D.SandCVT.D.Ware exact and fine; add a regression under a non-default RM.🤖 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 1698 - 1764, Update fp_convert so the inexact conversions CVT.S.D, CVT.S.W, CVT.S.L, and CVT.D.L receive and honor the FCSR.RM mode through the corresponding FPU conversion helpers, rather than using mode-blind casts. Leave the exact CVT.D.S and CVT.D.W paths unchanged, preserve existing flag and trap handling, and add a regression covering non-default rounding.Source: Coding guidelines
🧹 Nitpick comments (1)
.coderabbit.yaml (1)
172-185: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude
docs/STATUS.mdin local review guidance.The configured checks rely on status/count accuracy but omit the declared source of truth from
filePatterns.Proposed fix
filePatterns: - "AGENTS.md" - "docs/accuracy-ledger.md" - "docs/engineering-lessons.md" + - "docs/STATUS.md" - "CONTRIBUTING.md"As per coding guidelines,
docs/STATUS.mdmust be read before work and is the single source of truth for counts and state.🤖 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 @.coderabbit.yaml around lines 172 - 185, Update the code_guidelines.filePatterns configuration to include docs/STATUS.md alongside the existing guidance files, ensuring local reviews read the documented source of truth for counts and state.Source: Coding guidelines
🤖 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 1505-1518: Update the earlier fp_arith documentation describing
the FCSR Cause field to include bits 17:12 instead of 16:12, explicitly
identifying Cause.E (Unimplemented Operation) at bit 17. Keep the documentation
consistent with the CAUSE_MASK definition and its stale-bit-clearing behavior.
---
Outside diff comments:
In `@crates/rustyn64-cpu/src/pipeline.rs`:
- Around line 1698-1764: Update fp_convert so the inexact conversions CVT.S.D,
CVT.S.W, CVT.S.L, and CVT.D.L receive and honor the FCSR.RM mode through the
corresponding FPU conversion helpers, rather than using mode-blind casts. Leave
the exact CVT.D.S and CVT.D.W paths unchanged, preserve existing flag and trap
handling, and add a regression covering non-default rounding.
---
Nitpick comments:
In @.coderabbit.yaml:
- Around line 172-185: Update the code_guidelines.filePatterns configuration to
include docs/STATUS.md alongside the existing guidance files, ensuring local
reviews read the documented source of truth for counts and state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e0a38df4-08ac-4aec-9eb3-944b9d0748dd
📒 Files selected for processing (3)
.coderabbit.yamlcrates/rustyn64-cpu/src/pipeline.rsdocs/accuracy-ledger.md
… copied RustyNES's `.coderabbit.yaml` is far more complete than this one was, and comparing them surfaced a dozen valid keys this file never set. All were re-verified against schema.v2.json rather than trusted from the sibling repo. Added: sequence_diagrams, estimate_code_review_effort, changed_files_summary, related_issues/related_prs, suggested_labels/reviewers (with the auto-apply counterparts explicitly OFF -- single-maintainer repo), slop_detection, auto_review.ignore_title_keywords, the full finishing_touches block, three more pre_merge_checks, `!Cargo.lock` in path_filters, knowledge_base.web_search, and an explicit 49-entry tools list. Where this DELIBERATELY differs from RustyNES, and why: - `markdownlint` ON here, OFF there in spirit. This repo has no markdownlint CI job at all -- it is pre-commit only, so it silently does not run for anyone without the hook. That makes it the one linter CodeRabbit ADDS rather than duplicates. - `clippy` OFF. CI runs it at pedantic + nursery with `-D warnings`, a strict superset of default clippy, so the tool could only repeat findings that already block the merge or contradict a lint the workspace allows. - `opengrep` OFF as a semgrep fork, on the same duplicate-findings reasoning RustyNES applies to pylint/flake8. - Tool list rebuilt from THIS repo's footprint (Rust, Markdown, TOML, YAML, shell, one Python file). RustyNES needs detekt/swiftlint/luacheck/clang for its Kotlin, Swift, Lua and C; this project has none of those. - `finishing_touches` fully off, including autofix and fix_ci. Every change here goes through one conditional gate and every guard is mutation-checked before it is kept; a bot-authored commit bypasses both. - `drafts: true` (RustyNES has false). Branches here run long -- PR #27 reached 49 commits -- and the expensive mistakes are the ones caught before the branch is finished. `ignore_title_keywords` is the escape hatch. - `learnings.scope: local` rather than auto. This is a public repo and the conventions learned here -- notably the inverted NaN classification -- are correct for the VR4300 and wrong almost everywhere else. Path instructions gained per-crate entries for the CPU (reverse-cascade latch reads), core (the one permitted chip-to-chip edge), the test harness (an oracle that runs nothing looks exactly like one that passes) and scripts (two of them are commit gates that block committing a commercial ROM). Note for the sibling repo: RustyNES's config still states the markdownlint hook is pinned to v0.39.0. Both repos pin v0.49.1; the same stale claim was corrected here earlier in this branch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
doublegate
changed the title
…7:12 The comment and the code disagreed in exactly the way this project's own convention warns about -- and this pair had already produced the stale-bit defect fixed in 3be3c85, where CAUSE_MASK matched the wrong comment rather than the architecture. Now states that Cause is 17:12, that bit 17 is Cause.E with no Enable bit and no sticky Flags twin, and that the narrower 16:12 range used by the enable comparison is a different statement about the five *maskable* conditions. Found by CodeRabbit under the assertive profile, citing the path instruction that comments disagreeing with the implementation must be flagged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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 @.coderabbit.yaml:
- Around line 424-431: Update the CodeRabbit configuration for the “unsafe stays
out of the chip crates” check so its enforcement matches the intended policy:
enable reviews.request_changes_workflow for blocking error checks, or change
this check’s mode from error to warning if it should remain non-blocking.
In `@docs/STATUS.md`:
- Around line 128-134: Update the SQRT status statement in the documentation to
say that SQRT (funct 4) is both undecoded and unimplemented, aligning it with
the implementation status described in Lines 115–117. Leave the surrounding
history and neighboring instruction details unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a2e228a3-8364-461a-9e3b-7aca4e750316
📒 Files selected for processing (3)
.coderabbit.yamldocs/STATUS.mddocs/accuracy-ledger.md
Two CodeRabbit findings, both correct. STATUS.md called SQRT "implemented-but-undecoded" eleven lines after saying it has no implementation. It is neither, so it is not an instance of the decoded-but-no-op pattern at all -- the conversions and compares were, until this sprint. The `unsafe` pre-merge check was set to `error` mode, which only blocks when `request_changes_workflow` is enabled; it is not, so the mode claimed a gate that could never fire. Downgraded to `warning`, which is also the honest level: `#![forbid(unsafe_code)]` makes this a COMPILE error in every chip crate, and this config's stated principle is not to duplicate a gate the repo already runs harder. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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)
1717-1758: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPass
FCSR.RMinto theCVT.S.*/CVT.D.Lpaths.pipeline.rsstill calls mode-blind helpers here, so directed rounding is ignored whenever the conversion is inexact, contrary to UM §7.2.4 and accuracy-ledger C-11. Add mode-aware helpers and regression cases forCVT.S.D,CVT.S.W,CVT.S.L, andCVT.D.Lunder all four rounding modes.🤖 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 1717 - 1758, Update the conversion handling in the `0o40` and `0o41` branches to pass the current `FCSR.RM` into mode-aware FPU helpers for `CVT.S.D`, `CVT.S.W`, `CVT.S.L`, and `CVT.D.L`, preserving the existing unimplemented-trap behavior for restricted long inputs. Add regression coverage for each conversion under all four rounding modes, including inexact results.Source: MCP tools
🤖 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 1477-1480: In the documentation comment near Cop1Control::ctc1 and
CAUSE_MASK, narrow the bit 17 clearing claim to state that the mask is the only
automatic clear in this arithmetic path. Preserve the existing explanation of
bit 17 and its lack of Enable and sticky Flags counterparts.
---
Outside diff comments:
In `@crates/rustyn64-cpu/src/pipeline.rs`:
- Around line 1717-1758: Update the conversion handling in the `0o40` and `0o41`
branches to pass the current `FCSR.RM` into mode-aware FPU helpers for
`CVT.S.D`, `CVT.S.W`, `CVT.S.L`, and `CVT.D.L`, preserving the existing
unimplemented-trap behavior for restricted long inputs. Add regression coverage
for each conversion under all four rounding modes, including inexact results.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cccb6091-7413-468f-93b5-760250d10e2a
📒 Files selected for processing (3)
.coderabbit.yamlcrates/rustyn64-cpu/src/pipeline.rsdocs/STATUS.md
Comment on lines
+1477
to
+1480
| /// **The field is 17:12, not 16:12.** Bit 17 is `Cause.E`, Unimplemented | ||
| /// Operation — part of `Cause` despite having no `Enable` bit and no sticky | ||
| /// `Flags` twin, which means the mask is the *only* thing that ever clears | ||
| /// it. This comment said 16:12 while `CAUSE_MASK` covered 16:12 too, and |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Narrow the “only clears it” claim.
Cop1Control::ctc1 also lets software clear writable Cause bits, so CAUSE_MASK is not the only mechanism that can clear bit 17. Change “the mask is the only thing that ever clears it” to “the only automatic clear in this arithmetic path”. This follows the FCSR contract in crates/rustyn64-cpu/src/cop1.rs.
🤖 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 1477 - 1480, In the
documentation comment near Cop1Control::ctc1 and CAUSE_MASK, narrow the bit 17
clearing claim to state that the mask is the only automatic clear in this
arithmetic path. Preserve the existing explanation of bit 17 and its lack of
Enable and sticky Flags counterparts.
This was referenced Jul 21, 2026
Merged
This was referenced Jul 29, 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 #27, the dominant remaining n64-systemtest block was the undecoded COP1 funct space —
roughly 1,700 of 2,682 failures. Everything needed was already implemented in
fpu.rs; none of itwas reachable.
Result
All sixteen
C.cond.fmttests now pass outright — from 42 failures apiece to zero.1. The compares and conversions (2,682 → 1,468)
Decode admitted only
funct 0..=3and5..=7in the.S/.Dformats, and never admitted theinteger source formats
.W/.Lat all — so every integer-to-float conversion was a silent no-optoo. Same shape of gap as
MOV.fmtin #27, found this time by asking which neighbouring encodingsshared the range rather than waiting for it to cost another investigation.
ROUND/TRUNC/CEIL/FLOORtake their rounding mode from the opcode and ignoreFCSR.RM;CVT.W/CVT.Lconsult it. That is the entire difference between the two families, and it isinvisible whenever
RMhappens to agree — so the test setsRMto nearest and converts-1.5,where truncate gives
-1and nearest gives-2.fp_arithis restructured around a single commit-or-trap point: each family returns anFpCommitplus flags, and the trap check, the
Cause-only write and the non-retirement happen once ratherthan being duplicated per family.
2. The NaN convention (1,468 → 1,098)
The VR4300 classifies a NaN as signalling when the significand's MSB is SET — the legacy MIPS
convention, inverted from IEEE-754:2008.
0x7FC0_0000, which Rust produces asf32::NANandeverything else calls quiet, raises Invalid on this processor.
Established from the oracle's own expectations, which name their constants the IEEE way and then
assert the opposite. For a non-signalling compare:
0x7FC0_0000(MSB set)0x7FBF_FFFF(MSB clear)The signalling compare forms raise for both and so do not distinguish the conventions —
checking only those would have left the question open.
The corroboration that makes it more than a curve fit: the VR4300's own default NaN result is
0x7FBF_FFFF, MSB clear. Read as IEEE, that is a processor whose invalid-operation result is asignalling NaN, which would re-trap the instant anything touched it. Read under this convention it
is an ordinary quiet one. Two independent facts, from different tests, agreeing on the same
inversion. Accuracy ledger C-12.
Five existing tests asserted the IEEE convention and were updated. One now asserts
is_snan_f32(f32::NAN)deliberately, because that is the case most likely to be "fixed" back bysomeone who has not read the ledger entry.
Testing
347 passing. Every guard mutation-checked — revert the fix, confirm red, restore.
Worth flagging: the decode arm initially had no test at all. Reverting it broke nothing, which
is exactly the decoded-but-no-op blind spot
AGENTS.mdnow warns about. The enumerated decode testand two execution tests were added until the revert goes red. The decode test enumerates the whole
range rather than spot-checking, because the failure mode here is a gap, and a gap is what a
single representative encoding does not find.
Also
.coderabbit.yaml, tuned to this project's decided rules rather than generic Rust style — clippy(pedantic + nursery,
-D warnings) already gates the generic advice, so the useful thing a reviewbot can add is catching contradictions with an ADR or invented facts. It also lists the deliberate
deviations (reverse pipeline cascade, reproduced errata, the inverted NaN convention, soft-float
over native operators) so they are not reported as defects.
Not in scope
Phase 1 is not complete; v0.2.0 is not cut. The dominant remaining block is now the
unmaskable unimplemented-operation cause (bit 17), which the VR4300 raises for subnormal
operands and results and for a quiet-NaN operand to an arithmetic operation.
SQRT(funct 4)also stays undecoded — there is no square-root implementation, so it remains
Cop1Unimplementedrather than becoming a wrong result.