feat(rdp-gpu): grade the GPU backend against the Angrylion corpus (42/43) - #242
Merged
Conversation
42 of 43 .rvec vectors match on both paths, and the one divergence runs the opposite way from the expected one. `conformance_gpu::census` replays the whole registered conformance corpus through parallel-rdp and grades each vector against **Angrylion's** golden framebuffer -- the same independent oracle the software rasterizer is graded by, not against the software rasterizer's own output. Two implementations agreeing proves nothing about either; two independently matching a third does. That also makes a disagreement diagnosable: software-only means a GPU defect, GPU-only means a software gap, and the census reports which. Reproduced byte-for-byte through the GPU: triangles (flat, shaded, textured, Z-buffered, fractional, negative-slope, right-major), texture loads (Load Tile, Load Block, TLUT, CI4/CI8, 4-bit), the combiner, the blender, dither, tile clamp/mask/mirror/shift, and the 3-point filter. **parallel-rdp does not implement the `key_en` chroma-key alpha compare**, which RustyN64's software rasterizer does (#160). Verified in its source rather than inferred from pixels: `op_set_other_modes` decodes bits 9-19 of `words[0]` and never bit 8 -- there is no `1 << 8` anywhere in the function; `set_color_key` routes `key_center`/`key_scale` only to the combiner's mux inputs, a different feature; `key_width`, which the alpha compare needs, is written and never read; and no shader mentions a key. The gate asserts the EXACT census, so a vector leaving the known-gap set fails as loudly as one joining it -- an unexplained improvement is as much a signal as a regression. Byte order was the hazard to get right, and it fails silently: parallel-rdp stores RDRAM in native little-endian word order (`vram8.data[index ^ 3]`) while RustyN64 stores big-endian bytes, so every crossing reverses each aligned 4-byte group. Pinned by an involution test plus one that fixes the exact permutation, since an 8-byte reversal would also round-trip. `GpuRdp` now OWNS its RDRAM rather than borrowing it. The buffer must be page-aligned or the direct host-import path is silently replaced by a staging copy; a contract a caller can satisfy by accident is worse than no contract, so the caller no longer gets the chance. RDRAM is reached through `with_rdram`/`with_rdram_mut`, which use upstream's `begin_read_rdram`/`end_write_rdram` -- when host import is unavailable the device renders into its own buffer and the shim's allocation is stale, so reading it directly would work on one machine and silently return pre-render bytes on another. One `GpuRdp` per vector, deliberately: a shared context would carry TMEM, tile and combiner state between vectors, and a vector that passes only because its predecessor left the right state behind is not evidence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 10 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe GPU RDP wrapper now owns aligned RDRAM. The test harness replays the ChangesGPU RDP parity
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant GPUConformance
participant GpuRdp
participant Angrylion
CI->>GPUConformance: run corpus census
GPUConformance->>GpuRdp: replay each vector
GpuRdp-->>GPUConformance: return framebuffer
GPUConformance->>Angrylion: compare with golden framebuffer
GPUConformance-->>CI: report exact census
Possibly related PRs
🚥 Pre-merge checks | ✅ 7 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (7 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: 8
🤖 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 `@CHANGELOG.md`:
- Around line 98-107: Update the earlier [Unreleased] changelog entry around the
command processor lifetime description to remove the claim that it borrows an
RDRAM slice or enforces a lifetime parameter. Replace it with the current
GpuRdp-owned-RDRAM design and its with_rdram/with_rdram_mut access model,
avoiding a second corrective entry.
In `@crates/rustyn64-rdp-gpu/shim/prdp_shim.cpp`:
- Around line 88-94: Update the allocation and cleanup paths around
ctx->rdram.ptr and AlignedBuffer::~AlignedBuffer to support MSVC C++17 by using
the platform-appropriate aligned allocation and matching deallocation functions,
including _aligned_malloc and _aligned_free on Windows while preserving the
existing POSIX behavior. If this backend is intentionally POSIX-only instead,
document that constraint in docs/rdp.md.
In `@crates/rustyn64-rdp-gpu/shim/prdp_shim.h`:
- Around line 41-43: Update the return-value documentation for prdp_create in
prdp_shim.h to include rejection when rdram_size is zero or not a multiple of
the required alignment among the conditions that return NULL. Keep the existing
failure conditions and no-throw boundary contract unchanged.
In `@crates/rustyn64-rdp-gpu/src/lib.rs`:
- Around line 224-240: Update with_rdram_mut to inspect the status returned by
prdp_end_write_rdram after the callback completes, returning Some(out) only when
publication succeeds and None when it fails. Preserve the existing null-pointer
handling and callback result behavior.
In `@crates/rustyn64-test-harness/src/conformance_gpu.rs`:
- Around line 10-17: Update the module documentation near the outcome table to
describe four outcomes, adding a row for the `(false, false)`/`neither`
classification recorded by `Census` and `census`. Explain that when neither
software nor GPU matches the golden result, the vector or oracle is suspect,
while preserving the existing interpretations for the other three cases.
In `@crates/rustyn64-test-harness/tests/rdp_conformance_gpu.rs`:
- Around line 69-73: The census test currently enforces only a weak minimum and
does not verify the vectors present in both sets. In
crates/rustyn64-test-harness/tests/rdp_conformance_gpu.rs:69-73, replace the
c.total() threshold with exact assertions for c.total() and c.both.len(), using
the committed values from docs/STATUS.md. In docs/rdp.md:1008-1010, retain the
“exact census, not a threshold” wording after the test enforces those values; no
direct documentation change is otherwise required.
In `@docs/rdp.md`:
- Around line 968-969: Update the “No shared RDRAM” item in docs/rdp.md to
remove the stale claim that the binding borrows a buffer or relies on a lifetime
parameter. Describe the current safety mechanism instead: GpuRdp owns RDRAM
through the backend, and access is coordinated via the with_rdram and
with_rdram_mut handshakes while no machine is running.
- Around line 977-988: Update the stale accuracy-battery count in STATUS.md from
54 to 56 probes, reflecting 43 RDP and 13 VI vectors. Preserve the GPU gate
status as not started and not passed, and run the pinned markdownlint hook
before merging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 42e23943-f2f7-4eaf-b794-1b17a992e8e7
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!Cargo.lock
📒 Files selected for processing (11)
.github/workflows/ci.ymlCHANGELOG.mdcrates/rustyn64-rdp-gpu/shim/prdp_shim.cppcrates/rustyn64-rdp-gpu/shim/prdp_shim.hcrates/rustyn64-rdp-gpu/src/lib.rscrates/rustyn64-rdp-gpu/tests/smoke.rscrates/rustyn64-test-harness/Cargo.tomlcrates/rustyn64-test-harness/src/conformance_gpu.rscrates/rustyn64-test-harness/src/lib.rscrates/rustyn64-test-harness/tests/rdp_conformance_gpu.rsdocs/rdp.md
…nwind Adopts five of six findings from the Antigravity review of #242. **The hardcoded 4096-byte alignment is now queried from the device.** It was the only thing standing between this and a silent fallback on hardware whose `minImportedHostPointerAlignment` exceeds a page: the import would fail, parallel-rdp would stage every access through a copy, and the frame would come out byte-identical with nothing in any return value to say so. Granite does not surface the value, so the shim asks Vulkan directly via `VkPhysicalDeviceExternalMemoryHostPropertiesEXT`; the device now comes up BEFORE the allocation, because the allocation's alignment is a property of it. 4096 remains the floor and the fallback for a device that reports nothing. **`with_rdram_mut` publishes host writes on the unwind path.** `f` can panic — the conformance harness's word swap asserts on a length mismatch, and did — and a panic caught higher up would have skipped `end_write_rdram`, leaving the device rendering from stale memory with no error reported. Now an RAII guard. **The conformance replay writes straight into mapped RDRAM.** It was staging an 8 MiB copy and swapping that across: two full passes and a 16 MiB allocation per vector, 43 times over. The equivalent fusion in the frontend measures 3.3x, so this was not hypothetical. Doing that surfaced a real bug in my own comment: it claimed every vector region is 4-byte aligned, and a committed vector carries a **2-byte** preload. The word-wise swap asserted on it. The regions now go through per-byte `^ 3` helpers, which hold for any address and any length — the assert is what proved the claim wrong, which is the argument for having written it as an assert. Plus the two nitpicks: the backslash continuations inside `echo "::notice::"` were injecting the next line's indentation into the annotation text, and the `* 2` doubleword-to-word conversion is now a named constant. **Rejected: bumping this crate's version for the `GpuRdp::new` signature change.** All twelve crates in this workspace carry a literal `version = "0.8.0"` that is bumped uniformly at release; `rustyn64-rdp-gpu` has never been published, was added in #241 in this same unreleased cycle, and has one in-tree consumer. Bumping it alone would desync the workspace and imply a release that has not happened. The change is recorded in CHANGELOG's `[Unreleased]`, which is this project's stated mechanism for exactly this. Census unchanged at 42/43, and the smoke test still takes the direct-import path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
Adjudication — 5 of 6 adopted, in
|
All eight CodeRabbit findings on #242. Each was right; three were substantive. **`with_rdram_mut` was discarding `end_write_rdram`'s status** — the one failure it exists to detect. A failed publish means host writes never reached the device, and the conformance replay uses exactly this `Some`/`None` to decide a vector was set up, so it would have graded a vector against an unwritten framebuffer instead of reporting `RdramUnmappable`. It also contradicted the contract docs/rdp.md records for this wrapper. The RAII guard now records the outcome in a `Cell` — `drop` cannot return one — and the function returns `None` when it failed, while still publishing on the unwind path. **`std::aligned_alloc` does not exist on MSVC** at any language level: its `free` cannot release an over-aligned block, so the CRT has a separate `_aligned_malloc`/`_aligned_free` pair and mixing them with `free` is undefined. Now behind `#ifdef _MSC_VER`. Untested here, since the `gpu-rdp` CI job is Linux-only, and the comment says so. **The census was documented as exact in two places and asserted as `>= 35` in one.** The threshold was the weaker half: a vector leaving the corpus entirely would have passed while the prose still claimed 42 of 43. Now `EXPECTED_TOTAL` and `EXPECTED_BOTH` are asserted exactly, and both move by hand when a vector is added. Three documentation defects, all of the kind this repo keeps writing lessons about — a claim that was true when written and is not now: - The CHANGELOG's `[Unreleased]` carried the #241 entry saying the wrapper "ties the command processor's lifetime to a borrowed RDRAM slice" two paragraphs from the entry reversing exactly that. Neither has shipped, so the release notes would have carried both. Amended in place rather than corrected twice. - The parity module's table listed three outcomes while `Census` has four buckets. The missing row is the one a reader most needs: neither path reproduces, which indicts the vector or the oracle rather than either rasterizer. - docs/rdp.md's "No shared RDRAM" item still described the borrow-based mechanism. The substance held; only the stated mechanism was stale. Plus the header's NULL list, which omitted the size-validation rejection — and that header is explicitly held correct by review rather than by a generator, so an incomplete contract there is the whole risk. Census unchanged at 42/43. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-test-harness/src/conformance_gpu.rs`:
- Around line 77-96: Add a dedicated round-trip test for the write_region and
read_region helpers, covering both ordinary data and a 2-byte unaligned/preload
case; write source bytes into a test RDRAM buffer, read them back at the same
logical address, and assert the result matches the source.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 13c3d1b8-bea0-4bd3-a323-f048a8d4774e
📒 Files selected for processing (4)
.github/workflows/ci.ymlcrates/rustyn64-rdp-gpu/shim/prdp_shim.cppcrates/rustyn64-rdp-gpu/src/lib.rscrates/rustyn64-test-harness/src/conformance_gpu.rs
… pair The `^ 3` region helpers carry the general case — any address, any length, including the 2-byte preload that broke the word-wise path — and had no direct test while `swap_words_into` had two. CodeRabbit's point. Two tests, because a round trip alone is not enough: `write_region` and `read_region` are mutual inverses, so **two consistently wrong functions round trip perfectly**. The second test asserts they agree with `swap_words_into` where both apply, which is what actually pins the permutation. Mutation-checked, and the pair is what makes it worth stating: changing `^ 3` to `^ 1` in `write_region` alone fails two tests; changing it in BOTH — still a perfect involution — fails exactly one, the cross-mechanism one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`(i + 1) as u8` tripped `clippy::cast_possible_truncation`. It could not actually truncate — `len` maxes at 9 — but the gate does not reason about that and should not have to. Landed red because the previous commit's gate ran in a block separated from the commit by `;` rather than `&&`, so a failing lint did not stop the commit. That is the same shape as piping a gate into `tail`: the check ran, its answer was discarded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Antigravity review (Gemini via Ultra)This PR adds a GPU RDP conformance gate to grade the Blocking issues
Suggestions
Nitpicks
Automated first-pass review by |
doublegate
added a commit
that referenced
this pull request
Aug 1, 2026
…olds Three more findings on #244, all correct. **The gate could pass on an incomplete run.** The corpus check accepted `>= 35` vectors while claiming 43, and the sequence comparison used `zip`, which stops at the shorter side — so a second run producing fewer frames would have compared only the prefix and passed. The corpus count is now asserted against `RDP_VECTORS.len()` rather than a literal, so it cannot drift from the registry, and the second sequence's length is asserted before the `zip`. That is the second time in two PRs that a `>=` threshold sat beside prose claiming an exact number. The threshold is always the weaker half of the pair. **The CHANGELOG contradicted itself inside `[Unreleased]`**: the #243 entry still said the GPU path "makes no determinism claim of its own" a few paragraphs below the entry making one. Neither has shipped, so the release notes would have carried both. The older entry now points at ADR 0015 instead of asserting the negative — the same amendment-rather-than-append treatment the borrowed-RDRAM entry got in #242. Plus a comma before an essential `because` clause in the ADR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
"Does the GPU RDP have all the features and accuracy of the software one?" is a
question that needs a gate, not an opinion. This builds it — and, per this
project's own rule, builds it before the thing it grades (the Bus wiring, which
is the next PR).
The corpus already existed: 43 registered
.rvecvectors whose goldens areAngrylion's output, license-clean, and already the oracle the software
rasterizer is graded against.
The result
42 of 43 vectors match Angrylion on both paths.
Reproduced byte-for-byte through parallel-rdp: triangles (flat, shaded,
textured, Z-buffered, fractional, negative-slope, right-major), texture loads
(Load Tile, Load Block, TLUT, CI4/CI8, 4-bit), the combiner, the blender,
dither, tile clamp/mask/mirror/shift, and the 3-point filter.
The one divergence runs the opposite way
tex_tri_chromakey_alpha_16. parallel-rdp does not implement thekey_enchroma-key alpha compare — the feature RustyN64 added in #160 and which matches
Angrylion. This is checkable in its source, not inferred from pixels:
op_set_other_modes(rdp_device.cpp) decodes bits 9–19 ofwords[0]andnever bit 8, which is
key_en. There is no1 << 8anywhere in thefunction.
op_set_key_r/op_set_key_gbdo store the key — butRenderer::set_color_keyroutes
key_center/key_scaleto the combiner's mux inputs (theKEY_CENTER / KEY_SCALE sources, a different feature).
key_width, which thealpha compare needs, is written and never read.
parallel-rdp/shaders/mentions a key at all.The diagnostic that made this obvious: the same 27 pixels are non-zero on both
paths — so rasterization and coverage agree exactly — and only the colour differs
(
0x4321vs0x0001, i.e. the shade colour vs black with alpha). A coveragematch with a colour miss points at the combiner, not the rasterizer.
So the premise that a GPU backend needs catching up to the software one is not
right in either direction: parallel-rdp is more complete almost everywhere and
less complete here.
Design
Graded by the oracle, not by each other. Software-only ⇒ a GPU defect;
GPU-only ⇒ a software gap; the census names which vectors fall where, so a
disagreement is diagnosable rather than merely alarming.
Asserts the exact census, not a threshold. A vector leaving the known-gap
set fails as loudly as one joining it — an unexplained improvement is as much a
signal as a regression.
Cannot pass vacuously. No Vulkan device ⇒ it says, by name, that it verified
nothing. The CI step fails if the test neither grades nor reports a skip.
One
GpuRdpper vector. A shared context would carry TMEM, tile and combinerstate across vectors, and a vector that passes only because its predecessor left
the right state behind is not evidence of anything. Costs a device init per
vector; buys independence.
Byte order — the silent hazard
parallel-rdp stores RDRAM in native little-endian word order
(
vram8.data[index ^ 3]throughout its shaders); RustyN64 stores big-endianbytes, and the
.rvecgoldens are in RustyN64's layout. Every crossing reverseseach aligned 4-byte group. Getting it wrong renders right-shaped/wrong-coloured
output rather than failing.
Pinned two ways: the swap is an involution (so one function serves both
directions), and the exact permutation is fixed — because an 8-byte reversal
would also round-trip and pass an involution test alone.
GpuRdpnow owns its RDRAMThe buffer must be page-aligned or the direct host-import path is silently
replaced by a staging copy (#241 shipped exactly that bug). A contract a caller
can satisfy by accident is worse than no contract, so the caller no longer gets
the chance to get it wrong — the shim allocates and aligns it.
RDRAM is reached through
with_rdram/with_rdram_mut, which use upstream'sbegin_read_rdram/end_write_rdram. That is not ceremony: when host import isunavailable the device renders into its own buffer and the shim's allocation
is stale, so reading it directly would work on a machine where the import
succeeded and silently return pre-render bytes on one where it did not.
What this is NOT
Still no Bus integration, no shared RDRAM with a running machine, no frontend
feature, no dirty-region sync and therefore no ADR 0004 determinism claim. That
is the next PR;
docs/rdp.mdkeeps the list current.Gates run locally
cargo fmt --all --check;cargo clippy --workspace --all-targets -- -D warnings; clippy withgpu-rdpon bothrustyn64-rdp-gpuandrustyn64-test-harness;cargo test --workspace; bothgpu-rdptest suites;RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps;scripts/check_en_us.sh;pre-commit run markdownlint --all-files; theno_stdthumbv7em build. All green, one conditional, no pipes.
🤖 Generated with Claude Code