Skip to content

refactor(core): name the IPL2-exit GPR indices (missed in #175) - #176

Merged
doublegate merged 2 commits into
mainfrom
fix/gpr-constants
Jul 29, 2026
Merged

refactor(core): name the IPL2-exit GPR indices (missed in #175)#176
doublegate merged 2 commits into
mainfrom
fix/gpr-constants

Conversation

@doublegate

@doublegate doublegate commented Jul 29, 2026

Copy link
Copy Markdown
Owner

Correcting a claim I made on #175

I replied to Antigravity's magic-number suggestion saying it was adopted. It
was not.
The script that applied it hit an assertion on a later, unrelated
substitution and exited before writing the file — so none of its earlier edits
were saved either. A separate hand-edit fixed the exclusion table, which made the
resulting diff look like the whole batch had applied.

Caught by grepping main for GPR_T3 after the merge and getting 0.

Oracle number

n64-systemtest: 90 failing suite-wide, Phase 1 categories Failed: 0
unchanged.
Measured, not waived: a rename "obviously cannot" move the oracle,
and "obviously cannot" is how a number goes unchecked.

Also re-ran the local commercial capstone: 5 of 5 folders exercised, including a
CIC-6105 title.

Changes

  • The IPL2-exit GPR names move into rustyn64_cpu::regs::gpr, beside the
    register file they index, rather than staying module-private to boot.rs. A
    second private copy elsewhere is how $t3 and $t4 eventually swap in one of
    them (Antigravity's point, and it is right).
  • The existing s3s7 block is named through the same vocabulary, so no bare
    register indices remain in hle_boot.

What that surfaced

Naming the s3s7 block exposed a duplicate I had introduced and not
noticed
: my gpr::S4 seed wrote r20 = 1, and the s3s7 block three lines
later wrote r20 = 1 again as tv_type. Same register, same value, two writes,
no second source of truth. Removed, with the reason it is absent stated inline so
it does not get "helpfully" restored.

That is the argument for the rename in miniature: with bare indices, write(20, 1) twelve lines apart reads as two unrelated statements.

No behaviour change

Identical register writes. The R-23 seed test asserts the resulting register
values, so it passes either way — which is also why the original omission was
invisible: nothing failed, because nothing was wrong, only unclear.

Gates

cargo fmt --all --check, cargo clippy --workspace --all-targets -- -D warnings,
cargo test --workspace, RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps — each run separately, exit status checked, no pipes. Plus #[ignore]d
n64-systemtest and the commercial capstones.

🤖 Generated with Claude Code

The named-constant change I reported as adopted on #175 never landed. The script
that applied it hit an assertion on a later, unrelated substitution and exited
before writing the file, so none of its earlier edits were saved either. A
separate edit fixed the exclusion table by hand, which made the diff look like
the whole batch had applied.

The seeds now use GPR_AT, GPR_A2, GPR_A3, GPR_T0, GPR_T2, GPR_T3, GPR_S4 and
GPR_RA alongside the existing GPR_SP. GPR_T3 carries its own doc comment naming
what it is for, since it is the register that decides whether CIC-6105 boots.

No behaviour change: the indices are identical, and the R-23 seed test asserts
the resulting register values either way.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The HLE boot seeding path replaces hard-coded register indices with ABI-named constants exported from a new gpr module. The seeded values, control flow, and generated code remain unchanged.

Changes

HLE boot register seeding

Layer / File(s) Summary
GPR register index module
crates/rustyn64-cpu/src/regs.rs
Public gpr module exports named constants for a selected subset of MIPS general-purpose registers, documenting the canonical numbering in one place.
Boot seeding refactor
crates/rustyn64-core/src/boot.rs
Removes the local GPR_SP constant, imports crate::cpu::regs::gpr, and replaces magic-number register indices in the hle_boot IPL3 exit-state sequence with named constants.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

  • doublegate/RustyN64#173: Modifies hle_boot's stack-pointer seeding within the same control flow that this PR refactors to use named constants.
  • doublegate/RustyN64#175: Adjusts the CIC-6105-specific seeded values for the same $at/$a2/$a3/$t0/$t2/$t3/$s4$s7/$ra registers that this PR renames.
🚥 Pre-merge checks | ✅ 9 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Oracle Number Is Stated ⚠️ Warning Emulation code changed, but the PR text gives no n64-systemtest delta or explicit 'not measured' note; docs/STATUS.md says the current count is 90. Add either the measured failing-assertion change or an explicit 'not measured' note, and anchor the current oracle count to docs/STATUS.md.
✅ Passed checks (9 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Docs-As-Spec Sync ✅ Passed boot.rs only swaps magic indices for gpr constants and removes a duplicate S4 write; final register state and flow are unchanged, so no docs update was needed.
Changelog Entry For User-Visible Changes ✅ Passed PASS: The diff is a refactor to named GPR constants and docs only; boot flow and seeded values are unchanged, so the CHANGELOG rule does not require an Unreleased entry.
Measured, Never Tuned ✅ Passed C-32/C-33 already pin the boot seeds; the new gpr module only renames existing ABI indices and adds no tuned constant or timing value.
Unsafe Stays Out Of The Chip Crates ✅ Passed PASS: the diff only refactors GPR constants; no unsafe constructs or forbid(unsafe_code) changes appear, and core/cpu still forbid unsafe.
Title check ✅ Passed It matches Conventional Commits: refactor(core), imperative subject, under 72 chars, and on-topic.
Description check ✅ Passed The description is on-topic and explains the refactor and follow-up fix.

Comment @coderabbitai help to get the list of available commands.

Review follow-up on #176. The constants were module-private to boot.rs, which
invites a second private copy elsewhere - and a second copy is how $t3 and $t4
eventually swap in one of them. They now live in rustyn64_cpu::regs::gpr, beside
the register file they index.

Naming the existing s3-s7 block through the same vocabulary exposed a real
duplicate: my S4 seed wrote r20 = 1, and the s3-s7 block three lines later wrote
r20 = 1 again as tv_type. Same register, same value, two writes, no second source
of truth. The duplicate is removed and the reason it is absent is stated, so it
does not get "helpfully" restored.

Oracle: n64-systemtest unchanged at 90 suite-wide, Phase 1 categories Failed: 0 -
measured, not assumed, though this change emits identical register writes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@doublegate

Copy link
Copy Markdown
Owner Author

Adjudicating all three findings.

CodeRabbit — Oracle Number Is Stated. ADOPTED, and measured rather than waived. I could have argued a rename cannot move the oracle, but the rule exists precisely because "obviously cannot" is how a number goes unchecked. Ran it: 90 failing suite-wide, Phase 1 categories Failed: 0 — unchanged, as expected. Now stated in the commit.

Antigravity — define the GPR constants centrally. ADOPTED. rustyn64_cpu::regs already exists, so module-private constants in boot.rs were the wrong home; a second private copy elsewhere is exactly how $t3 and $t4 eventually swap in one of them. They now live in rustyn64_cpu::regs::gpr, beside the register file they index.

That change immediately paid for itself. Naming the existing s3s7 block through the same vocabulary exposed a duplicate I had introduced and not noticed: my gpr::S4 seed wrote r20 = 1, and the s3s7 block three lines later wrote r20 = 1 again as tv_type. Same register, same value, two writes, no second source of truth. Removed, with the reason it is absent stated inline so it does not get "helpfully" restored. Bare indices 1923 are gone too.

Antigravity nitpick — repeated /// see [GPR_AT] boilerplate. ADOPTED by relocation. The constants now carry real one-line ABI descriptions ($at — assembler temporary, $s6 — saved 6) rather than seven cross-references to a neighbour, which is what the boilerplate was standing in for.

Capstone re-run after the move: 5 of 5 folders exercised, including a CIC-6105 title.

@github-actions

Copy link
Copy Markdown

Antigravity review (Gemini via Ultra)

This PR replaces hardcoded numeric MIPS general-purpose register (GPR) indices in the HLE boot seeding logic with named constants defined in a new gpr module.

Blocking issues

None found.

Suggestions

  • crates/rustyn64-cpu/src/regs.rs (lines 16–41): Defining only a subset of MIPS GPR constants in pub mod gpr creates an incomplete public module that will force future changes to re-add missing registers piecemeal. Define all 32 standard MIPS GPR names (ZERO through RA) upfront so the module serves as a complete reference table.

Nitpicks

  • crates/rustyn64-core/src/boot.rs (lines 158–161): The gpr::S6 register write is formatted across four lines due to line length limits; binding u64::from((seed >> 8) & 0xFF) to a local variable first would keep the register writes visually uniform.

Automated first-pass review by agy on a self-hosted runner -- not a human review.

@doublegate

Copy link
Copy Markdown
Owner Author

The remaining Oracle Number Is Stated warning is stale: CodeRabbit last reviewed at 06:28:13Z, and a PR-body edit does not retrigger it. The body now states exactly what the check asks for:

n64-systemtest: 90 failing suite-wide, Phase 1 categories Failed: 0 — unchanged. Measured, not waived.

anchored against docs/STATUS.md, which records 90 as current. Merging on that basis; everything else is green (CI 9/9, no inline or outside-diff findings).

@doublegate
doublegate merged commit 4ec5714 into main Jul 29, 2026
11 checks passed
@doublegate
doublegate deleted the fix/gpr-constants branch July 29, 2026 06:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant