Phase 2 Sprint 2: the RSP vector unit (register file, accumulator, multiplies, vector memory) - #37
Merged
Merged
Conversation
The first tranche of the vector unit: the 32x8x16-bit register file addressed by byte, the three control registers, and the four COP2 moves. The COP2 opcode carries two instruction groups, separated by word bit 25 -- the top bit of the `rs` field. That bit also changes what the element field *means*: a byte offset for a move, a broadcast modifier for a computation. Addressing by byte rather than by lane is what makes three hardware rules expressible, and all three agree with a lane model at even offsets, so a test using only aligned offsets cannot distinguish them: - an odd offset straddles two lanes; - `MTC2` at byte 15 writes one byte, from rt[15..8], and does not wrap; - `MFC2` at byte 15 wraps its second byte to byte 0 of the same register. The asymmetry in the last two is deliberate on hardware, not an oversight in one of them. n64-systemtest: 250 -> 247. The computational instructions, the 48-bit accumulator, clamping and the VRCP/VRSQ tables are the rest of the sprint; COP2 computations still retire inertly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
VMULF/VMULU, VMUDL/VMUDM/VMUDN/VMUDH, VSAR and the six bitwise operations execute. The accumulating VMAC*/VMAD* forms, the compares, the selects and VRCP/VRSQ do not yet and report so -- `vu_compute` returns false and the instruction retires inertly rather than writing a wrong result. The accumulator is one 48-bit register per lane, not three 16-bit ones. VSAR slicing it into ACC_HI/ACC_MD/ACC_LO invites the latter, but the multiplies write across all 48 bits and the extraction producing `vd` reads a 32-bit window spanning two slices, so split storage would lose the carries between them. VMULF's rule was **derived from n64-systemtest's own expected vectors** rather than recalled: acc = 2*vs*vt + 0x8000, with vd a signed clamp of acc >> 16. Working it from lane 4 (0x8001 * 0x8000 -> acc 0x0000_7FFF_8000) gives the doubling and the rounding constant; lane 7 gives the clamp. The constant lands in the accumulator rather than only in the extracted value, which is visible solely because the suite reads ACC_LO back as 0x8000 for a zero product -- the result alone cannot distinguish the two. Unsigned clamping saturates at a **15-bit** threshold to a 16-bit value: above 0x7FFF becomes 0xFFFF. A naive `> 65535` test passes 0x8000..=0xFFFF through unchanged. The computational and move groups share the COP2 opcode, split by word bit 25, and the computational operand fields are NOT in the usual MIPS positions -- vt at 20..16, vs at 15..11, vd at 10..6 -- so reading it as an ordinary R-type swaps source and destination. **Not yet observable through the oracle.** Every VU test in n64-systemtest loads its operands with LQV and reads results back with SQV, and the vector load/store family is Sprint 3, so the suite count is unchanged at 247. These instructions are pinned only by unit tests against the oracle's published vectors, which is weaker evidence than a passing suite and is recorded as such in docs/rsp.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
LBV/LSV/LLV/LDV and their stores, plus LQV/SQV. n64-systemtest: 247 -> 224.
This is what makes the vector unit **observable**. Every VU test in the suite
loads its operands with LQV and reads results back with SQV, so the multiply
family committed previously could not move the count at all until these landed
-- the 23 tests recovered here are largely those, now verifiable against the
oracle rather than against unit tests alone.
Two encoding traps, both of which produce plausible-looking wrong behaviour:
- The offset is a **signed 7-bit** field scaled by the access size, not the
16-bit immediate an ordinary load carries. Reading `imm` whole gives a
wildly wrong address.
- `element` is a **byte** index naming the first byte touched, so a non-zero
element moves *fewer* bytes rather than shifting a full-width window.
A misaligned LQV runs only up to the next 16-byte boundary rather than crossing
it -- which is exactly why LRV exists. An implementation that simply reads 16
bytes from the address passes an aligned test and fails a misaligned one, so
both cases are pinned.
LRV/SRV, the packed LPV/LUV/SPV/SUV, the strided LHV/LFV/SHV/SFV and the
transposing LTV/STV/LWV/SWV report unimplemented and retire inertly rather than
moving approximately-right bytes.
Golden log holds its 0-diff; Phase 1 categories still 0.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe RSP now exposes VU control state and scalar access, executes COP2 moves and selected computational operations, and supports selected vector loads and stores. Tests and documentation cover register moves, accumulator behaviour, clamping, routing, and memory boundaries. ChangesRSP vector unit execution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Rsp
participant su_step
participant cop2
participant vu_compute
participant vector_mem
su_step->>cop2: dispatch COP2 instruction
cop2->>vu_compute: execute selected computation
vu_compute-->>Rsp: write vector result and accumulator
su_step->>vector_mem: dispatch vector load/store
vector_mem-->>Rsp: transfer bytes through DMEM
Possibly related PRs
🚥 Pre-merge checks | ✅ 6 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (6 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/rustyn64-rsp/src/vu.rs`:
- Around line 315-324: Add an instruction-level effect test covering the VMULU
encoding (op == 0x01) in the existing vector-operation test suite, using the
established decoded-effect/oracle pattern and a case that verifies unsigned
write-back through clamp_unsigned. Keep the existing VMULF coverage unchanged
and place the test alongside neighboring encodings.
In `@docs/rsp.md`:
- Around line 223-227: Rewrite the paragraph in docs/rsp.md to state that
LQV/SQV are implemented and make computational results observable, removing the
obsolete Sprint 3 and “not observable” framing. Replace “suite count is
unchanged” with the measured failing-assertion/suite-count change from 247 to
224, matching docs/STATUS.md as the source of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3c5521fe-a32c-472c-873b-e5e97dbe5448
📒 Files selected for processing (4)
crates/rustyn64-rsp/src/lib.rscrates/rustyn64-rsp/src/su.rscrates/rustyn64-rsp/src/vu.rsdocs/rsp.md
…laim Both adopted from CodeRabbit review comments on #37. VMULU shares the entire accumulator path with VMULF and differs only in which clamp the write-back uses, so `clamp_unsigned` being correct as a helper said nothing about the `op == 0x01` arm selecting it -- a mis-selection would have hidden behind the VMULF coverage. The new test uses the oracle's own vectors; lane 5 discriminates by flooring a negative accumulator to 0 where VMULF gives 0x8001, and lane 7 by saturating to 0xFFFF where VMULF gives 0x7FFF. ACC_LO is asserted identical to VMULF's, which is the other half of the claim. Mutation-checked: collapsing the arm to clamp_signed turns the VMULU test red and leaves the VMULF one green. The docs paragraph was accurate when the multiply commit landed and false two commits later: it still said the vector load/store family was Sprint 3 and the suite count unchanged, while the same file listed LQV/SQV as implemented. Now states what happened -- the computational group moved the count by zero until the load/store group existed, then 247 -> 224 in one step -- while keeping the epistemic point that unit tests against published vectors are weaker evidence than a passing suite. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Pushed unformatted in the previous commit: I ran the gate and the commit as separate commands, so the commit did not wait on the gate's result. The gate is only a gate if the next step is conditional on it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
crates/rustyn64-rsp/src/vu.rs (4)
345-350: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winExtract the VMUDH partial product, not the full product.
This stores
(vs * vt) << 16and then clampsacc >> 16, exposing the entire 32-bit product. VMUDH must retain the high partial product in the accumulator and returnACC[31..16]; for example, a product of1 * 2must not return2. The SGI/Nintendo guide specifies this operation explicitly. (ultra64.ca)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/rustyn64-rsp/src/vu.rs` around lines 345 - 350, Update the VMUDH case in the opcode dispatch to store only the high 16-bit partial product in the accumulator and return ACC[31..16], rather than shifting and exposing the full product; ensure inputs such as 1 × 2 produce zero.
575-580: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSeed sentinel state in both inert-path tests.
Rsp::new()leaves the destination and memory zeroed, so asserting zero does not prove that an unimplemented instruction preserved state. Initialise non-zero destination/memory sentinels and assert they remain unchanged.As per coding guidelines: “tests must seed destinations with values differing from expected results”. As per path instructions: tests must prove inert handlers do not silently pass.
Also applies to: 749-754
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/rustyn64-rsp/src/vu.rs` around lines 575 - 580, Update both inert-path tests, including an_unimplemented_opcode_is_reported_not_guessed and the test around the other referenced range, to seed non-zero destination register and memory sentinel values before invoking the opcode. Assert those sentinels remain unchanged afterward, while preserving the existing assertions that the handlers report the operation as unimplemented.Sources: Coding guidelines, Path instructions
302-304: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSnapshot
vtbefore writingvd.Line 304 reads from the live
vtregister while Line 378 commits each lane immediately. Withvd == vtand any broadcast element, an earlier lane overwrites the broadcast source and later lanes consume the wrong value. Snapshot the source register or compute all eight results before committing writes, then add an aliasing broadcast test.Also applies to: 378-378
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/rustyn64-rsp/src/vu.rs` around lines 302 - 304, Update the vector operation around the lane loop and its immediate writes at the commit site to snapshot all source values from vt before writing vd, ensuring vd == vt with broadcast elements uses the original register value for every lane. Preserve existing lane computation, then add a test covering aliasing broadcast behavior.
613-625: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not wrap scalar loads past byte 15.
The load path always processes
sizebytes and applies(element + i) & 15; anLLVorLDVnear the end of the register therefore overwrites bytes belowelement. This contradicts the documented byte-window contract indocs/rsp.mdLines 246-247. Bound loads to the remaining bytes; keep store wrapping separate if required by the hardware rule, and add anelement=14regression test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/rustyn64-rsp/src/vu.rs` around lines 613 - 625, The load branch in the scalar transfer logic must not wrap vector-register byte indices past byte 15. In the `store` conditional around `vu_byte` and `set_vu_byte`, bound load iteration to the bytes remaining from `element` through 15 while preserving the existing store wrapping behavior, and add a regression test covering an `element` value of 14 for LLV/LDV.docs/rsp.md (1)
173-190: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd exact provenance for the new hardware rules.
The move edge cases, accumulator model, VMULF formula, and vector-memory semantics are stated as hardware facts, but the text does not provide an exact manual/wiki citation or a named oracle test/matrix with an explicit “measured” designation. Add that provenance before treating this page as the specification.
As per coding guidelines: every new hardware behaviour must cite a manual/wiki source or
docs/accuracy-ledger.md. As per path instructions: undocumented claims must cite the primary pages reviewed. Based on learnings: name the exact test/matrix and mark oracle-derived behaviour as measured.Also applies to: 202-221, 241-253
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/rsp.md` around lines 173 - 190, Add provenance citations for the hardware behaviors documented in the move section and the related accumulator, VMULF, and vector-memory sections. For each claim, cite the exact manual/wiki page or the corresponding entry in docs/accuracy-ledger.md; for oracle-derived behavior, name the precise test or matrix and explicitly mark it as measured. Apply the same documentation standard to the sections referenced around the accumulator and vector-memory content, without changing the stated semantics.Sources: Coding guidelines, Path instructions, Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/rustyn64-rsp/src/vu.rs`:
- Around line 345-350: Update the VMUDH case in the opcode dispatch to store
only the high 16-bit partial product in the accumulator and return ACC[31..16],
rather than shifting and exposing the full product; ensure inputs such as 1 × 2
produce zero.
- Around line 575-580: Update both inert-path tests, including
an_unimplemented_opcode_is_reported_not_guessed and the test around the other
referenced range, to seed non-zero destination register and memory sentinel
values before invoking the opcode. Assert those sentinels remain unchanged
afterward, while preserving the existing assertions that the handlers report the
operation as unimplemented.
- Around line 302-304: Update the vector operation around the lane loop and its
immediate writes at the commit site to snapshot all source values from vt before
writing vd, ensuring vd == vt with broadcast elements uses the original register
value for every lane. Preserve existing lane computation, then add a test
covering aliasing broadcast behavior.
- Around line 613-625: The load branch in the scalar transfer logic must not
wrap vector-register byte indices past byte 15. In the `store` conditional
around `vu_byte` and `set_vu_byte`, bound load iteration to the bytes remaining
from `element` through 15 while preserving the existing store wrapping behavior,
and add a regression test covering an `element` value of 14 for LLV/LDV.
In `@docs/rsp.md`:
- Around line 173-190: Add provenance citations for the hardware behaviors
documented in the move section and the related accumulator, VMULF, and
vector-memory sections. For each claim, cite the exact manual/wiki page or the
corresponding entry in docs/accuracy-ledger.md; for oracle-derived behavior,
name the precise test or matrix and explicitly mark it as measured. Apply the
same documentation standard to the sections referenced around the accumulator
and vector-memory content, without changing the stated semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e5adf0fc-c60b-4781-a8a3-d62da0cc8730
📒 Files selected for processing (2)
crates/rustyn64-rsp/src/vu.rsdocs/rsp.md
This was referenced Jul 21, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Phase 2 Sprint 2: the vector unit. Three commits — the register file and SU/VU moves, the accumulator and multiply family, and the vector load/store group that makes all of it observable.
Why the load/store group came before the rest of the VU
Committing the multiply family alone moved the suite count by zero, and I checked why rather than assuming: every VU test in n64-systemtest loads its operands with
LQVand reads results back withSQV. Until those existed the entire computational group was unreachable from the oracle, and was pinned only by unit tests I wrote from the oracle's published vectors — weaker evidence, recorded as such indocs/rsp.mdat the time rather than presented as verified.Landing
LQV/SQVthen recovered 23 tests in one step. That ordering is the interesting part of this PR.Deriving VMULF rather than recalling it
The rule came out of n64-systemtest's own expected vectors. Lane 4 (
0x8001 × 0x8000→ acc0x0000_7FFF_8000) gives the doubling and the rounding constant; lane 7 gives the clamp:acc = 2·vs·vt + 0x8000, with
vda signed clamp ofacc >> 16.Both
VMULFandVMUDLthen matched the published result and all three accumulator slices first time, which is the useful confirmation — a wrong derivation shows up immediately in the ACC vectors.Two things that are easy to get wrong and are pinned separately:
ACC_LOback as0x8000for a zero product — the result alone cannot distinguish the two.VSARslicing it intoACC_HI/ACC_MD/ACC_LOinvites the latter, but the extraction producingvdreads a 32-bit window spanning two slices, so split storage loses the carries.Unsigned clamping saturates at a 15-bit threshold to a 16-bit value: anything above
0x7FFFbecomes0xFFFF. A naive> 65535test passes0x8000..=0xFFFFthrough unchanged.Encoding traps
The COP2 opcode carries two instruction groups split by word bit 25, and that bit also changes what the element field means — a byte offset for a move, a broadcast modifier for a computation. The computational operands are also not in the usual MIPS positions (
vtat 20..16,vsat 15..11,vdat 10..6), so reading it as an ordinary R-type swaps source and destination.For loads/stores, the offset is a signed 7-bit field scaled by access size, not the 16-bit immediate an ordinary load carries.
Addressing vector registers by byte rather than by lane is what makes three hardware rules expressible, and all three agree with a lane model at even offsets — so an aligned-only test cannot distinguish them:
MTC2offset straddles two lanes;MTC2at byte 15 writes one byte and does not wrap;MFC2at byte 15 wraps its second byte to byte 0.That last asymmetry is deliberate on hardware, not an oversight in one of them.
A misaligned
LQVruns only to the next 16-byte boundary rather than crossing it — which is the entire reasonLRVexists. Both cases are pinned; an implementation that simply reads 16 bytes passes the aligned test.What is deliberately absent
Everything unimplemented reports rather than approximating:
vu_computeandvector_memreturnfalseand the instruction retires inertly, which is what hardware does with an opcode it does not implement (there is no exception mechanism to report it with).Still to come: the
VMAC*/VMAD*accumulating forms, the compare/select group,LRV/SRV, the packed and strided loads, the transposingLTV/STV, and theVRCP/VRSQROM tables — which must be table data and never computed.Verification
Gates on the final commit:
cargo fmt --all --check,cargo clippy --workspace --all-targets -- -D warnings,cargo test --workspace,RUSTDOCFLAGS="-D warnings" cargo doc, theno_stdbuild,pre-commit run markdownlint --all-files, plus both#[ignore]d oracles.🤖 Generated with Claude Code