Repository navigation
Upstream PR batch 1 U6: shader semantics correctness (ALU, LDS, export) - #481
Conversation
Ported from upstream PR boykopovar#5081 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Ported from upstream PR boykopovar#5214 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Ported from upstream PR boykopovar#5110 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Ported from upstream PR boykopovar#5117 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Ported from upstream PR boykopovar#5426 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Ported from upstream PR boykopovar#4996 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…reference Ported from upstream PR boykopovar#5450 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Ported from upstream PR boykopovar#5471. The Vop3InterpDecode unit-test hunk is not ported (that test file is not on main); the execution test InterpolationF16 carries the coverage. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…_div_scale masks Ported from upstream PR boykopovar#5162 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…t_f32_f64 Ported from upstream PR boykopovar#5159 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Ported from upstream PR boykopovar#5160. Conflicts resolved against main: kept main's barrier condition for non-LDS shared atomics; the unit-test hunk for SRT read patching (a different upstream change, not on main) is dropped. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Ported from upstream PR boykopovar#5492 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Ported from upstream PR boykopovar#5498 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ing and round the array layer half to even in color compare emulation Ported from upstream PR boykopovar#907 (follow-up to its predecessor). Hand-applied to main's reworked emitter: NOT_EQUAL uses OpFUnordNotEqual, the array layer is clamp(roundEven(z)), and ResourceMaterializer rejects TRUNC_COORD with point filtering. The upstream execution-test rewrite is not ported (it targets a diverged test file). Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The ported DS fixes add MemoryInfo::flushDenormalResults (now encoded in the plan) and new IR opcodes, so cached payloads from format 34 must not load. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…device shim The upstream execution tests for the ported fixes compile unchanged against a Metal stand-in for the Vulkan test device (guest buffer descriptors are mirrored into shared MTLBuffers). Covers boykopovar#5162 (VALU carry-out masks), boykopovar#5159 (v_cvt_f32_f64 result modes), boykopovar#5492 (ds_add_f32) and boykopovar#5498 (DS src2 lane order). The LdsSrc2 and LdsSrc2Wide tests use device-memory LDS, which the Metal bridge rejects, and are not registered. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…l build Registers the upstream tests for boykopovar#4996, boykopovar#5426 and boykopovar#5450 (already on main's tree but not built for Metal) and a high-half decode check for boykopovar#5471. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
… Metal recompiler Standalone copy of the boykopovar#5160 verifyFunctionLdsUpdates check. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Buffer-store variant of the boykopovar#5214 execution test. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Registers boykopovar#5117's upstream test through the device shim. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Guest-replay draw for boykopovar#5110: alpha comes from export channel 1. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Guest-replay draw for boykopovar#5081. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…emulation Recompile-level check for boykopovar#907. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c13150315d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| state.module.AddFunction(spv::OpBranch, pendingMerge); | ||
| EmitLabel(state, pendingMerge); | ||
| state.module.AddFunction(spv::OpPhi, boolean, doneNext, served, electedMerge, done, loopBody); | ||
| state.module.AddFunction(spv::OpMemoryBarrier, ConstantU32(state, spv::ScopeWorkgroup), ConstantU32(state, spv::MemorySemanticsAcquireReleaseMask | LdsMemorySemantics(state))); |
There was a problem hiding this comment.
Skip lane serialization for function-backed LDS src2
When a vertex or fragment shader uses a DS src2 opcode, LDS is backed by per-invocation Function storage, so different lanes cannot alias each other's data. Nevertheless, both new EmitSharedSrc2U32 and EmitSharedSrc2U64 unconditionally enter this loop, electing every active lane and issuing a workgroup-scoped barrier after each update. This turns each otherwise independent src2 operation into 32/64 sequential iterations and reintroduces the workgroup barrier that the function-LDS path avoids elsewhere; bypass the lane-ordered helper and update directly when InFunctionStorage(state, mem.kind) is true.
Useful? React with 👍 / 👎.
… history line Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DgW4n3qYgi5jAAohEmXRQj
Upstream PR batch 1, U6: shader semantics correctness (ALU, LDS, export)
Ports 14 of the 16 triaged upstream correctness fixes, one commit per upstream PR ("Ported from upstream PR #N"). Frontend and SPIR-V changes feed our Metal bridge; no Vulkan paths are touched.
Ported
Notes on how they landed:
ShaderMemory.cpp; kept main's barrier condition for non-LDS shared atomics and dropped the unrelated SRT-read-patching test hunk that rode along.Vop3InterpDecodehunk is not ported (that test file is not on main); a small decode check replaces it.ShaderDiskCache::FormatVersionis bumped 34 -> 35 once (separate commit):MemoryInfo::flushDenormalResultsis now encoded in the plan and fix(shader): run DS src2 lanes one at a time in lane order boykopovar/AnyPS5#5498 adds IR opcodes.Skipped
Tests (all run on Metal, ctest, GPU for the first group)
core/libs/prx/libSceAgcDriver/tests/metal_semantics/: a small shim presents the Vulkan test-device interface on Metal (guest buffer descriptors are mirrored into shared MTLBuffers), so upstream execution tests compile unchanged and run on the GPU:ValuMaskInactiveLanes(fix(shader): clear the bits of inactive lanes in VALU carry-out and v_div_scale masks boykopovar/AnyPS5#5162),VCvtF32F64ResultModes(fix(shader): apply the f32 output modifier and denormal flush to v_cvt_f32_f64 boykopovar/AnyPS5#5159),LdsAddF32(fix(shader): flush ds_add_f32 denormals and NaNs as the hardware does boykopovar/AnyPS5#5492),LdsSrc2LaneOrder(fix(shader): run DS src2 lanes one at a time in lane order boykopovar/AnyPS5#5498),BarrierGlobalMemory(fix(shader): order buffer and image memory at s_barrier boykopovar/AnyPS5#5117; the barrier check is on the emitted SPIR-V semantics, the exchange itself is not a deterministic race detector).TgSizeWaveIndex(fix(shader): put the compute wave index in TG_SIZE[11:6] boykopovar/AnyPS5#5214, buffer-store variant of the upstream global-store test),FunctionLdsUpdates(fix(shader): update per-invocation LDS without atomics boykopovar/AnyPS5#5160, standalone copy of upstream's SPIR-V check),ColorCompareReject(fix(agc): color compare emulation: review follow-up to #668 boykopovar/AnyPS5#907 TRUNC_COORD),Vop3InterpHighHalf(fix(shader): decode the high-half result of v_interp_p2_f16 boykopovar/AnyPS5#5471).ImageSampleGather(fix(shader): accept MIMG sampler tuples ending in VCC boykopovar/AnyPS5#4996),FlatScratch(fix(shader): decode GFX10 scratch offsets as one dword boykopovar/AnyPS5#5426),InterpolationReferenceRange(fix(recompiler): name the attribute of an out-of-range interpolation reference boykopovar/AnyPS5#5450).anyps5_metal_guest_draw_replaygains two real draws: 32_AR export (fix(shader): take the 32_AR export alpha from channel 1 as GFX10 does boykopovar/AnyPS5#5110) and an NGG vertex shader reading the group info (fix(recompiler): give vertex-path shaders the NGG group info boykopovar/AnyPS5#5081, wave32 and wave64).LdsSrc2andLdsSrc2Wide(use device-memory LDS, which the Metal bridge rejects). The upstream files for fix(shader): take the 32_AR export alpha from channel 1 as GFX10 does boykopovar/AnyPS5#5110, fix(recompiler): give vertex-path shaders the NGG group info boykopovar/AnyPS5#5081, fix(shader): update per-invocation LDS without atomics boykopovar/AnyPS5#5160, fix(shader): put the compute wave index in TG_SIZE[11:6] boykopovar/AnyPS5#5214, fix(shader): decode the high-half result of v_interp_p2_f16 boykopovar/AnyPS5#5471 execution tests are in tree as ported but are Vulkan-only.cteston the shader-bridge/guest-replay configuration, 50/50 passed (before the final rebase onto main, which touched no shader code).Game smoke (one run per side, 280 s wall, eboot lock, on AC power)
Branch vs main, same title image: both reach guest entry, no fault, no uncaught exception, 0 skipped draws or dispatches, no conversion failures, identical open sequence for the first 2600 log lines (intro movies), same single unimplemented import. The branch run was cold-cache (format 35: 60 shaders converted, 0 failures); the main run used the warm cache. The branch run progressed further into asset loading within the wall time; run-to-run timing varies on the shared machine. Screens were not compared pixel by pixel, and neither run was confirmed to reach a 3D map in 280 s.
Performance
Not measured.
Pre-existing issues seen (not touched)
tests/execution/EmulatedColorCompare.cppon main does not compile (expectedVariant,RunSrgbCubeZeroundefined).MetalReplay/UpstreamCorrectness/CMakeLists.txtis not included by any build file (already tracked as [BT-09] 20 tests (VideoOut, Flip, UpstreamCorrectness) are reachable only through an undocumented diagnostic wrapper #194).LdsSrc2LaneOrderruns without the layer for that reason).🤖 Generated with Claude Code
Docs: the nine upstream docs hunks (5110, 5117, 5426, 5159, 5162, 5160, 5492, 5498, 907) are ported into docs/dev/TechnicalDebt.md (this tree has no docs/dev/debt/shader.md); the stale DS src2, LDS float atomics, f32 denormal, output modifier and color compare entries are updated, and the ShaderDiskCache history now has a 35 line.