feat(cpu): loads, stores, and the unaligned family - #6
Merged
Conversation
T-11-003. The CPU can now reach memory. mem.rs holds the data shaping as pure byte-level transforms -- no bus, no registers -- so the fiddly parts are testable without a machine. The LW/LWU distinction is called out because it is easy to lose: LW sign-extends its 32-bit result into the 64-bit register and LWU does not. Confusing them silently breaks any address above 2 GiB and nothing else. The unaligned family is where the real difficulty is. MIPS has no unaligned load; LWL/LWR (and LDL/LDR) are used AS A PAIR to assemble an unaligned value from two aligned accesses, each merging part of the addressed word into the destination while preserving the rest. Every shift direction here is decided by the N64 being big-endian, and getting one backwards yields plausible-looking values that are wrong only at unaligned addresses -- a miserable bug to find later. So they are tested as a PAIR, the way they are actually used, plus a store-then-load round trip across every byte offset. That round trip is the strongest available statement that the shift directions agree with each other, and it is what told me my SWR test assertion was wrong rather than the implementation: I had asserted SWR merges rt's low bytes into memory's TOP bytes, when it fills backward from the addressed byte to the word start. Checked by hand against the byte layout before changing anything. EX resolves the effective address (base + SIGN-extended offset) and DC performs the access. That split is the whole point of the pipeline -- DC is the cycle the scheduler interleaves the RCP around (ADR 0007) -- and it is why the address computation and the access are in different modules. An unaligned access in an aligned form raises AddressError. The LWL/LWR family is exempt by construction, since being usable at any byte offset is precisely why it exists. THE LOAD-DELAY INTERLOCK IS LIVE. It has had a `load_interlocks` predicate since PR #2 with nothing to interlock against; loads give it one. It reproduces the hardware's documented imprecision -- matching the rs or rt encoded field whether or not it is used as a source, exempting $zero, not crossing GPR/FPR. One real bug found by its integration test: I compared against the `rf_ex` latch, but in the reverse cascade EX runs BEFORE RF, so by the time rf_stage executes the instruction that was in EX has already moved to `ex_dc` and `rf_ex` has been vacated. The check therefore silently never fired. Now compares `ex_dc`, with the reasoning recorded at the site so it does not get "simplified" back. Mutation-tested: disabling the load check fails the test. Two existing tests had premises invalidated -- they asserted LW and opcode 0o77 decode to Reserved, which stopped being true the moment loads and stores landed. Repointed at genuinely unimplemented encodings (BEQ, which arrives in T-11-004, and the unused SPECIAL funct 0o01) rather than deleted. Gate: fmt, clippy -D warnings (0), 106 tests (was 94), rustdoc -D warnings, no_std cross-build. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements memory load and store instructions (T-11-003), including aligned loads/stores, the unaligned load/store family (LWL/LWR/LDL/LDR/SWL/SWR/SDL/SDR), and the load-delay interlock. It introduces a new mem module for data shaping and integrates memory operations into the pipeline's EX and DC stages, supported by comprehensive unit and integration tests. Feedback suggests optimizing the read_width and write_width helper functions in pipeline.rs to utilize the bus's 32-bit read/write paths for 4-byte and 8-byte accesses instead of performing byte-by-byte loops, which introduces a performance bottleneck.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Pull request overview
This PR implements the VR4300’s first end-to-end memory reachability: decoding/exec wiring for loads/stores (including the unaligned LWL/LWR/LDL/LDR + SWL/SWR/SDL/SDR family), a byte-precise shaping layer, and DC-stage bus access consistent with the five-stage pipeline split (ADR 0007). It also activates the load-delay interlock using the correct latch under the reverse cascade.
Changes:
- Add
mem.rsto centralize load/store width/signedness shaping and unaligned merge transforms as pure byte-level operations. - Introduce
MemOpfromEX→DC, and implementDC-stage memory accesses plus address-error behavior for misaligned aligned-form accesses. - Extend decode/execute and add integration tests covering loads/stores, unaligned pairs, and the load-delay interlock; update changelog accordingly.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/rustyn64-cpu/src/pipeline.rs | Adds MemOp plumbing, DC-stage memory access, load-delay interlock latch fix, and end-to-end RAM-backed tests. |
| crates/rustyn64-cpu/src/mem.rs | Implements pure byte-level shaping + unaligned load/store merge transforms with unit tests. |
| crates/rustyn64-cpu/src/lib.rs | Exposes the new mem module and re-exports LoadKind/StoreKind. |
| crates/rustyn64-cpu/src/exec.rs | Adds MemOp and wires loads/stores/unaligned ops to emit EX→DC memory requests. |
| crates/rustyn64-cpu/src/decode.rs | Adds opcode decoding for loads/stores and marks loads for the load-delay interlock. |
| CHANGELOG.md | Documents the new CPU memory reachability and the now-live load-delay interlock. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Adopts Gemini's comment on PR #6, with its signature change declined. read_width/write_width looped byte-by-byte for every access, so a 4-byte load issued four read_u8 calls and an 8-byte load eight -- bypassing Bus::read_u32 entirely, which rustyn64-core overrides with a fast RDRAM path. Those are the two most common memory operations, and memory access is the hot path for a core whose stated goal is sustained full speed (docs/performance.md). 4-8x more bus calls than necessary on the common case is worth fixing before the pattern spreads. Now dispatched on width: read_u8 for 1-2 bytes, read_u32 for 4, and two read_u32 calls for 8 (big-endian, high word at the lower address). NOT adopted: the suggestion also moved alignment checking into these helpers, returning Option/bool. already validates alignment against the specific LoadKind/StoreKind before calling them, and the unaligned family passes an address it aligned down itself. Adding a second check would put the rule in two places where it can drift, and the width-based check proposed is weaker than the kind-based one that already exists -- LoadKind::SignedByte and LWL want different answers for the same width. The reasoning is recorded at the function so the absence of a check there does not read as an oversight. Verified by the existing round-trip tests, which would catch an endianness error in the new 8-byte path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
T-11-003. The CPU can now reach memory.
What landed
mem.rs— data shaping as pure byte-level transforms. Width/signedness rules, alignment requirements, and theLWL/LWR/LDL/LDR+SWL/SWR/SDL/SDRmerges.EXresolves the effective address;DCperforms the access. That split is the point of the pipeline —DCis the cycle the scheduler interleaves the RCP around (ADR 0007).AddressError. TheLWL/LWRfamily is exempt by construction.The unaligned family is the hard part
MIPS has no unaligned load —
LWL/LWRare used as a pair to assemble a value from two aligned accesses. Every shift direction is decided by the N64 being big-endian, and getting one backwards yields plausible values that are wrong only at unaligned addresses.So it's tested as a pair, plus a store-then-load round-trip across every byte offset. That round-trip is what told me my
SWRassertion was wrong rather than the implementation — I'd asserted it mergesrt's low bytes into memory's top bytes, when it fills backward from the addressed byte to the word start. Verified by hand against the byte layout before touching anything.The load-delay interlock is live
It's had a
load_interlockspredicate since #2 with nothing to interlock against. Loads give it one, and the integration test immediately found a real bug: I compared against therf_exlatch, but in the reverse cascadeEXruns beforeRF— so by the timerf_stageexecutes, that instruction has moved toex_dcandrf_exis vacated. The check silently never fired. Now comparesex_dc, with the reasoning recorded at the site. Mutation-tested.Two tests repointed, not deleted
They asserted
LWand opcode0o77decode toReserved— no longer true once loads and stores exist. Repointed at genuinely unimplemented encodings (BEQ, arriving in T-11-004, and the unused SPECIAL funct0o01).Gate
fmt,clippy -D warnings(0), 106 tests (was 94),rustdoc -D warnings,no_stdcross-build.