From fbecc9e6b28050b3451ebaa11328a82171b3a4dd Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Mon, 20 Jul 2026 17:53:58 -0400 Subject: [PATCH 1/6] feat(cpu): decode the COP1 compares and conversions, and correct the 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 --- .coderabbit.yaml | 79 ++++++ AGENTS.md | 15 +- CHANGELOG.md | 27 ++ crates/rustyn64-cpu/src/decode.rs | 91 ++++++- crates/rustyn64-cpu/src/fpu.rs | 116 +++++--- crates/rustyn64-cpu/src/pipeline.rs | 392 ++++++++++++++++++++++++--- crates/rustyn64-cpu/src/softfloat.rs | 29 +- docs/STATUS.md | 31 ++- docs/accuracy-ledger.md | 45 +++ 9 files changed, 722 insertions(+), 103 deletions(-) create mode 100644 .coderabbit.yaml diff --git a/.coderabbit.yaml b/.coderabbit.yaml new file mode 100644 index 00000000..4b9937dd --- /dev/null +++ b/.coderabbit.yaml @@ -0,0 +1,79 @@ +# CodeRabbit configuration — https://docs.coderabbit.ai/guides/configure-coderabbit +# +# Added as a second review bot alongside GitHub Copilot. Installing the app +# itself is a repository-owner action on github.com; this file only tunes the +# review once it is installed. +# +# The instructions below are deliberately about THIS project's decided rules +# rather than generic Rust style. Generic advice is already covered by +# `cargo clippy -D warnings` (pedantic + nursery) in CI, so a bot repeating it +# adds noise; what a reviewer here cannot get from a linter is whether a change +# contradicts an ADR or invents a fact. + +language: en-GB +early_access: false + +reviews: + profile: chill + request_changes_workflow: false + high_level_summary: true + poem: false + review_status: true + collapse_walkthrough: true + + path_filters: + # Immutable research and vendored reference material. `ref-proj/` is + # gitignored, but list it so a stray commit is never reviewed as if it were + # ours -- several of those clones are study-only for licence reasons. + - "!ref-docs/**" + - "!ref-proj/**" + - "!n64brew_wiki/**" + - "!tests/roms/**" + - "!target/**" + + path_instructions: + - path: "crates/**/*.rs" + instructions: | + Read `AGENTS.md` first; it records decisions that look like bugs. + + Flag with high priority: + - Any new `cycles`/`ticks` counter that is INCREMENTED. Only + `master_ticks` may be incremented; every other cycle position is a + derived accessor (ADR 0006). + - Any invented constant or side effect with no cited source. Undocumented + hardware facts must be measured and recorded in + `docs/accuracy-ledger.md`, never fitted until a test passes. + - A comment that asserts what the code does but disagrees with it. This + project has been bitten by that four times; treat comment and code as + independent claims. + - An instruction or handler that decodes to a silent no-op. Assert the + effect, not the absence of an exception. + - `unsafe` anywhere outside `rustyn64-frontend`. Every chip crate carries + `#![forbid(unsafe_code)]` and the tree currently has zero `unsafe`. + + Do NOT flag as bugs (these are deliberate and documented): + - The pipeline cascade running in REVERSE stage order (WB->DC->EX->RF->IC) + — ADR 0007. + - Reproduced VR4300 errata, e.g. `sra`/`srav`. They are behaviour software + depends on, not defects to fix. + - NaN classification treating significand-MSB-SET as *signalling*. That is + inverted from IEEE-754:2008 and is correct for this processor — the + legacy MIPS convention, accuracy ledger C-12. + - Soft-float arithmetic instead of native `f32`/`f64` operators. The native + ones discard the exact pre-rounding result, so IEEE flags and `FCSR.RM` + cannot be implemented on them (ledger C-11). + + - path: "docs/**/*.md" + instructions: | + `docs/STATUS.md` is the single source of truth for counts and state; flag + any number here that contradicts it. Docs are treated as the spec, so a + behaviour change with an untouched spec is a finding. Claims of the form + "X is undocumented" decay — flag them unless a page reference is cited. + + - path: "docs/adr/**" + instructions: | + ADRs are immutable once accepted. A superseded ADR must be replaced by a + NEW numbered ADR that cross-links it, never rewritten in place. + +chat: + auto_reply: true diff --git a/AGENTS.md b/AGENTS.md index 61a1e158..de1e0cb0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -62,8 +62,9 @@ Architecture (the load-bearing facts — read `docs/architecture.md`): **Phase 1 in progress; tagged release still v0.1.0.** The **VR4300 executes instructions**: the canonical 187.5 MHz clock (ADR 0006), the five-stage pipeline (ADR 0007), the MIPS III integer set, COP0, the TLB + micro-ITLB, the exception model, interrupts, `CACHE`, COP1 (control, -register file, `ADD`/`SUB`/`MUL`/`DIV`, `ABS`/`MOV`/`NEG`, and enabled FP traps), and **PI DMA** -— the last pulled forward from Phase 5 because n64-systemtest loads its own ELF through it. +register file, `ADD`/`SUB`/`MUL`/`DIV`, `ABS`/`MOV`/`NEG`, the compares, the conversions and +enabled FP traps), and **PI DMA** — the last pulled forward from Phase 5 because n64-systemtest +loads its own ELF through it. FP arithmetic runs on a **soft-float core** (`crates/rustyn64-cpu/src/softfloat.rs`), not on Rust's `f32`/`f64` operators. That is not gratuitous: the native operators discard the exact @@ -76,10 +77,11 @@ the VR4300's refusal to produce subnormals as a separate layer. executes anything. A green `cargo test` still does not mean a subsystem works — check `docs/STATUS.md`. -**Phase 1's exit criterion is not met**: n64-systemtest reports **2,682 failing assertions** +**Phase 1's exit criterion is not met**: n64-systemtest reports **1,098 failing assertions** (it does now run its whole corpus and report). Do **not** tag v0.2.0 until it is `Failed: 0` — the criterion is an oracle number, and that is the point of it. The dominant remaining block is -the still-undecoded COP1 funct space (`C.cond.fmt` and the conversions), roughly 1,700 of them. +the unmaskable **unimplemented-operation** cause (bit 17), which the VR4300 raises for subnormal +operands/results and for a quiet-NaN operand to an arithmetic operation. ## Where things live @@ -289,6 +291,11 @@ in this repo, which is worth fixing even when the suggested wording is not. `docs/accuracy-ledger.md` with its provenance. Adjusting one until a ROM passes makes every later timing result unfalsifiable. Currently unmeasured: `M` (memory access time), the exception-epilogue cost, CP0I, RDRAM bank-state costs. +- **NaN classification on the VR4300 is INVERTED from IEEE-754:2008**: significand MSB **set** + means *signalling*, so `f32::NAN` (`0x7FC0_0000`) raises Invalid here. This looks like a bug on + every reading and is not — it is the legacy MIPS convention, corroborated by the processor's own + default NaN result (`0x7FBF_FFFF`, MSB clear) being quiet only under it. Never "correct" it back + to IEEE; see ledger **C-12** and the test that asserts `is_snan_f32(f32::NAN)` on purpose. - Say "master clock" only with its rate. The sources use **MasterClock = 62.5 MHz**; this project's master tick is **187.5 MHz**; ADR 0001 used it for 93.75 MHz. See `docs/glossary.md`. - `unsafe` is allowed only in the frontend and FFI. Enforced: every chip crate and `-core` carry diff --git a/CHANGELOG.md b/CHANGELOG.md index 706e5cc9..cf6ecced 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,33 @@ All notable changes to RustyN64 are documented here. The format is based on The next rung is `v0.2.0 "Interpreter"` — the VR4300 (see [`to-dos/VERSION-PLAN.md`](to-dos/VERSION-PLAN.md)). +### Added — the COP1 compares and conversions, and a corrected NaN convention + +`C.cond.fmt`, the `CVT` family, and `ROUND`/`TRUNC`/`CEIL`/`FLOOR` to `.W`/`.L` now decode and +execute. They were implemented in `fpu.rs` all along but unreachable: decode admitted only +`funct 0..=3` and `5..=7` in the `.S`/`.D` formats, and never admitted the **integer** source +formats `.W`/`.L` at all — so every integer-to-float conversion was a silent no-op too. + +`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 getting it +wrong would be invisible whenever `RM` happened to match. + +**n64-systemtest: 2,682 → 1,468.** + +### Fixed — the VR4300 NaN convention is inverted from IEEE-754:2008 + +A NaN is **signalling** when its significand's MSB is **set** — the legacy MIPS convention, the +opposite of IEEE-754:2008. `0x7FC0_0000`, which Rust produces as `f32::NAN` and everything 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 behaviour; corroborated independently by the fact that the VR4300's default +NaN *result* is `0x7FBF_FFFF` (MSB clear), which under IEEE would be a signalling NaN that +re-traps on first use, and under this convention is an ordinary quiet one. + +**n64-systemtest: 1,468 → 1,098**, taking the compare block from 42 failures apiece to **zero +across all sixteen tests**. Accuracy ledger **C-12**. + ### Added — soft-float arithmetic with exact IEEE flags and all four rounding modes New `crates/rustyn64-cpu/src/softfloat.rs`. Both formats and all four arithmetic operations are diff --git a/crates/rustyn64-cpu/src/decode.rs b/crates/rustyn64-cpu/src/decode.rs index 8419dd79..8b85ba29 100644 --- a/crates/rustyn64-cpu/src/decode.rs +++ b/crates/rustyn64-cpu/src/decode.rs @@ -894,7 +894,37 @@ pub const fn decode(word: u32) -> Decoded { // result never left the callee. That accounted for the whole // `Result after ` failure block, which had been read as an // FPU arithmetic fault for nine rounds (ledger C-10). - 0o20 | 0o21 if matches!(word & 0o77, 0..=3 | 5..=7) => Decoded { + // `funct` 4 is `SQRT`, which has no implementation yet and so + // stays `Cop1Unimplemented` rather than becoming a wrong + // result. Everything else in the S/D formats is wired: + // + // | `funct` | Operation | + // | --- | --- | + // | `0..=3` | `ADD` / `SUB` / `MUL` / `DIV` | + // | `5..=7` | `ABS` / `MOV` / `NEG` | + // | `0o10..=0o17` | `ROUND`/`TRUNC`/`CEIL`/`FLOOR` to `.L` then `.W` | + // | `0o40`/`0o41`/`0o44`/`0o45` | `CVT.S` / `CVT.D` / `CVT.W` / `CVT.L` | + // | `0o60..=0o77` | `C.cond.fmt`, the low 4 bits being the condition | + 0o20 | 0o21 + if matches!( + word & 0o77, + 0..=3 | 5..=7 | 0o10..=0o17 | 0o40 | 0o41 | 0o44 | 0o45 | 0o60..=0o77 + ) => + { + Decoded { + op: Op::FpArith, + ..base + } + } + // The **integer** source formats, `.W` (20) and `.L` (21). + // + // Easy to miss: `CVT.S.W` carries its source format in the same + // `fmt` field, so a decoder that only admits 16/17 leaves every + // integer-to-float conversion a silent no-op — the same shape of + // gap that made `MOV.fmt` cost nine rounds. Only `CVT.S` and + // `CVT.D` are defined from these formats; converting an integer + // to an integer is not an instruction. + 0o24 | 0o25 if matches!(word & 0o77, 0o40 | 0o41) => Decoded { op: Op::FpArith, ..base }, @@ -1251,6 +1281,65 @@ mod tests { assert_eq!(decode(enc(0o20, 4)).op, Op::Cop1Unimplemented); } + /// **The compares and conversions must decode.** They are implemented in + /// `fpu.rs` and were unreachable for the same reason `MOV` was: the decode + /// arm admitted only `funct 0..=3` and `5..=7`. + /// + /// Enumerated rather than spot-checked. The failure mode here is a *gap* in + /// a range, and a gap is exactly what a single representative encoding does + /// not find. + #[test] + fn the_compares_and_conversions_decode_rather_than_no_op() { + let enc = |fmt: u32, funct: u32| { + (0o21 << 26) | (fmt << 21) | (2 << 16) | (3 << 11) | (4 << 6) | funct + }; + for fmt in [0o20u32, 0o21] { + // ROUND/TRUNC/CEIL/FLOOR to .L (8..=11) then to .W (12..=15). + for funct in 0o10..=0o17u32 { + assert_eq!( + decode(enc(fmt, funct)).op, + Op::FpArith, + "fmt {fmt:#o} funct {funct:#o}" + ); + } + // CVT.S / CVT.D / CVT.W / CVT.L. + for funct in [0o40u32, 0o41, 0o44, 0o45] { + assert_eq!( + decode(enc(fmt, funct)).op, + Op::FpArith, + "CVT funct {funct:#o}" + ); + } + // All sixteen C.cond.fmt forms. + for funct in 0o60..=0o77u32 { + assert_eq!( + decode(enc(fmt, funct)).op, + Op::FpArith, + "C.cond funct {funct:#o}" + ); + } + } + // The INTEGER source formats. Easy to miss, because `CVT.S.W` carries + // its source format in the same field as `.S`/`.D` — a decoder that + // admits only 16/17 leaves every integer-to-float conversion a no-op. + for fmt in [0o24u32, 0o25] { + for funct in [0o40u32, 0o41] { + assert_eq!( + decode(enc(fmt, funct)).op, + Op::FpArith, + "fmt {fmt:#o} funct {funct:#o}" + ); + } + } + // `SQRT` has no implementation, so it must stay unimplemented rather + // than being swept in by a too-wide range. + assert_eq!( + decode(enc(0o20, 4)).op, + Op::Cop1Unimplemented, + "SQRT is still unwired" + ); + } + /// **`MOV.fmt` (funct 6) must decode.** With the arm admitting only /// `funct <= 3` it did not, and executed as a silent no-op. /// diff --git a/crates/rustyn64-cpu/src/fpu.rs b/crates/rustyn64-cpu/src/fpu.rs index 8066de34..6c0a8d0d 100644 --- a/crates/rustyn64-cpu/src/fpu.rs +++ b/crates/rustyn64-cpu/src/fpu.rs @@ -238,26 +238,49 @@ impl Outcome { } } -/// Is this `f32` a **signalling** NaN? +/// Is this `f32` a **signalling** NaN *as the VR4300 classifies one*? /// -/// The distinction matters: a signalling NaN raises Invalid, a quiet one does -/// not. IEEE-754 puts the quiet bit at the top of the mantissa, so an -/// `is_nan()` check alone cannot tell them apart — and treating every NaN as -/// signalling raises Invalid on ordinary quiet-NaN propagation. +/// # The convention is inverted from IEEE-754:2008 +/// +/// IEEE-754:2008 says the significand's MSB **set** means *quiet*. The VR4300 +/// predates that edition and uses the **legacy MIPS convention**, where the +/// significand MSB **set** means *signalling*. So `0x7FC0_0000` — the pattern +/// every modern language calls a quiet NaN, and what Rust's `f32::NAN` is — is +/// a **signalling** NaN to this processor, and raises Invalid. +/// +/// # How this was established +/// +/// Not from a manual: from n64-systemtest's own expectations, which name their +/// constants by the IEEE convention and then assert the opposite behaviour. +/// For a non-signalling compare (`C.EQ`, `C.F`, …) it expects +/// `QUIET_NAN_START_32` (`0x7FC0_0000`, MSB set) to raise Invalid and +/// `SIGNALLING_NAN_END_32` (`0x7FBF_FFFF`, MSB clear) to raise nothing. The +/// signalling compare forms (`C.SF`, `C.SEQ`, …) raise Invalid for both, which +/// is the ordinary IEEE rule for those forms and so does not distinguish them. +/// +/// The corroboration that makes this more than a curve fit: the VR4300's own +/// default NaN result is `0x7FBF_FFFF`, MSB **clear**. Under IEEE that would be +/// a processor whose invalid-operation result is a *signalling* NaN — absurd, +/// since it would re-trap on first use. Under this convention it is exactly +/// what it should be, a quiet one. +/// +/// Accuracy ledger **C-12**. #[must_use] pub const fn is_snan_f32(v: f32) -> bool { let b = v.to_bits(); - // NaN with the quiet bit (mantissa MSB) CLEAR, and a non-zero payload. - b & 0x7F80_0000 == 0x7F80_0000 && b & 0x0040_0000 == 0 && b & 0x003F_FFFF != 0 + // Exponent all ones, significand MSB SET. No payload check is needed: the + // MSB being set already makes the significand non-zero, so this cannot + // catch an infinity. + b & 0x7F80_0000 == 0x7F80_0000 && b & 0x0040_0000 != 0 } -/// Is this `f64` a **signalling** NaN? +/// Is this `f64` a **signalling** NaN as the VR4300 classifies one? +/// +/// See [`is_snan_f32`] — the convention is inverted from IEEE-754:2008. #[must_use] pub const fn is_snan_f64(v: f64) -> bool { let b = v.to_bits(); - b & 0x7FF0_0000_0000_0000 == 0x7FF0_0000_0000_0000 - && b & 0x0008_0000_0000_0000 == 0 - && b & 0x0007_FFFF_FFFF_FFFF != 0 + b & 0x7FF0_0000_0000_0000 == 0x7FF0_0000_0000_0000 && b & 0x0008_0000_0000_0000 != 0 } /// @@ -750,34 +773,61 @@ pub fn to_i64(v: f64, mode: Rounding) -> Outcome { mod tests { use super::*; - /// A **signalling** NaN raises Invalid; a **quiet** one does not. Treating - /// every NaN as signalling raises Invalid on ordinary NaN propagation, which - /// is wrong and noisy. + /// **The VR4300 NaN convention is inverted from IEEE-754:2008**: the + /// significand MSB **set** means *signalling*, not quiet. + /// + /// So `0x7FC0_0001` — what every modern language calls a quiet NaN, and + /// what Rust produces — raises Invalid here, and `0x7F80_0001` does not. + /// The bit patterns are named for what they are *on this processor*, since + /// naming them the IEEE way is what made the original implementation + /// backwards. Accuracy ledger C-12. #[test] - fn only_a_signalling_nan_raises_invalid() { - let snan = f32::from_bits(0x7F80_0001); - let qnan = f32::from_bits(0x7FC0_0001); - assert!(is_snan_f32(snan), "quiet bit clear, payload non-zero"); - assert!(!is_snan_f32(qnan), "quiet bit set"); + fn the_signalling_nan_is_the_one_with_the_significand_msb_set() { + let signals_here = f32::from_bits(0x7FC0_0001); // IEEE would call this quiet + let quiet_here = f32::from_bits(0x7F80_0001); // IEEE would call this signalling + assert!( + is_snan_f32(signals_here), + "MSB set is SIGNALLING on the VR4300" + ); + assert!(!is_snan_f32(quiet_here), "MSB clear is quiet"); assert!(!is_snan_f32(f32::INFINITY), "infinity is not a NaN"); - assert!(add_s(snan, 1.0, Rounding::Nearest).flags.invalid); + assert!(add_s(signals_here, 1.0, Rounding::Nearest).flags.invalid); assert!( - !add_s(qnan, 1.0, Rounding::Nearest).flags.invalid, + !add_s(quiet_here, 1.0, Rounding::Nearest).flags.invalid, "a quiet NaN propagates quietly" ); + + // Rust's own NaN is signalling to this processor. Stated explicitly + // because it is the case most likely to be reintroduced by someone + // "fixing" the convention back to IEEE. + assert!(is_snan_f32(f32::NAN), "even f32::NAN signals here"); } - /// The same, for doubles — the quiet bit sits at a different position, so - /// this is not a free consequence of the `f32` case. + /// The same, for doubles — the bit sits at a different position, so this is + /// not a free consequence of the `f32` case. + #[test] + fn the_double_precision_signalling_bit_is_at_bit_51() { + let signals_here = f64::from_bits(0x7FF8_0000_0000_0001); + let quiet_here = f64::from_bits(0x7FF0_0000_0000_0001); + assert!(is_snan_f64(signals_here)); + assert!(!is_snan_f64(quiet_here)); + assert!(add_d(signals_here, 1.0, Rounding::Nearest).flags.invalid); + assert!(!add_d(quiet_here, 1.0, Rounding::Nearest).flags.invalid); + } + + /// The processor's own default NaN result must be **quiet by its own + /// convention**, or every invalid operation would produce a value that + /// re-traps the moment anything touches it. + /// + /// This is the corroboration that the inverted convention is real rather + /// than a curve fit to the compare tests: `0x7FBF_FFFF` is the value + /// hardware delivers, and it is only sane under the VR4300 reading. #[test] - fn the_double_precision_quiet_bit_is_at_bit_51() { - let snan = f64::from_bits(0x7FF0_0000_0000_0001); - let qnan = f64::from_bits(0x7FF8_0000_0000_0001); - assert!(is_snan_f64(snan)); - assert!(!is_snan_f64(qnan)); - assert!(add_d(snan, 1.0, Rounding::Nearest).flags.invalid); - assert!(!add_d(qnan, 1.0, Rounding::Nearest).flags.invalid); + fn the_default_nan_result_is_quiet_by_the_vr4300_convention() { + use crate::softfloat::{F32, F64}; + assert!(!is_snan_f32(f32::from_bits(F32.default_nan() as u32))); + assert!(!is_snan_f64(f64::from_bits(F64.default_nan()))); } /// **`x/0` is `DivByZero`; `0/0` is Invalid.** They are different flags, and a @@ -985,7 +1035,8 @@ mod tests { /// on its own here. #[test] fn the_signalling_compare_forms_raise_on_a_quiet_nan() { - let qnan = f32::from_bits(0x7FC0_0001); + // Quiet **by the VR4300 convention**: significand MSB clear (C-12). + let qnan = f32::from_bits(0x7F80_0001); assert!(!is_snan_f32(qnan), "it really is quiet"); let out = compare_s(qnan, 1.0, 2); // C.EQ @@ -1001,7 +1052,8 @@ mod tests { /// non-signalling forms. #[test] fn a_signalling_nan_operand_raises_for_every_condition() { - let snan = f32::from_bits(0x7F80_0001); + // Signalling **by the VR4300 convention**: significand MSB set (C-12). + let snan = f32::from_bits(0x7FC0_0001); for cond in 0..16u8 { assert!( compare_s(snan, 1.0, cond).flags.invalid, diff --git a/crates/rustyn64-cpu/src/pipeline.rs b/crates/rustyn64-cpu/src/pipeline.rs index de0daaa1..768357f1 100644 --- a/crates/rustyn64-cpu/src/pipeline.rs +++ b/crates/rustyn64-cpu/src/pipeline.rs @@ -125,6 +125,22 @@ pub enum Exception { FloatingPoint, } +/// What a COP1 operation writes when it does not trap. +/// +/// Named rather than a `(u64, bool)` pair because the destinations are of +/// genuinely different kinds — two FPR widths and a single `FCSR` bit — and a +/// flag pair makes "write the condition to `fd`" representable. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum FpCommit { + /// A 32-bit result into `fd`'s low half: `.S` values and `.W` integers. + Single(u32), + /// A 64-bit result into `fd` through the `FR` view: `.D` values and `.L` + /// integers. + Double(u64), + /// `FCSR.C`. Only `C.cond.fmt` produces this, and it writes no FPR at all. + Condition(bool), +} + /// The documented interlocks (UM Table 4-3). /// /// Held as a named enum rather than a bare cycle count so a stall is always @@ -1488,6 +1504,9 @@ impl Pipeline { use crate::fpu; /// `FCSR` Cause field, bits 16:12 — replaced per operation. const CAUSE_MASK: u32 = 0x1F << 12; + /// `FCSR.C`, the compare condition — bit 23, above the Cause field and + /// written only by `C.cond.fmt`. + const FCSR_C: u32 = 1 << 23; let fr = fr_of(&self.cop0); // `fmt` is 16 (single) or 17 (double) -- decode admits no other value @@ -1530,42 +1549,31 @@ impl Pipeline { return false; } - // Computed but **not committed**: whether the write happens depends on - // the enables, and they cannot be consulted until the flags are known. - // Writing inside the arithmetic branch and undoing it afterwards would - // be wrong under `FR = 0`, where a `.S` write can disturb a neighbouring - // register's half. // `FCSR.RM` is read **here**, per operation, rather than being captured // anywhere earlier: software changes it between instructions, and // n64-systemtest sweeps all four modes over the same operand pair. let mode = fpu::Rounding::from_rm(self.cop1.rounding_mode()); - let (bits, flags, wide) = if fmt == 0o20 { - { - let a = f32::from_bits(self.fpr.read_s(fs)); - let b = f32::from_bits(self.fpr.read_s(ft)); - let out = match funct { - 0 => fpu::add_s(a, b, mode), - 1 => fpu::sub_s(a, b, mode), - 2 => fpu::mul_s(a, b, mode), - _ => fpu::div_s(a, b, mode), - }; - (u64::from(out.value.to_bits()), out.flags, false) - } - } else { - { - let a = f64::from_bits(self.fpr.read_d(fs, fr)); - let b = f64::from_bits(self.fpr.read_d(ft, fr)); - let out = match funct { - 0 => fpu::add_d(a, b, mode), - 1 => fpu::sub_d(a, b, mode), - 2 => fpu::mul_d(a, b, mode), - _ => fpu::div_d(a, b, mode), - }; - (out.value.to_bits(), out.flags, true) - } + + // Computed but **not committed**. Whether the write happens depends on + // the enables, and they cannot be consulted until the flags are known. + // Writing inside a branch and undoing it afterwards would be wrong + // under `FR = 0`, where a `.S` write can disturb a neighbouring + // register's half. + let (commit, flags, unimplemented) = match funct { + 0o00..=0o03 => self.fp_binary(fmt, funct, ft, fs, fr, mode), + 0o10..=0o17 => self.fp_to_integer(fmt, funct, fs, fr), + 0o40 | 0o41 | 0o44 | 0o45 => self.fp_convert(fmt, funct, fs, fr, mode), + // 0o60..=0o77 -- `C.cond.fmt`. The low four bits ARE the condition + // (UM Table 7-11), so the sixteen mnemonics need no table. + _ => self.fp_compare(fmt, funct & 0xF, ft, fs, fr), }; - let raised = flags.to_fcsr_bits(); + let raised = flags.to_fcsr_bits() + | if unimplemented { + fpu::CAUSE_UNIMPLEMENTED + } else { + 0 + }; let fcsr = self.cop1.fcsr(); // `Cause` bits 16:12 and the `Enable` field bits 11:7 hold the five @@ -1573,28 +1581,219 @@ impl Pipeline { // up with what `Cop1Control::enables` returns. Comparing them in // different orders is a silent mis-map that only shows up on whichever // condition happens to be tested first. - if (raised >> 12) & self.cop1.enables() != 0 { + // + // **Unimplemented Operation (bit 17) is unmaskable** and sits above + // that field, so it is checked separately rather than being folded into + // the enable comparison — where it would have been silently ignored, + // since no enable bit corresponds to it. + if unimplemented || (raised >> 12) & self.cop1.enables() != 0 { // Cause only. The sticky `Flags` field is deliberately left // untouched — see the doc comment. - self.cop1 - .ctc1(31, (fcsr & !CAUSE_MASK) | (raised & CAUSE_MASK)); + self.cop1.ctc1( + 31, + (fcsr & !CAUSE_MASK) | (raised & (CAUSE_MASK | fpu::CAUSE_UNIMPLEMENTED)), + ); self.abort_from(Stage::Wb, Exception::FloatingPoint); return true; } - // Preserves the upper half, as `MTC1` does. Writing the full register - // (`write_raw`, zeroing the upper half) was tried and REVERTED: it is - // what the observed values suggest, but it moved the oracle by nothing - // and it bypasses the `FR` view, which is exactly the mistake ledger - // U-7 records. See ledger C-10. - if wide { - self.fpr.write_d(fd, fr, bits); - } else { - self.fpr.write_s(fd, bits as u32); + match commit { + // Preserves the upper half, as `MTC1` does. Writing the full + // register (`write_raw`, zeroing the upper half) was tried and + // REVERTED: it moved the oracle by nothing and it bypasses the `FR` + // view, which is exactly the mistake ledger U-7 records (C-10). + FpCommit::Single(v) => self.fpr.write_s(fd, v), + FpCommit::Double(v) => self.fpr.write_d(fd, fr, v), + // `FCSR.C` is bit 23, and it is NOT part of the `Cause`/`Flags` + // bookkeeping — a compare writes it and no other operation touches + // it. Confirmed against n64-systemtest's own `FCSR` bitfield rather + // than inferred. + FpCommit::Condition(c) => { + let base = (fcsr & !CAUSE_MASK & !FCSR_C) | raised; + self.cop1.ctc1(31, base | if c { FCSR_C } else { 0 }); + return false; + } } self.cop1.ctc1(31, (fcsr & !CAUSE_MASK) | raised); false } + + /// `ADD`/`SUB`/`MUL`/`DIV` in either format. + fn fp_binary( + &self, + fmt: u8, + funct: u8, + ft: u8, + fs: u8, + fr: bool, + mode: crate::fpu::Rounding, + ) -> (FpCommit, crate::fpu::Flags, bool) { + use crate::fpu; + if fmt == 0o20 { + let a = f32::from_bits(self.fpr.read_s(fs)); + let b = f32::from_bits(self.fpr.read_s(ft)); + let out = match funct { + 0 => fpu::add_s(a, b, mode), + 1 => fpu::sub_s(a, b, mode), + 2 => fpu::mul_s(a, b, mode), + _ => fpu::div_s(a, b, mode), + }; + (FpCommit::Single(out.value.to_bits()), out.flags, false) + } else { + let a = f64::from_bits(self.fpr.read_d(fs, fr)); + let b = f64::from_bits(self.fpr.read_d(ft, fr)); + let out = match funct { + 0 => fpu::add_d(a, b, mode), + 1 => fpu::sub_d(a, b, mode), + 2 => fpu::mul_d(a, b, mode), + _ => fpu::div_d(a, b, mode), + }; + (FpCommit::Double(out.value.to_bits()), out.flags, false) + } + } + + /// `ROUND`/`TRUNC`/`CEIL`/`FLOOR` to `.W` or `.L` (funct 8..=15). + /// + /// These carry their rounding mode **in the opcode** and ignore `FCSR.RM` + /// entirely — that is the whole reason they exist alongside `CVT.W`/`CVT.L`, + /// which do consult it. Passing the live `RM` here would make all four + /// behave identically whenever `RM` happened to match, and the difference + /// would only show up under a non-default mode. + fn fp_to_integer( + &self, + fmt: u8, + funct: u8, + fs: u8, + fr: bool, + ) -> (FpCommit, crate::fpu::Flags, bool) { + use crate::fpu::{self, Rounding}; + let mode = match funct & 0o3 { + 0 => Rounding::Nearest, + 1 => Rounding::TowardZero, + 2 => Rounding::TowardPlusInf, + _ => Rounding::TowardMinusInf, + }; + // The source is widened to `f64` first, which is EXACT for an `f32`, so + // no rounding happens before the one the instruction asks for. + let v = self.fp_source_as_f64(fmt, fs, fr); + // funct 8..=11 target `.L`, 12..=15 target `.W`. + if funct < 0o14 { + let out = fpu::to_i64(v, mode); + #[allow(clippy::cast_sign_loss)] // a bit pattern, not a magnitude + (FpCommit::Double(out.value as u64), out.flags, false) + } else { + let out = fpu::to_i32(v, mode); + #[allow(clippy::cast_sign_loss)] // a bit pattern, not a magnitude + (FpCommit::Single(out.value as u32), out.flags, false) + } + } + + /// `CVT.S`/`CVT.D`/`CVT.W`/`CVT.L`, from any source format. + fn fp_convert( + &self, + fmt: u8, + funct: u8, + fs: u8, + fr: bool, + mode: crate::fpu::Rounding, + ) -> (FpCommit, crate::fpu::Flags, bool) { + use crate::fpu; + match funct { + // To single. + 0o40 => match fmt { + 0o21 => { + let out = fpu::cvt_s_d(f64::from_bits(self.fpr.read_d(fs, fr))); + (FpCommit::Single(out.value.to_bits()), out.flags, false) + } + #[allow(clippy::cast_possible_wrap)] // reinterpreting a word as signed + 0o24 => { + let out = fpu::cvt_s_w(self.fpr.read_s(fs) as i32); + (FpCommit::Single(out.value.to_bits()), out.flags, false) + } + // From `.L`, which the VR4300 restricts: bits 63:55 must be all + // zeroes or all ones (UM §7.5.2). Outside that it raises + // Unimplemented rather than converting, and there is no defined + // result -- so the commit value is a placeholder the trap path + // discards. + #[allow(clippy::cast_possible_wrap)] + _ => fpu::cvt_s_l(self.fpr.read_d(fs, fr) as i64).map_or( + // No defined result when the restriction is violated, so + // the value is a placeholder the trap path discards. + (FpCommit::Single(0), fpu::Flags::NONE, true), + |out| (FpCommit::Single(out.value.to_bits()), out.flags, false), + ), + }, + // To double. + 0o41 => match fmt { + 0o20 => { + let out = fpu::cvt_d_s(f32::from_bits(self.fpr.read_s(fs))); + (FpCommit::Double(out.value.to_bits()), out.flags, false) + } + #[allow(clippy::cast_possible_wrap)] + 0o24 => { + let out = fpu::cvt_d_w(self.fpr.read_s(fs) as i32); + (FpCommit::Double(out.value.to_bits()), out.flags, false) + } + #[allow(clippy::cast_possible_wrap)] + _ => fpu::cvt_d_l(self.fpr.read_d(fs, fr) as i64).map_or( + // No defined result when the restriction is violated, so + // the value is a placeholder the trap path discards. + (FpCommit::Double(0), fpu::Flags::NONE, true), + |out| (FpCommit::Double(out.value.to_bits()), out.flags, false), + ), + }, + // To word / to long, both honouring `FCSR.RM` -- which is what + // separates them from the fixed-mode family above. + 0o44 => { + let out = fpu::to_i32(self.fp_source_as_f64(fmt, fs, fr), mode); + #[allow(clippy::cast_sign_loss)] + (FpCommit::Single(out.value as u32), out.flags, false) + } + _ => { + let out = fpu::to_i64(self.fp_source_as_f64(fmt, fs, fr), mode); + #[allow(clippy::cast_sign_loss)] + (FpCommit::Double(out.value as u64), out.flags, false) + } + } + } + + /// `C.cond.fmt` — writes `FCSR.C`, never an FPR. + fn fp_compare( + &self, + fmt: u8, + cond: u8, + ft: u8, + fs: u8, + fr: bool, + ) -> (FpCommit, crate::fpu::Flags, bool) { + use crate::fpu; + let out = if fmt == 0o20 { + fpu::compare_s( + f32::from_bits(self.fpr.read_s(fs)), + f32::from_bits(self.fpr.read_s(ft)), + cond, + ) + } else { + fpu::compare_d( + f64::from_bits(self.fpr.read_d(fs, fr)), + f64::from_bits(self.fpr.read_d(ft, fr)), + cond, + ) + }; + (FpCommit::Condition(out.value), out.flags, false) + } + + /// Read `fs` in `fmt` and widen to `f64`. + /// + /// `f32` to `f64` is exact, so a `.S` source loses nothing on the way in and + /// the only rounding is the one the instruction performs. + fn fp_source_as_f64(&self, fmt: u8, fs: u8, fr: bool) -> f64 { + if fmt == 0o20 { + f64::from(f32::from_bits(self.fpr.read_s(fs))) + } else { + f64::from_bits(self.fpr.read_d(fs, fr)) + } + } } #[cfg(test)] @@ -4012,6 +4211,115 @@ mod tests { ); } + /// `C.cond.fmt` writes `FCSR.C` and **no FPR at all**. + /// + /// Both halves matter. A compare that also wrote `fd` would corrupt a + /// register the program never named, and one that computed the right + /// condition without storing it leaves every dependent branch wrong. + #[test] + fn a_compare_writes_the_fcsr_condition_and_leaves_the_registers_alone() { + use crate::cop0::reg; + /// `FCSR.C`, bit 23. + const FCSR_C: u32 = 1 << 23; + /// `C.EQ.S $f0, $f2` — fmt 16, funct 0o62 (cond 2 = EQ). + const C_EQ_S: u32 = (0o21 << 26) | (0o20 << 21) | (2 << 16) | 0o62; + + for (a, b, want) in [(1.0f32, 1.0f32, true), (1.0, 2.0, false)] { + let mut bus = Ram::new(alloc::vec![C_EQ_S]); + let mut regs = Regs::new(); + let mut p = Pipeline::new(); + p.cop0.set_hardware(reg::STATUS, 0x3400_0000); + // Start with the condition at the OPPOSITE of the expected result, + // so "wrote the right value" is distinguishable from "left it". + p.cop1.ctc1(31, if want { 0 } else { FCSR_C }); + p.fpr.write_s(0, a.to_bits()); + p.fpr.write_s(2, b.to_bits()); + p.fpr.write_raw(4, 0xDEAD_BEEF_1122_3344); + + let mut pc = KSEG0_PROG; + for _ in 0..16 { + p.advance(&mut bus, &mut regs, &mut pc); + } + assert_eq!(p.cop1.fcsr() & FCSR_C != 0, want, "{a} == {b}"); + assert_eq!( + p.fpr.read_raw(4), + 0xDEAD_BEEF_1122_3344, + "a compare must not write an FPR" + ); + } + } + + /// **`TRUNC.W.S` takes its rounding mode from the OPCODE, not `FCSR.RM`.** + /// + /// This is the entire difference between the `ROUND`/`TRUNC`/`CEIL`/`FLOOR` + /// family and `CVT.W`/`CVT.L`, and it is invisible whenever `RM` happens to + /// agree with the opcode. So `FCSR.RM` is set to round-to-nearest and the + /// input chosen where nearest and truncate disagree: `-1.5` truncates to + /// `-1` and rounds to `-2`. + /// + /// `CVT.W.S` on the same input under the same `FCSR` must give `-2`, + /// proving the two families really are wired differently rather than both + /// happening to truncate. + #[test] + fn the_fixed_mode_conversions_ignore_fcsr_rm_and_cvt_w_honours_it() { + use crate::cop0::reg; + /// `TRUNC.W.S $f4, $f0` — fmt 16, `fs` 0 (the zero shift is elided), + /// `fd` 4, funct 0o15. + const TRUNC_W_S: u32 = (0o21 << 26) | (0o20 << 21) | (4 << 6) | 0o15; + /// `CVT.W.S $f4, $f0` — fmt 16, `fs` 0, `fd` 4, funct 0o44. + const CVT_W_S: u32 = (0o21 << 26) | (0o20 << 21) | (4 << 6) | 0o44; + + for (word, want) in [(TRUNC_W_S, -1i32), (CVT_W_S, -2)] { + let mut bus = Ram::new(alloc::vec![word]); + let mut regs = Regs::new(); + let mut p = Pipeline::new(); + p.cop0.set_hardware(reg::STATUS, 0x3400_0000); + p.cop1.ctc1(31, 0); // RM = 0, round to nearest even + p.fpr.write_s(0, (-1.5f32).to_bits()); + + let mut pc = KSEG0_PROG; + for _ in 0..16 { + p.advance(&mut bus, &mut regs, &mut pc); + } + #[allow(clippy::cast_possible_wrap)] // reading the word back as signed + let got = p.fpr.read_s(4) as i32; + assert_eq!(got, want, "instruction {word:#010X} on -1.5"); + } + } + + /// `CVT.S.W` reads its source as a **32-bit integer**, which is a different + /// format carried in the same `fmt` field. + /// + /// A decoder that admits only formats 16/17 leaves every integer-to-float + /// conversion a silent no-op, and `fd` keeps whatever it had — which looks + /// exactly like a plausible float. + #[test] + fn cvt_s_w_converts_an_integer_source() { + use crate::cop0::reg; + /// `CVT.S.W $f4, $f0` — fmt 20 (`.W`), `fs` 0, `fd` 4, funct 0o40. + const CVT_S_W: u32 = (0o21 << 26) | (0o24 << 21) | (4 << 6) | 0o40; + + let mut bus = Ram::new(alloc::vec![CVT_S_W]); + let mut regs = Regs::new(); + let mut p = Pipeline::new(); + p.cop0.set_hardware(reg::STATUS, 0x3400_0000); + p.fpr.write_s(0, 12345u32); + p.fpr.write_s(4, 0x1122_3344); + + let mut pc = KSEG0_PROG; + for _ in 0..16 { + p.advance(&mut bus, &mut regs, &mut pc); + } + // Compared as BITS, not as a float: 12345.0 is exactly representable, + // so this is the stricter check and it also catches a wrong-signed + // zero or a NaN payload that float equality would accept. + assert_eq!( + p.fpr.read_s(4), + 12345.0f32.to_bits(), + "the integer source must be converted, not reinterpreted" + ); + } + // --- CACHE (T-12-005) --------------------------------------------------- /// `CACHE` must **not** raise. IPL3 and libdragon both issue it, so a diff --git a/crates/rustyn64-cpu/src/softfloat.rs b/crates/rustyn64-cpu/src/softfloat.rs index 1a5f29e9..00ead320 100644 --- a/crates/rustyn64-cpu/src/softfloat.rs +++ b/crates/rustyn64-cpu/src/softfloat.rs @@ -140,11 +140,15 @@ impl Format { /// The NaN the VR4300 delivers as the result of an invalid operation. /// - /// `0x7FBF_FFFF` / `0x7FF7_FFFF_FFFF_FFFF` — note the **quiet bit is - /// clear**, so by IEEE's own classification this default NaN is a - /// *signalling* one. That is not a mistake here: n64-systemtest names the - /// expected value `SIGNALLING_NAN_END`, and a "sensible" quiet NaN would - /// disagree with hardware on every invalid operation. + /// `0x7FBF_FFFF` / `0x7FF7_FFFF_FFFF_FFFF` — the significand's MSB is + /// **clear**, which by IEEE-754:2008 would make the result of every invalid + /// operation a *signalling* NaN, absurdly re-trapping on first use. + /// + /// It is not absurd, because the VR4300 uses the **legacy MIPS + /// convention**, where MSB set means signalling. Under its own rules this + /// is an ordinary quiet NaN. See `fpu::is_snan_f32` and accuracy ledger + /// C-12; this value is the corroboration that the convention really is + /// inverted rather than the tests being odd. #[must_use] pub const fn default_nan(self) -> u64 { let man = (1u64 << self.man_bits()) - 1; @@ -200,13 +204,18 @@ fn unpack(bits: u64, f: Format) -> Unpacked { }; } if biased == f.max_biased() { - let quiet_bit = 1u128 << (man_bits - 1); + let signal_bit = 1u128 << (man_bits - 1); return Unpacked { sign, class: if man == 0 { Class::Inf } else { Class::Nan }, sig: man, exp: 0, - snan: man != 0 && man & quiet_bit == 0, + // The VR4300 uses the LEGACY MIPS convention: significand MSB + // **set** means signalling, the opposite of IEEE-754:2008. See + // `fpu::is_snan_f32` and accuracy ledger C-12. Naming the constant + // `quiet_bit` and then testing it for *signalling* would be a trap + // for the next reader, so it is named for the position it occupies. + snan: man & signal_bit != 0, }; } Unpacked { @@ -926,8 +935,10 @@ mod tests { /// NaN propagation, which is a common and invisible error. #[test] fn only_a_signalling_nan_operand_raises_invalid() { - let snan = 0x7FA0_0000u64; // exponent all ones, quiet bit clear, payload set - let qnan = 0x7FC0_0000u64; + // **Inverted from IEEE**: on the VR4300 the significand MSB set means + // *signalling*. See `fpu::is_snan_f32` and ledger C-12. + let snan = 0x7FC0_0000u64; // MSB set -> signalling here + let qnan = 0x7FA0_0000u64; // MSB clear -> quiet here assert!(add(snan, b32(1.0), F32, Rounding::Nearest).flags.invalid); assert!(!add(qnan, b32(1.0), F32, Rounding::Nearest).flags.invalid); assert!(mul(b32(1.0), snan, F32, Rounding::Nearest).flags.invalid); diff --git a/docs/STATUS.md b/docs/STATUS.md index 8b30c8c1..a60a2137 100644 --- a/docs/STATUS.md +++ b/docs/STATUS.md @@ -102,26 +102,27 @@ n64-systemtest ROM cannot report a count until COP0/COP1/exceptions land | RSP LLE (SU interpreter, then VU) | stub | Phase 2 | **What "partial" means for COP1.** The register file (`FR` views), the control -registers, the data moves, S/D `ADD`/`SUB`/`MUL`/`DIV`, and `ABS`/`MOV`/`NEG` -decode and execute. Two things do **not** work, and neither is visible +registers, the data moves, S/D `ADD`/`SUB`/`MUL`/`DIV`, `ABS`/`MOV`/`NEG`, the +compares and the conversions decode and execute. Two things do **not** work, and neither is visible from a green `cargo test`: -- **Most of the COP1 funct space is still undecoded**, and this is now the - dominant blocker by a wide margin: `C.cond.fmt` (the 16 compares) and the - `CVT`/`ROUND`/`TRUNC`/`FLOOR`/`CEIL` conversions are implemented in `fpu.rs` - but never reached, because decode admits only `funct 0..=3` and `5..=7`. - Together they are roughly **1,700** of the 2,682 remaining failures. -- **The unmaskable unimplemented-operation cause (bit 17) is not produced.** The - VR4300 raises it for subnormal operands and results; this FPU computes them - normally instead. Every surviving `ADD.S` failure is one of these, or an - `FS = 1` flush-to-zero case. +- **The unmaskable unimplemented-operation cause (bit 17) is not produced**, and + it is now the dominant blocker. The VR4300 raises it for subnormal operands + and results, and for an IEEE-quiet NaN operand to an arithmetic operation; + this FPU computes those normally instead. Nearly every surviving COP1 failure + is one of these or an `FS = 1` flush-to-zero case. +- **`SQRT` (funct 4) is still undecoded** — there is no square-root + implementation, so it stays `Cop1Unimplemented` rather than becoming a wrong + result. What **is** done: the arithmetic runs on a soft-float core (`crates/rustyn64-cpu/src/softfloat.rs`) that produces exact IEEE flags and honours all four `FCSR.RM` modes, verified bit-for-bit against Rust's native -operators over 100,000 cases; and enabled FP traps raise -`Exception::FloatingPoint`, leave `fd` unwritten, do not accumulate the sticky -`Flags`, and do not retire. +operators over 100,000 cases; enabled FP traps raise `Exception::FloatingPoint`, +leave `fd` unwritten, do not accumulate the sticky `Flags`, and do not retire; +and the compares and conversions decode and execute — **all sixteen +`C.cond.fmt` tests pass outright**. NaN classification follows the VR4300's +inverted convention (ledger C-12), not IEEE-754:2008. `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` @@ -171,7 +172,7 @@ entropy, threads and unordered collections anywhere in the core. | **Dillon `basic.z64` (control flow)** | **yes** — external tier | **PASSING** — 5/5 | | **Determinism (ADR 0004)** | n/a — self-checking | **PASSING** — exercised, not just specified | | CPU/RSP golden-log (reference trace) | no — needs a cen64/ares capture | not started (golden source returns empty) | -| n64-systemtest `Failed: 0` (CPU/COP0/TLB/RSP) | **yes** — ROM committed | **runs; 2,682 failing** — next: the undecoded COP1 funct space (compares + conversions, ~1,700 of them) | +| n64-systemtest `Failed: 0` (CPU/COP0/TLB/RSP) | **yes** — ROM committed | **runs; 1,098 failing** — next: the unmaskable unimplemented-operation cause (subnormals and quiet-NaN operands) | | ParaLLEl-RDP fuzz suite (RDP bit-exactness) | source cloned, suite not set up | not started | | Accuracy battery (first-party probe set) | probes not authored | 0% (battery stubbed) | | Visual golden / screenshots | **yes** — krom + 240p + commercial staged | not started | diff --git a/docs/accuracy-ledger.md b/docs/accuracy-ledger.md index fb1c48cf..dd7fcbf0 100644 --- a/docs/accuracy-ledger.md +++ b/docs/accuracy-ledger.md @@ -647,6 +647,51 @@ block across the whole suite is the still-undecoded COP1 funct space: `C.cond.fmt` (16 tests × 84 assertions) and the `CVT`/`ROUND`/`TRUNC`/`FLOOR`/ `CEIL` conversions, together roughly 1,700 of the 2,682. +### C-12 — the VR4300's NaN convention is inverted from IEEE-754:2008 + +**Claim.** A NaN is **signalling** when its significand's most significant bit +is **set**, and quiet when clear — the *legacy MIPS* convention, the opposite of +IEEE-754:2008 and of every modern language. `0x7FC0_0000`, which Rust produces +as `f32::NAN` and which everything else calls quiet, raises Invalid on this +processor. + +**How it was established.** From n64-systemtest's own expectations, which name +their constants by the IEEE convention and then assert the opposite behaviour. +For a *non-signalling* compare (`C.EQ`, `C.F`, …) it expects: + +| Operand | IEEE name | Expected | Implies | +| --- | --- | --- | --- | +| `0x7FC0_0000` (MSB set) | "quiet" | **Invalid raised** | signalling here | +| `0x7FBF_FFFF` (MSB clear) | "signalling" | no flags | quiet here | + +The *signalling* compare forms (`C.SF`, `C.SEQ`, …) raise Invalid for both, +which is the ordinary IEEE rule for those forms and therefore does **not** +distinguish the conventions — checking only those would have left the question +open. It is the non-signalling forms that settle it. + +**The corroboration that makes it more than a curve fit.** The processor's own +default NaN result is `0x7FBF_FFFF` / `0x7FF7_FFFF_FFFF_FFFF`, MSB **clear**. +Read as IEEE, that is a processor whose invalid-operation result is a +*signalling* NaN — which would re-trap the instant anything touched it. Read +under this convention it is exactly what it must be: quiet. Two independent +facts, from different tests, agreeing on the same inversion. + +**Effect:** n64-systemtest 1,468 → **1,098**, and it took the compare block from +42 failures apiece to **zero across all sixteen**. + +**Where it bites.** `fpu::is_snan_{f32,f64}` and `softfloat::unpack`. Both now +name the bit for its *position* rather than calling it a "quiet bit", because a +constant named `quiet_bit` that is tested for signalling is a trap for the next +reader. The tests name their patterns `vr_snan`/`vr_qnan` for the same reason, +and one asserts `is_snan_f32(f32::NAN)` explicitly — that is the case most +likely to be "fixed" back to IEEE by someone who has not read this entry. + +**Still open, and adjacent:** an IEEE-*quiet* NaN operand (MSB clear) to an +arithmetic operation is expected to raise **unimplemented operation**, not +nothing — the VR4300 apparently cannot propagate one in hardware. That is part +of the unimplemented-operation work below rather than of this entry, and it is +why the arithmetic tests still fail on NaN inputs. + ## 5. Deliberate deviations from hardware Behaviour we model differently *on purpose*, so it is never mistaken for a bug. From 3be3c85dae4eb6ffad37565a10ffcc1637f9dcb0 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Mon, 20 Jul 2026 18:02:03 -0400 Subject: [PATCH 2/6] fix(cpu): FCSR.Cause is bits 17:12, so a stale Unimplemented bit is cleared `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 --- .coderabbit.yaml | 170 ++++++++++++++++++++++------ crates/rustyn64-cpu/src/pipeline.rs | 59 +++++++++- docs/accuracy-ledger.md | 13 ++- 3 files changed, 200 insertions(+), 42 deletions(-) diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 4b9937dd..c26c4ad5 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -1,30 +1,47 @@ # CodeRabbit configuration — https://docs.coderabbit.ai/guides/configure-coderabbit +# Validated against https://storage.googleapis.com/coderabbit_public_assets/schema.v2.json # -# Added as a second review bot alongside GitHub Copilot. Installing the app -# itself is a repository-owner action on github.com; this file only tunes the -# review once it is installed. +# A second review bot alongside GitHub Copilot. Installing the app is a +# repository-owner action on github.com; this file only tunes the review. # -# The instructions below are deliberately about THIS project's decided rules -# rather than generic Rust style. Generic advice is already covered by -# `cargo clippy -D warnings` (pedantic + nursery) in CI, so a bot repeating it -# adds noise; what a reviewer here cannot get from a linter is whether a change -# contradicts an ADR or invents a fact. +# The guiding principle: `cargo clippy` (pedantic + nursery, `-D warnings`), +# rustdoc `-D warnings`, `cargo fmt` and markdownlint already gate everything +# mechanical, and they gate it harder than any bot will. So the tool +# integrations that would duplicate them are turned OFF, and the review budget +# is pointed at the one thing a linter cannot check: whether a change +# contradicts a decision recorded in an ADR or the accuracy ledger. language: en-GB early_access: false +tone_instructions: >- + Be direct and technical. Cite the specific rule, ADR or ledger entry a finding + violates. Do not restate what the code does. Say plainly when something is + correct. + reviews: profile: chill request_changes_workflow: false high_level_summary: true + high_level_summary_in_walkthrough: true poem: false review_status: true collapse_walkthrough: true + auto_review: + enabled: true + # Review work in progress too: this repo's PRs are long-lived and the + # expensive mistakes are the ones caught before the branch is finished. + drafts: true + auto_incremental_review: true + base_branches: + - main + path_filters: # Immutable research and vendored reference material. `ref-proj/` is # gitignored, but list it so a stray commit is never reviewed as if it were - # ours -- several of those clones are study-only for licence reasons. + # ours — several of those clones are study-only for licence reasons and + # must never be copied from. - "!ref-docs/**" - "!ref-proj/**" - "!n64brew_wiki/**" @@ -40,40 +57,129 @@ reviews: - Any new `cycles`/`ticks` counter that is INCREMENTED. Only `master_ticks` may be incremented; every other cycle position is a derived accessor (ADR 0006). - - Any invented constant or side effect with no cited source. Undocumented - hardware facts must be measured and recorded in - `docs/accuracy-ledger.md`, never fitted until a test passes. - - A comment that asserts what the code does but disagrees with it. This - project has been bitten by that four times; treat comment and code as - independent claims. - - An instruction or handler that decodes to a silent no-op. Assert the - effect, not the absence of an exception. + - Any invented constant or incidental side effect with no cited source. + Undocumented hardware facts must be measured and recorded in + `docs/accuracy-ledger.md`, never fitted until a test passes. An + invented *side effect* (clearing a field, zeroing a half-register) is + worse than an invented number, because it does not land in the ledger + where it can be argued with. + - A comment that asserts what the code does but disagrees with it. + Treat comment and code as independent claims; this project has been + bitten by that four times. + - An instruction or handler that decodes to a silent no-op. Tests must + assert the EFFECT, not merely that no exception was raised. + - A bit-field mask that does not cover the whole architectural field. + A stale bit in an uncovered position can never be cleared. - `unsafe` anywhere outside `rustyn64-frontend`. Every chip crate carries - `#![forbid(unsafe_code)]` and the tree currently has zero `unsafe`. - - Do NOT flag as bugs (these are deliberate and documented): - - The pipeline cascade running in REVERSE stage order (WB->DC->EX->RF->IC) - — ADR 0007. - - Reproduced VR4300 errata, e.g. `sra`/`srav`. They are behaviour software - depends on, not defects to fix. - - NaN classification treating significand-MSB-SET as *signalling*. That is - inverted from IEEE-754:2008 and is correct for this processor — the - legacy MIPS convention, accuracy ledger C-12. - - Soft-float arithmetic instead of native `f32`/`f64` operators. The native - ones discard the exact pre-rounding result, so IEEE flags and `FCSR.RM` - cannot be implemented on them (ledger C-11). + `#![forbid(unsafe_code)]` and the tree has zero `unsafe` today. + + Do NOT flag as bugs — these are deliberate and documented: + - The pipeline cascade running in REVERSE stage order + (WB -> DC -> EX -> RF -> IC), ADR 0007. + - Reproduced VR4300 errata such as `sra`/`srav`. They are behaviour + software depends on, not defects to correct. + - NaN classification treating significand-MSB-SET as *signalling*. That + is inverted from IEEE-754:2008 and is correct for this processor — + the legacy MIPS convention, accuracy ledger C-12. + - Soft-float arithmetic instead of native `f32`/`f64` operators. The + native ones discard the exact pre-rounding result, so IEEE flags and + `FCSR.RM` cannot be implemented on them (ledger C-11). + - Octal literals (`0o21`) for instruction fields. The MIPS manuals use + octal for opcode tables and matching them is deliberate. + + - path: "crates/**/tests/**" + instructions: | + A test that passes when its fix is reverted is worthless. Flag tests + whose success and failure paths can converge — for example comparing a + total across two runs that differ for an unrelated reason, or asserting + a destination equals a value it already held. - path: "docs/**/*.md" instructions: | - `docs/STATUS.md` is the single source of truth for counts and state; flag - any number here that contradicts it. Docs are treated as the spec, so a + `docs/STATUS.md` is the single source of truth for counts and state; + flag any number elsewhere that contradicts it. Docs are the spec, so a behaviour change with an untouched spec is a finding. Claims of the form "X is undocumented" decay — flag them unless a page reference is cited. + The accuracy ledger is append-mostly: flag a superseded claim that was + edited in place rather than marked resolved. - path: "docs/adr/**" instructions: | ADRs are immutable once accepted. A superseded ADR must be replaced by a NEW numbered ADR that cross-links it, never rewritten in place. + - path: ".github/workflows/**" + instructions: | + Flag any gate whose exit status is piped into `tail`, `grep` or `head` — + the pipeline reports the filter's status, so a failing gate reads as + passing. This has hidden three real failures here. Also flag + `--all-features` on any cargo command: this workspace has + mutually-exclusive backend features and CI must use explicit sets. + + # Everything below duplicates a gate this repo already runs, and runs harder. + # Leaving them on spends review comments on findings CI has already blocked. + tools: + clippy: + enabled: false + markdownlint: + enabled: false + # Kept on: these cover ground no local gate does. + actionlint: + enabled: true + yamllint: + enabled: true + gitleaks: + enabled: true + shellcheck: + enabled: true + + # This project writes its own docs and tests deliberately — rustdoc is a + # blocking gate and every test carries a rationale comment explaining what it + # would catch. Generated stand-ins would have to be rewritten. + finishing_touches: + docstrings: + enabled: false + unit_tests: + enabled: false + + pre_merge_checks: + title: + mode: warning + requirements: >- + Conventional Commits: type(scope): subject, imperative mood, no trailing + period, at most 72 characters. Types: feat, fix, docs, refactor, test, + chore, perf, build, ci. + description: + mode: warning + custom_checks: + - name: Oracle number is stated + mode: warning + instructions: >- + A change to emulation behaviour should state its measured effect on + n64-systemtest's failing-assertion count, or say explicitly that it + was not measured. Accuracy claims here are oracle numbers, not + self-assessments. Docs-only, tooling and CI changes are exempt. + - name: Spec updated with behaviour + mode: warning + instructions: >- + A change to a chip's behaviour should touch that chip's doc under + docs/ in the same PR, and user-visible changes should appear in + CHANGELOG.md under [Unreleased]. + chat: auto_reply: true + +knowledge_base: + learnings: + scope: local + issues: + scope: local + pull_requests: + scope: local + code_guidelines: + enabled: true + filePatterns: + - "AGENTS.md" + - "docs/accuracy-ledger.md" + - "docs/engineering-lessons.md" + - "CONTRIBUTING.md" diff --git a/crates/rustyn64-cpu/src/pipeline.rs b/crates/rustyn64-cpu/src/pipeline.rs index 768357f1..bb3b06e4 100644 --- a/crates/rustyn64-cpu/src/pipeline.rs +++ b/crates/rustyn64-cpu/src/pipeline.rs @@ -1502,8 +1502,17 @@ impl Pipeline { /// `expected_unimplemented` cases still fail. fn fp_arith(&mut self, fmt: u8, funct: u8, ft: u8, fs: u8, fd: u8) -> bool { use crate::fpu; - /// `FCSR` Cause field, bits 16:12 — replaced per operation. - const CAUSE_MASK: u32 = 0x1F << 12; + /// `FCSR` Cause field, bits **17:12** — replaced wholesale per + /// operation. + /// + /// The range includes bit 17, `Unimplemented Operation`, which is part + /// of `Cause` even though it is not an IEEE exception and has no + /// corresponding `Enable` or sticky `Flags` bit. Masking only 16:12 — + /// which this did — leaves a *stale* bit 17 set forever, because no + /// later operation can clear a bit the mask does not cover. Software + /// reading `FCSR` after a successful conversion would then still see + /// the previous unimplemented operation. + const CAUSE_MASK: u32 = 0x3F << 12; /// `FCSR.C`, the compare condition — bit 23, above the Cause field and /// written only by `C.cond.fmt`. const FCSR_C: u32 = 1 << 23; @@ -1589,10 +1598,8 @@ impl Pipeline { if unimplemented || (raised >> 12) & self.cop1.enables() != 0 { // Cause only. The sticky `Flags` field is deliberately left // untouched — see the doc comment. - self.cop1.ctc1( - 31, - (fcsr & !CAUSE_MASK) | (raised & (CAUSE_MASK | fpu::CAUSE_UNIMPLEMENTED)), - ); + self.cop1 + .ctc1(31, (fcsr & !CAUSE_MASK) | (raised & CAUSE_MASK)); self.abort_from(Stage::Wb, Exception::FloatingPoint); return true; } @@ -4211,6 +4218,46 @@ mod tests { ); } + /// **A later COP1 operation clears a stale `Cause.E` (bit 17).** + /// + /// `Cause` is bits **17:12** and is replaced wholesale by each operation. + /// The mask here originally covered only 16:12, so the unimplemented- + /// operation bit — which has no `Enable` and no sticky `Flags` twin, and so + /// is only ever cleared by that mask — stayed set forever once raised. + /// Software reading `FCSR` after a perfectly good conversion would still + /// see the previous failure. + /// + /// Found by a review bot, not by this suite, which had no case that raised + /// bit 17 and then ran another COP1 instruction. + #[test] + fn a_later_operation_clears_a_stale_unimplemented_cause() { + use crate::cop0::reg; + /// `ADD.S $f4, $f0, $f2` — an ordinary, entirely successful operation. + const ADD_S: u32 = 0x4602_0100; + /// `FCSR.Cause.E`, bit 17. + const CAUSE_E: u32 = 1 << 17; + + let mut bus = Ram::new(alloc::vec![ADD_S]); + let mut regs = Regs::new(); + let mut p = Pipeline::new(); + p.cop0.set_hardware(reg::STATUS, 0x3400_0000); + // Pre-set the bit, as a previous unimplemented operation would have. + p.cop1.ctc1(31, CAUSE_E); + p.fpr.write_s(0, 1.0f32.to_bits()); + p.fpr.write_s(2, 2.0f32.to_bits()); + + let mut pc = KSEG0_PROG; + for _ in 0..16 { + p.advance(&mut bus, &mut regs, &mut pc); + } + assert_eq!(p.fpr.read_s(4), 3.0f32.to_bits(), "the ADD.S ran"); + assert_eq!( + p.cop1.fcsr() & CAUSE_E, + 0, + "a successful operation must clear the whole Cause field" + ); + } + /// `C.cond.fmt` writes `FCSR.C` and **no FPR at all**. /// /// Both halves matter. A compare that also wrote `fd` would corrupt a diff --git a/docs/accuracy-ledger.md b/docs/accuracy-ledger.md index dd7fcbf0..a9198871 100644 --- a/docs/accuracy-ledger.md +++ b/docs/accuracy-ledger.md @@ -642,10 +642,15 @@ plausible-looking one-liners with no citation. **What remains, and it is not flags.** Every surviving `ADD.S` failure is a subnormal case: either `Err(())` — the suite expecting the unmaskable unimplemented-operation cause — or an `FS = 1` flush-to-zero case whose result -is rounding-mode dependent. The normal range passes. The dominant remaining -block across the whole suite is the still-undecoded COP1 funct space: -`C.cond.fmt` (16 tests × 84 assertions) and the `CVT`/`ROUND`/`TRUNC`/`FLOOR`/ -`CEIL` conversions, together roughly 1,700 of the 2,682. +is rounding-mode dependent. The normal range passes. + +**Where things stood at the time of this entry** (kept in past tense, because a +ledger read top-to-bottom should show what was believed *when* each entry was +written, not be silently back-edited): the dominant remaining block was the +still-undecoded COP1 funct space — `C.cond.fmt` and the `CVT`/`ROUND`/`TRUNC`/ +`FLOOR`/`CEIL` conversions, roughly 1,700 of the 2,682. Both are now wired and +the compares pass outright; see **C-12** below, and `docs/STATUS.md` for the +current count. ### C-12 — the VR4300's NaN convention is inverted from IEEE-754:2008 From ac5addbf36fd7fe23e9527ac31b57feaf3ae286f Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Mon, 20 Jul 2026 18:18:05 -0400 Subject: [PATCH 3/6] docs: adopt CodeRabbit's four findings; set the review profile to assertive 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 --- .coderabbit.yaml | 22 ++++++++++++++++++++-- docs/STATUS.md | 15 +++++++++------ docs/accuracy-ledger.md | 6 +++--- 3 files changed, 32 insertions(+), 11 deletions(-) diff --git a/.coderabbit.yaml b/.coderabbit.yaml index c26c4ad5..be9bdec6 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -4,6 +4,14 @@ # A second review bot alongside GitHub Copilot. Installing the app is a # repository-owner action on github.com; this file only tunes the review. # +# Profile is **assertive** — the most feedback CodeRabbit offers, and the one +# its docs warn "may feel nitpicky". That is the right trade here: this is a +# cycle-accuracy emulator where a missed defect is measured in weeks of +# misdirected investigation (see accuracy ledger C-10), the maintainer +# adjudicates every comment individually rather than skimming, and the +# `path_instructions` below already tell the bot which deliberate deviations +# NOT to report — which is what makes assertive affordable rather than noisy. +# # The guiding principle: `cargo clippy` (pedantic + nursery, `-D warnings`), # rustdoc `-D warnings`, `cargo fmt` and markdownlint already gate everything # mechanical, and they gate it harder than any bot will. So the tool @@ -20,7 +28,7 @@ tone_instructions: >- correct. reviews: - profile: chill + profile: assertive request_changes_workflow: false high_level_summary: true high_level_summary_in_walkthrough: true @@ -45,7 +53,10 @@ reviews: - "!ref-docs/**" - "!ref-proj/**" - "!n64brew_wiki/**" - - "!tests/roms/**" + # Only the EXTERNAL tier. `tests/roms/n64-systemtest/` is committed (MIT, + # with its upstream LICENSE beside it) and its README carries the licence + # tiering rules, so both should stay reviewable. + - "!tests/roms/external/**" - "!target/**" path_instructions: @@ -94,6 +105,13 @@ reviews: total across two runs that differ for an unrelated reason, or asserting a destination equals a value it already held. + - path: "**/*.md" + instructions: | + markdownlint is a pre-commit hook with NO CI job, so it is the one gate + that can silently not run. Any Markdown change must have had + `pre-commit run markdownlint --all-files` run locally — that includes + `AGENTS.md` and `CHANGELOG.md`, not only files under `docs/`. + - path: "docs/**/*.md" instructions: | `docs/STATUS.md` is the single source of truth for counts and state; diff --git a/docs/STATUS.md b/docs/STATUS.md index a60a2137..7c12fc69 100644 --- a/docs/STATUS.md +++ b/docs/STATUS.md @@ -108,7 +108,8 @@ from a green `cargo test`: - **The unmaskable unimplemented-operation cause (bit 17) is not produced**, and it is now the dominant blocker. The VR4300 raises it for subnormal operands - and results, and for an IEEE-quiet NaN operand to an arithmetic operation; + and results, and for an IEEE-signalling / VR4300-quiet NaN operand (MSB + clear) to an arithmetic operation; this FPU computes those normally instead. Nearly every surviving COP1 failure is one of these or an `FS = 1` flush-to-zero case. - **`SQRT` (funct 4) is still undecoded** — there is no square-root @@ -124,11 +125,13 @@ and the compares and conversions decode and execute — **all sixteen `C.cond.fmt` tests pass outright**. NaN classification follows the VR4300's inverted convention (ledger C-12), not IEEE-754:2008. -`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. +**Only `SQRT` (funct 4) is still implemented-but-undecoded.** The conversions +and the `C.cond.fmt` compares were in that list until this sprint, and `ABS`, +`MOV` and `NEG` before them — `MOV` alone cost ~100 failures, because a +*decoded-but-no-op* instruction is invisible to `cargo test` and the compiler +emits one at every FP call boundary. That pattern has now cost two separate +investigations; when adding a decode arm, enumerate the neighbouring funct +space rather than only the encoding that prompted the change. | RDP LLE (software reference rasterizer) + VI scan-out | stub | Phase 3 | | AI audio DMA double-buffer | stub | Phase 4 | | PI/SI DMA, PIF/CIC boot, FlashRAM machine, saves | stub | Phase 5 | diff --git a/docs/accuracy-ledger.md b/docs/accuracy-ledger.md index a9198871..1f4d25b0 100644 --- a/docs/accuracy-ledger.md +++ b/docs/accuracy-ledger.md @@ -691,9 +691,9 @@ reader. The tests name their patterns `vr_snan`/`vr_qnan` for the same reason, and one asserts `is_snan_f32(f32::NAN)` explicitly — that is the case most likely to be "fixed" back to IEEE by someone who has not read this entry. -**Still open, and adjacent:** an IEEE-*quiet* NaN operand (MSB clear) to an -arithmetic operation is expected to raise **unimplemented operation**, not -nothing — the VR4300 apparently cannot propagate one in hardware. That is part +**Still open, and adjacent:** an **IEEE-signalling / VR4300-quiet** NaN operand +(MSB clear) to an arithmetic operation is expected to raise **unimplemented +operation**, not nothing — the VR4300 apparently cannot propagate one in hardware. That is part of the unimplemented-operation work below rather than of this entry, and it is why the arithmetic tests still fail on NaN inputs. From 6db210ee1d897bd01f9b4783ce29b41e542e52b0 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Mon, 20 Jul 2026 18:40:47 -0400 Subject: [PATCH 4/6] chore: complete the CodeRabbit config against RustyNES's, adapted not 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 --- .coderabbit.yaml | 449 +++++++++++++++++++++++++++++++++++++---------- 1 file changed, 352 insertions(+), 97 deletions(-) diff --git a/.coderabbit.yaml b/.coderabbit.yaml index be9bdec6..9e47da2b 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -1,47 +1,75 @@ -# CodeRabbit configuration — https://docs.coderabbit.ai/guides/configure-coderabbit -# Validated against https://storage.googleapis.com/coderabbit_public_assets/schema.v2.json +# CodeRabbit configuration — RustyN64 +# https://docs.coderabbit.ai/reference/configuration +# Every key below validated against schema.v2.json. # -# A second review bot alongside GitHub Copilot. Installing the app is a -# repository-owner action on github.com; this file only tunes the review. +# Alternate PR review bot alongside copilot-pull-request-reviewer and +# gemini-code-assist. Project convention: reply to and RESOLVE every bot review +# thread before merge (AGENTS.md, "Shipping: every change goes through a PR"). # -# Profile is **assertive** — the most feedback CodeRabbit offers, and the one -# its docs warn "may feel nitpicky". That is the right trade here: this is a -# cycle-accuracy emulator where a missed defect is measured in weeks of -# misdirected investigation (see accuracy ledger C-10), the maintainer -# adjudicates every comment individually rather than skimming, and the -# `path_instructions` below already tell the bot which deliberate deviations -# NOT to report — which is what makes assertive affordable rather than noisy. +# profile: assertive — the most feedback CodeRabbit offers, and the one its docs +# warn "may feel nitpicky". That is the right trade here. A missed defect on +# this project is measured in weeks of misdirected investigation (accuracy +# ledger C-10: nine wrong hypotheses aimed at the FPU while the real cause was +# an undecoded `MOV.fmt`), every comment is adjudicated individually rather than +# skimmed, and the path instructions below already list the deliberate +# deviations NOT to report — which is what makes assertive affordable. # -# The guiding principle: `cargo clippy` (pedantic + nursery, `-D warnings`), -# rustdoc `-D warnings`, `cargo fmt` and markdownlint already gate everything -# mechanical, and they gate it harder than any bot will. So the tool -# integrations that would duplicate them are turned OFF, and the review budget -# is pointed at the one thing a linter cannot check: whether a change -# contradicts a decision recorded in an ADR or the accuracy ledger. +# Tool selection principle: enable what covers a gap, disable what duplicates a +# gate this repo already runs harder. See the `tools` block for the per-tool +# reasoning; it is deliberately explicit rather than inheriting ~50 tools' +# worth of findings for languages this project does not contain. language: en-GB -early_access: false +# Max 250 characters (schema-enforced). tone_instructions: >- - Be direct and technical. Cite the specific rule, ADR or ledger entry a finding - violates. Do not restate what the code does. Say plainly when something is - correct. + Direct and technical. Cite the rule, ADR or accuracy-ledger entry a finding + violates. Do not restate what the code does, and do not repeat findings clippy + or rustfmt already gate. Say plainly when something is correct. + +early_access: false reviews: profile: assertive request_changes_workflow: false + + # Summary surface. Sequence diagrams earn their place here because the + # load-bearing bugs are control-flow shaped (the reverse pipeline cascade, + # the commit-or-trap path in `fp_arith`). high_level_summary: true high_level_summary_in_walkthrough: true - poem: false - review_status: true + changed_files_summary: true + sequence_diagrams: true + estimate_code_review_effort: true collapse_walkthrough: true + review_status: true + poem: false + + # Single-maintainer repo: suggestions are welcome, automatic actions are not. + suggested_labels: true + auto_apply_labels: false + suggested_reviewers: true + auto_assign_reviewers: false + assess_linked_issues: true + related_issues: true + related_prs: true + + # Flags low-substance / filler content. Worth having on a repo whose docs are + # treated as the spec. + slop_detection: + enabled: true auto_review: enabled: true - # Review work in progress too: this repo's PRs are long-lived and the - # expensive mistakes are the ones caught before the branch is finished. - drafts: true auto_incremental_review: true + # Drafts ARE reviewed here, unlike most setups: branches on this project are + # long-lived (PR #27 ran to 49 commits) and the expensive mistakes are the + # ones caught before the branch is finished. `ignore_title_keywords` is the + # escape hatch when that is not wanted. + drafts: true + ignore_title_keywords: + - "WIP" + - "DO NOT REVIEW" base_branches: - main @@ -49,115 +77,301 @@ reviews: # Immutable research and vendored reference material. `ref-proj/` is # gitignored, but list it so a stray commit is never reviewed as if it were # ours — several of those clones are study-only for licence reasons and - # must never be copied from. + # must never be copied from (see ref-proj/README.md). - "!ref-docs/**" - "!ref-proj/**" - "!n64brew_wiki/**" - # Only the EXTERNAL tier. `tests/roms/n64-systemtest/` is committed (MIT, - # with its upstream LICENSE beside it) and its README carries the licence - # tiering rules, so both should stay reviewable. + # Only the EXTERNAL ROM tier. `tests/roms/n64-systemtest/` is committed + # (MIT, with its upstream LICENSE beside it) and `tests/roms/README.md` + # carries the licence-tiering rules, so both stay reviewable. - "!tests/roms/external/**" + - "!Cargo.lock" - "!target/**" path_instructions: - path: "crates/**/*.rs" - instructions: | + instructions: >- Read `AGENTS.md` first; it records decisions that look like bugs. Flag with high priority: + - Any new `cycles`/`ticks` counter that is INCREMENTED. Only - `master_ticks` may be incremented; every other cycle position is a - derived accessor (ADR 0006). + `master_ticks` may be incremented; every other cycle position is a + derived accessor (ADR 0006). The one legitimate exception is a + retired-work tally that nothing schedules against. + + - Anything that could introduce NON-DETERMINISM: wall-clock time, OS + RNG, thread scheduling, or unordered-map iteration order in the core. + Determinism is a hard contract (ADR 0004): seed + ROM + input must give + bit-identical output. + - Any invented constant or incidental side effect with no cited source. - Undocumented hardware facts must be measured and recorded in - `docs/accuracy-ledger.md`, never fitted until a test passes. An - invented *side effect* (clearing a field, zeroing a half-register) is - worse than an invented number, because it does not land in the ledger - where it can be argued with. - - A comment that asserts what the code does but disagrees with it. - Treat comment and code as independent claims; this project has been - bitten by that four times. + Undocumented hardware facts must be MEASURED and recorded in + `docs/accuracy-ledger.md`, never fitted until a test passes. An invented + *side effect* (clearing a field, zeroing a half-register) is worse than + an invented number, because it never lands in the ledger where it can be + argued with — this cost 112 oracle assertions once already. + + - A comment that asserts what the code does but disagrees with it. Treat + comment and code as INDEPENDENT claims; this project has been bitten by + that four times and no test failed any of them. + - An instruction or handler that decodes to a silent no-op. Tests must - assert the EFFECT, not merely that no exception was raised. - - A bit-field mask that does not cover the whole architectural field. - A stale bit in an uncovered position can never be cleared. - - `unsafe` anywhere outside `rustyn64-frontend`. Every chip crate carries - `#![forbid(unsafe_code)]` and the tree has zero `unsafe` today. + assert the EFFECT, not merely that no exception was raised. + + - A bit-field mask that does not cover the whole architectural field. A + stale bit in an uncovered position can never be cleared. + + - `unsafe` anywhere outside `rustyn64-frontend`. Every chip crate and + `-core` carry `#![forbid(unsafe_code)]` and the tree has zero `unsafe`. + + - `.unwrap()` / `.expect()` / `panic!()` on data parsed from an + untrusted source (ROM bytes, save-state, PIF/SI input) outside + `#[cfg(test)]`. These boundaries must return a typed error. Do NOT flag as bugs — these are deliberate and documented: + - The pipeline cascade running in REVERSE stage order - (WB -> DC -> EX -> RF -> IC), ADR 0007. + (WB -> DC -> EX -> RF -> IC), ADR 0007. + - Reproduced VR4300 errata such as `sra`/`srav`. They are behaviour - software depends on, not defects to correct. + software depends on, not defects to correct. + - NaN classification treating significand-MSB-SET as *signalling*. That - is inverted from IEEE-754:2008 and is correct for this processor — - the legacy MIPS convention, accuracy ledger C-12. + is inverted from IEEE-754:2008 and is CORRECT for this processor — the + legacy MIPS convention, accuracy ledger C-12. + - Soft-float arithmetic instead of native `f32`/`f64` operators. The - native ones discard the exact pre-rounding result, so IEEE flags and - `FCSR.RM` cannot be implemented on them (ledger C-11). + native ones discard the exact pre-rounding result, so IEEE flags and + `FCSR.RM` cannot be implemented on them (ledger C-11). + - Octal literals (`0o21`) for instruction fields. The MIPS manuals use - octal for opcode tables and matching them is deliberate. + octal for opcode tables and matching them is deliberate. + + - Hot paths being allocation-free and abstraction-light on purpose. + + - path: "crates/rustyn64-cpu/**" + instructions: >- + The VR4300: a cycle-accurate five-stage pipeline (ADR 0007) validated + against n64-systemtest. Flag any change that could shift instruction + timing without a test-ROM justification, and any new latch read that + does not account for the reverse cascade — by the time a stage runs, + every downstream stage has already moved its latch on, which has + silently produced a no-op twice. + + - path: "crates/rustyn64-core/**" + instructions: >- + The Bus owns all mutable subsystem state; each chip sees only the narrow + trait it needs. Flag any change that widens a crate's access beyond + that, or that adds a chip-to-chip dependency — the crate graph is + one-directional with exactly ONE permitted edge (`rustyn64-rdp` -> + `rustyn64-cart`, for the `RdramBus` trait), and any other breaks the + fuzz-in-isolation invariant. + + - path: "crates/rustyn64-test-harness/**" + instructions: >- + This is the accuracy oracle. Flag anything that could let it report + success without having run: a pass sentinel set by code that never + executed the tests, a skipped corpus reported as passed, or a silent + `continue` where a load failed. An oracle that runs nothing looks + exactly like one that passes. - path: "crates/**/tests/**" - instructions: | + instructions: >- A test that passes when its fix is reverted is worthless. Flag tests - whose success and failure paths can converge — for example comparing a - total across two runs that differ for an unrelated reason, or asserting - a destination equals a value it already held. + whose success and failure paths can converge — comparing a total across + two runs that differ for an unrelated reason, asserting a destination + equals a value it already held, or checking only that no exception was + raised. Every guard here is expected to have been mutation-checked. - path: "**/*.md" - instructions: | - markdownlint is a pre-commit hook with NO CI job, so it is the one gate - that can silently not run. Any Markdown change must have had - `pre-commit run markdownlint --all-files` run locally — that includes - `AGENTS.md` and `CHANGELOG.md`, not only files under `docs/`. + instructions: >- + markdownlint runs as a pre-commit hook with NO CI job, so it is the one + gate in this repo that can silently not run. Any Markdown change must + have had `pre-commit run markdownlint --all-files` run locally — that + includes `AGENTS.md` and `CHANGELOG.md`, not only files under `docs/`. + The hook pins markdownlint-cli v0.49.1 and the repo carries + `.markdownlint.json`; do not flag rules that configuration does not + enable. - path: "docs/**/*.md" - instructions: | - `docs/STATUS.md` is the single source of truth for counts and state; - flag any number elsewhere that contradicts it. Docs are the spec, so a - behaviour change with an untouched spec is a finding. Claims of the form - "X is undocumented" decay — flag them unless a page reference is cited. - The accuracy ledger is append-mostly: flag a superseded claim that was - edited in place rather than marked resolved. + instructions: >- + Docs are the SPEC here, not a changelog. `docs/STATUS.md` is the single + source of truth for counts and subsystem state; flag any number + elsewhere that contradicts it. A behaviour change with an untouched spec + is a finding. Claims of the form "X is undocumented" decay — flag them + unless a page reference is cited. `docs/accuracy-ledger.md` is + append-mostly: flag a superseded claim that was edited in place instead + of being marked resolved, since the record of wrong turns is the point. - path: "docs/adr/**" - instructions: | + instructions: >- ADRs are immutable once accepted. A superseded ADR must be replaced by a NEW numbered ADR that cross-links it, never rewritten in place. - path: ".github/workflows/**" - instructions: | + instructions: >- Flag any gate whose exit status is piped into `tail`, `grep` or `head` — - the pipeline reports the filter's status, so a failing gate reads as - passing. This has hidden three real failures here. Also flag - `--all-features` on any cargo command: this workspace has - mutually-exclusive backend features and CI must use explicit sets. + a pipeline reports the FILTER's status, so a failing gate reads as + passing. That has hidden three real failures here. Flag `--all-features` + on any cargo command: this workspace has mutually-exclusive backend + features and CI must use explicit sets. Also flag unpinned third-party + actions, missing least-privilege `permissions:` blocks, and secrets + echoed into logs. + + - path: "scripts/**" + instructions: >- + Developer tooling, not shipped product code, but `check_no_roms.sh` and + `check_no_conflict_markers.sh` are COMMIT GATES — a false pass there + ships a commercial ROM into a public repo. Review those two for + correctness of their exit codes above all else. - # Everything below duplicates a gate this repo already runs, and runs harder. - # Leaving them on spends review comments on findings CI has already blocked. tools: - clippy: - enabled: false + # --- Enabled: each covers a gap no local gate fills ------------------- markdownlint: - enabled: false - # Kept on: these cover ground no local gate does. + # ON, unlike clippy below, precisely because markdownlint has NO CI job + # here — it is pre-commit only, so it silently does not run for anyone + # without the hook installed. This is the one linter CodeRabbit adds + # rather than duplicates. + enabled: true + shellcheck: + # Two shell scripts, both commit gates that block committing a ROM. + enabled: true + ruff: + # One Python file: scripts/mirror_n64brew_wiki.py. + enabled: true actionlint: enabled: true + zizmor: + # GitHub Actions security auditing; no local equivalent. + enabled: true yamllint: enabled: true gitleaks: enabled: true - shellcheck: + trufflehog: + enabled: true + osvScanner: + # Dependency vulnerabilities. There is no `cargo audit` job in CI, so + # this is a genuine gap rather than a duplicate. enabled: true + semgrep: + enabled: true + ast-grep: + essential_rules: true + + # --- Disabled: duplicates a gate this repo already runs harder -------- + clippy: + # OFF. CI runs `cargo clippy --workspace --all-targets -- -D warnings` + # with workspace lints at pedantic + nursery, which is a strict SUPERSET + # of default clippy — so this can only repeat findings that already block + # the merge, or contradict a lint the workspace deliberately allows. + enabled: false + opengrep: + # OFF: a semgrep fork, enabled above. Running both duplicates findings. + enabled: false + pylint: + enabled: false # redundant with ruff + flake8: + enabled: false # redundant with ruff - # This project writes its own docs and tests deliberately — rustdoc is a - # blocking gate and every test carries a rationale comment explaining what it - # would catch. Generated stand-ins would have to be rewritten. + # --- Disabled: the language or ecosystem is not present here ---------- + # Footprint is Rust, Markdown, TOML, YAML, shell and one Python file. + # Listed explicitly so this file documents the stack rather than silently + # inheriting every default-on tool. + hadolint: + enabled: false # no Dockerfile + checkmake: + enabled: false # no Makefile + detekt: + enabled: false # no Kotlin + swiftlint: + enabled: false # no Swift + luacheck: + enabled: false # no Lua + clang: + enabled: false # no first-party C/C++ + cppcheck: + enabled: false + pmd: + enabled: false # no Java + fbinfer: + enabled: false + checkov: + enabled: false # no IaC + tflint: + enabled: false + buf: + enabled: false # no protobuf + regal: + enabled: false # no Rego + oasdiff: + enabled: false # no OpenAPI + sqlfluff: + enabled: false # no SQL + squawk: + enabled: false + prismaLint: + enabled: false + phpstan: + enabled: false + phpcs: + enabled: false + phpmd: + enabled: false + rubocop: + enabled: false + brakeman: + enabled: false + biome: + enabled: false # no JS/TS + oxc: + enabled: false + reactDoctor: + enabled: false + emberTemplateLint: + enabled: false + shopifyThemeCheck: + enabled: false + smartyLint: + enabled: false + htmlhint: + enabled: false + stylelint: + enabled: false + dotenvLint: + enabled: false + circleci: + enabled: false # GitHub Actions only + psscriptanalyzer: + enabled: false # no PowerShell + blinter: + enabled: false # no batch files + + # Suggestions are welcome; a bot COMMITTING is not. Every change here goes + # through one conditional gate (fmt + clippy + test + rustdoc + markdownlint + + # ROM/conflict checks) and every guard is mutation-checked before it is kept. + # A bot-authored commit bypasses both, so the write-side features are off. finishing_touches: docstrings: + # rustdoc runs at `-D warnings` with `missing_docs`, so coverage is + # already 100% and generated stand-ins would have to be rewritten: the + # doc comments here carry rationale, not restatement. enabled: false unit_tests: + # Every test here carries a comment explaining what it would catch and is + # verified to fail when its fix is reverted. A generated test satisfies + # neither. + enabled: false + simplify: + # Hot paths are intentionally allocation-free and abstraction-light; an + # auto-simplify suggestion is more likely to fight that than help. + enabled: false + autofix: + enabled: false + fix_ci: + enabled: false + resolve_merge_conflict: enabled: false pre_merge_checks: @@ -169,6 +383,11 @@ reviews: chore, perf, build, ci. description: mode: warning + docstrings: + mode: warning + threshold: 80 + issue_assessment: + mode: warning custom_checks: - name: Oracle number is stated mode: warning @@ -176,28 +395,64 @@ reviews: A change to emulation behaviour should state its measured effect on n64-systemtest's failing-assertion count, or say explicitly that it was not measured. Accuracy claims here are oracle numbers, not - self-assessments. Docs-only, tooling and CI changes are exempt. - - name: Spec updated with behaviour + self-assessments — `docs/STATUS.md` carries the current figure. Pass + for docs-only, tooling and CI changes. + - name: Docs-as-spec sync + mode: warning + instructions: >- + If this PR changes observable behaviour in a chip crate + (rustyn64-cpu, -rsp, -rdp, -audio, -cart, -core) rather than + refactoring internals, the matching docs/.md should change + in the same PR. Fail if a behaviour change has no corresponding docs + change and the PR body does not explain why none is needed. + - name: CHANGELOG entry for user-visible changes + mode: warning + instructions: >- + A user-visible change, new feature or user-facing fix should add an + entry under [Unreleased] in CHANGELOG.md. Pass for pure-internal + refactors, dependency-only bumps with no behaviour change, and + CI/tooling-only changes. + - name: Measured, never tuned mode: warning instructions: >- - A change to a chip's behaviour should touch that chip's doc under - docs/ in the same PR, and user-visible changes should appear in - CHANGELOG.md under [Unreleased]. + Flag any new hardware constant or timing value that is not either + (a) cited to a manual page or wiki article, or (b) recorded in + docs/accuracy-ledger.md with how it was measured. A constant adjusted + until a test ROM passes makes every later result built on it + unfalsifiable. This applies to invented BEHAVIOUR too — an incidental + state change with no cited authority. + - name: unsafe stays out of the chip crates + mode: error + instructions: >- + Every chip crate and rustyn64-core carry #![forbid(unsafe_code)] and + the tree has zero `unsafe`. Fail if this PR adds `unsafe` anywhere + outside rustyn64-frontend, or removes a forbid(unsafe_code) attribute. + A new unsafe block in the frontend must carry an adjacent // SAFETY: + comment stating the invariant relied on. chat: auto_reply: true knowledge_base: + opt_out: false + web_search: + enabled: true + code_guidelines: + enabled: true + filePatterns: + - "AGENTS.md" + - "CONTRIBUTING.md" + - "docs/architecture.md" + - "docs/testing-strategy.md" + - "docs/engineering-lessons.md" + - "docs/accuracy-ledger.md" + # Scoped local rather than auto: this is a public repo, and the hardware + # conventions learned here (notably the inverted NaN classification) are + # correct for the VR4300 and wrong almost everywhere else. Keeping learnings + # local stops them leaking into unrelated reviews. learnings: scope: local issues: scope: local pull_requests: scope: local - code_guidelines: - enabled: true - filePatterns: - - "AGENTS.md" - - "docs/accuracy-ledger.md" - - "docs/engineering-lessons.md" - - "CONTRIBUTING.md" From e90fb44565c4b5b3dda1a508a31ac6e6b3cd2a25 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Mon, 20 Jul 2026 18:47:04 -0400 Subject: [PATCH 5/6] docs(cpu): fp_arith said Cause was bits 16:12 while the mask covers 17: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 --- crates/rustyn64-cpu/src/pipeline.rs | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/crates/rustyn64-cpu/src/pipeline.rs b/crates/rustyn64-cpu/src/pipeline.rs index bb3b06e4..7f843425 100644 --- a/crates/rustyn64-cpu/src/pipeline.rs +++ b/crates/rustyn64-cpu/src/pipeline.rs @@ -1469,10 +1469,18 @@ impl Pipeline { /// /// # `FCSR` /// - /// `Cause` (bits 16:12) reports what *this* operation raised and is replaced - /// each time; `Flags` (6:2) is the sticky accumulation and is OR-ed in. - /// `Flags::to_fcsr_bits` produces both, so clearing only `Cause` before - /// OR-ing preserves the sticky half. + /// `Cause` is bits **17:12** and reports what *this* operation raised; it + /// is replaced wholesale each time. `Flags` (6:2) is the sticky + /// accumulation and is OR-ed in. `Flags::to_fcsr_bits` produces both, so + /// clearing only `Cause` before OR-ing preserves the sticky half. + /// + /// **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 + /// the result was a bit that could never be cleared once raised. Only the + /// five *maskable* conditions live in 16:12; that narrower range is what + /// the enable comparison below uses, and it is a different statement. /// /// # Enabled traps /// From 4e3cd882d8bbbd7ec3f35910c0b3d501d6f17c38 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Mon, 20 Jul 2026 18:51:46 -0400 Subject: [PATCH 6/6] docs: SQRT is unimplemented AND undecoded; downgrade the unsafe check 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 --- .coderabbit.yaml | 8 +++++++- docs/STATUS.md | 8 +++++--- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 9e47da2b..a7510951 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -421,8 +421,14 @@ reviews: until a test ROM passes makes every later result built on it unfalsifiable. This applies to invented BEHAVIOUR too — an incidental state change with no cited authority. + # `warning`, not `error`: an `error`-mode check only blocks when + # `request_changes_workflow` is enabled, which it is not here — so + # declaring it `error` would claim a gate that cannot fire. It does not + # need to. `#![forbid(unsafe_code)]` makes this a COMPILE error in every + # chip crate, which is a harder gate than any bot, and the config's own + # principle is not to duplicate those. - name: unsafe stays out of the chip crates - mode: error + mode: warning instructions: >- Every chip crate and rustyn64-core carry #![forbid(unsafe_code)] and the tree has zero `unsafe`. Fail if this PR adds `unsafe` anywhere diff --git a/docs/STATUS.md b/docs/STATUS.md index 7c12fc69..bce442b0 100644 --- a/docs/STATUS.md +++ b/docs/STATUS.md @@ -125,9 +125,11 @@ and the compares and conversions decode and execute — **all sixteen `C.cond.fmt` tests pass outright**. NaN classification follows the VR4300's inverted convention (ledger C-12), not IEEE-754:2008. -**Only `SQRT` (funct 4) is still implemented-but-undecoded.** The conversions -and the `C.cond.fmt` compares were in that list until this sprint, and `ABS`, -`MOV` and `NEG` before them — `MOV` alone cost ~100 failures, because a +**`SQRT` (funct 4) is the only COP1 operation that is neither decoded nor +implemented**, so it is not an instance of the pattern below. The conversions +and the `C.cond.fmt` compares *were* — implemented in `fpu.rs` and unreachable — +until this sprint, and `ABS`, `MOV` and `NEG` before them; `MOV` alone cost +~100 failures, because a *decoded-but-no-op* instruction is invisible to `cargo test` and the compiler emits one at every FP call boundary. That pattern has now cost two separate investigations; when adding a decode arm, enumerate the neighbouring funct