Repository navigation
fix(shader): read the ancillary render target index from Layer when the draw exports it - #5097
Conversation
8e727e5 to
a05127a
Compare
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
|
|
Thanks for moving layer to request23. Our staged fork heada5a282 uses your actual Independent untouched-author emission confirms twelve canonical23 cases; our registered test-loop bytes match all twelve exactly, including tessellation63/F16/layer combinations. Compatibility20 repetitions, full build and NVIDIA checks pass. AMD outcomes remain disclosed in the maintained alpha report. The #4451 PR head stays unchanged until #5097/#5137 merge, per review; the final rebase will use the resulting main. Earlier layer22/dither23 proposals were superseded by #4114's upstream22 allocation. AI-assisted: Codex. |
|
Test report: fixes Epic Mickey: Rebrushed (PPSA15657) on Linux, RX 7600, RADV Mesa 25.2.8. On main without this PR: the title's levels render with almost no blue. Mean blue in the game area is 0.6 out of 255, while red and green reach 60–70, so the image is yellow-green. The HUD is unaffected. Cause:
What I checked:
🤖 Generated with Claude Code |
adriadam10
left a comment
There was a problem hiding this comment.
Checked the logic (Layer * LayerExported in both lowerings, key/specialization plumbing, debt entry): it matches what the tests and the reports from @johnmaia (Epic Mickey, RX 7600, RADV) show. agc_shader_memory, agc_driver_layered_volume_draw, agc_driver_pixel_ancillary_word and pixel_ancillary pass on the PR merged onto current main.
One thing to fix:
core/shader/recompiler/ControlFlow/src/RequestSerializer.cpp:389: the newlayerExportedbyte is read atversion >= 22u, but main already writes and reads version 22 (writeRequestwrites22u,patchesPerGroupis read at>= 22u). A version 22 request produced by main has no such byte, so it is misread (the same kind of break as the version 18/19 case fixed in #4031). Move the flag to version 23: bump the written version and the upper bound, read it at>= 23u, and make the legacy-request test inShaderMemory.cpp(the loop over 19..21) cover 22 as well. #5137 and #4451 need to be re-based on that number.
…he draw exports it
|
@AlxcBTW Tag following request |
|
@TideOPL thanks for the tag. Sword of the Sea (PPSA25344, UE5) very likely takes the same path: UE's tonemapper builds its colour-grading LUT as a layered 32³ volume draw, and on |
…on 23 main allocates request version 22 to the tessellation patches per group, so a version 22 request has no layer export byte.
a05127a to
de5279b
Compare
|
Thanks, moved the flag to request version 23 (de5279b): the writer and the upper bound are 23, |
adriadam10
left a comment
There was a problem hiding this comment.
Re-checked on the head merged onto current main: Layer * LayerExported in both lowerings, key/specialization/disk-cache plumbing, request version 23 (>= 23u, versions 19-22 read no flag) and the debt entry. agc_shader_memory, agc_driver_layered_volume_draw, agc_driver_pixel_ancillary_word and pixel_ancillary pass; without the change the tests don't build. Also confirmed by @johnmaia on Epic Mickey (RADV, RX 7600). The only failure in the scoped run, agc_driver_tessellation, segfaults the same way without this PR. Merging once the full Build is green.
boykopovar#5097 added ShaderInfo::layerExported to the entry encoder, and this series moves that line into encodeShaderInfo, which plan entries use too. The entry round trip left the flag false, so an encoder and a decoder that both skipped it still passed. Set it like the other ShaderInfo flags: with layerExported dropped from both, the round trip now fails with "entry round trip: shader info differs". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@TideOPL a follow-up from Sword of the Sea, and a correction to my guess above. With this PR ported onto our lab line (RTX 5070 Laptop, Windows):
So I can't claim a fix on SotS. There was no regression either: the session ran normally with the PR in. Sorry for pointing at the wrong cause earlier. |
* test(recompiler): set layerExported in the disk cache entry round trip boykopovar#5097 added ShaderInfo::layerExported to the entry encoder, and this series moves that line into encodeShaderInfo, which plan entries use too. The entry round trip left the flag false, so an encoder and a decoder that both skipped it still passed. Set it like the other ShaderInfo flags: with layerExported dropped from both, the round trip now fails with "entry round trip: shader info differs". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit 5d3ea5f) * fix(shader): read both version 21 request layouts (cherry picked from commit dffdb50) * test(shader): cover version 21 F16 request layout (cherry picked from commit 0e087e9) * fix(shader): require the version 22 F16 request flag (cherry picked from commit 2ad825f) * fix(recompiler): accept SDWA source selects on v_cvt_f32_ubyte1, ubyte2 and ubyte3 v_cvt_f32_ubyte0 accepts every SDWA source selector, but the other three byte conversions had no VOP1 SDWA rule, so any SDWA source select on them was refused ("VOP1 SDWA source selector 4 of opcode 0x12 at pc 0xfcc is not supported"). The selected and optionally sign-extended source goes through the same operand path as for ubyte0 before byte N is extracted: SDWA supplies source 0 (RDNA 2 ISA guide 6.8 and 13.3.7) and the opcode reads S0.u[8N+7:8N] (12.8). agc_driver_sdwa_cvt_f32_ubyte: WORD_0 and WORD_1 selects on ubyte1, and sign-extended WORD_1 and BYTE_3 selects on ubyte2 and ubyte3. (cherry picked from commit 7b2104e) * feat(recompiler): read the scalar aperture sources as values Reading src_shared_base, src_shared_limit, src_private_base or src_private_limit (scalar source codes 235-238) refused the shader since apertures that the flat routing from boykopovar#1190 classifies: a base is the aperture's high dword over a low dword of 0, a limit is the same high dword over 0xffffffff (shared 0x80000000, private 0x70000000). A 64-bit read (s_mov_b64, a 64-bit VALU source, a wave64 lane mask) gets the whole value and a 32-bit read gets the low dword. A flat pointer built from the high dword therefore reaches LDS or scratch, and 64-bit compares against the limits order as on the hardware. The flat routing and the reads share one definition of the two high dwords. Sources: LLVM SIRegisterInfo.td (aperture registers are 64-bit, their low 32 bits are always 0 for a base and -1 for a limit) and getSegmentAperture in AMDGPULegalizerInfo.cpp and SIISelLowering.cpp (a 64-bit read whose high half is the aperture); the RDNA 2 ISA guide, sections 5.2, 5.8 (SH_MEM_BASES holds 16-bit bases) and 9.3; the KFD's MAKE_LDS_APP_LIMIT (limit = base | 0xffffffff); the gfx1030 and gfx1035 readouts on boykopovar#1190 and boykopovar#3803. The PS5's two high dwords stay inferred, as the shader technical debt records. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit dc1c12a) --------- Co-authored-by: Niko <nikobilger2000@gmail.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: MarcZX100 <marczxrr@gmail.com> Co-authored-by: Dean Galvin <dean.galvin@live.com> Co-authored-by: fdfawef324 <287205748+fdfawef324@users.noreply.github.com>
What
#4842 made the render target index in the packed pixel ancillary word (bits 16 and up) the constant 0, because no pre-rasterization stage wrote
Layerand reading the unwritten builtin drops the fragment's writes on AMD's Windows driver (#4800). Its description noted that once layered draws exportLayer, those draws need the real value. They do now: a layered draw ORsPA_CL_VS_OUT_CNTLbit 18 into the vertex request (State::layerExports), and #3763 renders layered draws into 3D color targets. A pixel shader in such a draw reads 0 for every layer.Layerwhen the draw exports it, 0 otherwise. The layer field isLayer * LayerExported, whereLayerExportedis a new pipeline specialization constant (PipelineSpecialization::LayerExported), handled like feat(shader): give reads the masked-select pass keeps the whole pixel ancillary word #4031'sRasterPrimitive: the variant records that it reads it (ShaderInfo::layerExported), and the module key and specialization carryShaderPixelStageInfo::layerExported. With the constant at 0 the driver folds the load away, which AMD Windows: draws whose pixel shader reads the packed ancillary VGPR leave pixels unwritten (agc_driver_relative_ancillary fails on main) #4800 showed leaves AMD's writes intact ("gl_Layerloaded but unused: writes land").lowerPackedAncillary(field reads) andComposePackedAncillary(whole-word reads, which composed only the primitive class and the sample index) both use it.DrawDecodesetslayerExportedfromState::layerExportsfor a pixel program that reads the ancillary word. The request serializer writes it from version 23 (main already uses 22 for the tessellation patches per group); older requests readfalse. The shader disk cache format moves to 29.shader.mddebt entry for the packed ancillary word now describes the two cases.Tested
ANYPS5_GPU=Radeon), WinLibs GCC 15.2, Release,BUILD_TESTING=ON, rebased onmain5d7f1ff.agc_driver_layered_volume_draw: the layered volume draw again, with a pixel shader that writes(ancillary >> 16) * 0.5to every channel, so the bands of layers 0, 1 and 2 must read 0, 128 and 255. Onmain(with the onelayerExportedline of the test removed) it fails:slice 2 texel (16, 0) channel 0 is 0, expected 128 (the band of layer 1). With this change it passes, also in the_vertexand_vertex_no_gplvariants.pixel_ancillary: rows with the layer exported; field reads and the composed word now include the layer only when exported, and the composed word must useLayerExported.agc_shader_memory: version 23 prefix, version 24 is refused, the flag round-trips, versions 19 to 22 read no flag, a truncated flag is refused. Reading the flag at version 22 fails the test.ctest -E windows_backtrace: 1077 of 1077 pass on 5d7f1ff (7 skips as onmain).0x164e),agc_driver_relative_ancillary(the AMD Windows: draws whose pixel shader reads the packed ancillary VGPR leave pixels unwritten (agc_driver_relative_ancillary fails on main) #4800 reproduction),agc_driver_pixel_ancillary_wordand theagc_driver_layered_volume_*tests, including the new case, pass.mainrun: the title needs other open PRs to reach gameplay.tools/check_conventions.py --base origin/mainpasses.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)