Repository navigation
feat(agc): accept UINT16_ABGR color exports into unsigned integer targets - #1073
Conversation
|
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.
|
|
I've had this PR in our Ghost of Yotei (PPSA26344 v1.008) integration tree for two long runs, and it removes the only UINT16_ABGR skips the game hit. Without it, With this PR at 7487092 on main 0d3c1ee, a 2,700 s run has no One thing this doesn't tell you: our log names a target only when a draw is skipped, so I can't see which integer format those targets have or what values reached them. The clamp-or-truncate question in your TechnicalDebt line stays open as far as Yotei goes. DetailsBuilds:
The skip, from
The log has 75 such lines, all With the PR: 0 lines in the cold 2,700 s run (1,115 presents) and 0 in the warm one, and none of the four main targets appears anywhere in the cold run's 526 MB |
adriadam10
left a comment
There was a problem hiding this comment.
Thanks, the decode/validation side looks right: the NUMBER_TYPE check matches the recompiler's existing mode 7 handling, blending is refused, and the TechnicalDebt line is in the right section. ctest passes locally (416/416) and CI is green.
One thing to fix:
tests/PixelUint16Export.cpp:15-26: the test counts SPIR-V opcodes (OpBitFieldUExtract, half unpacks), so it pins the IR shape, not the result. A different but correct lowering would fail it, and a wrong extraction (swapped halves, wrong shift) with four extracts would pass. Please run the shader and compare values, as the other execution tests do: export packed words with edge rows (0x0000ffff,0xffff0000,0x00010002,0xffffffff) and check each 16-bit half arrives as the expecteduvec4component, including thetargetExportMappingswizzle. Keep the mode 4 case for contrast if you like.
…gets SPI_SHADER_COL_FORMAT 7 (UINT16_ABGR) was rejected by DecodeState, though the recompiler already handles it: a pixel shader's MRT output under mode 7 is a uvec4, and a compressed export's 16-bit halves are taken with OpBitFieldUExtract. The format is now accepted when the target's CB_COLOR_INFO number type is UINT; into any other target it still throws, as does SINT16_ABGR (8), whose halves would need sign extension. The target records the export (ColorTarget::uintExport). Shader validation then requires a uint4 output at its location instead of a float4 one, and blending into it is refused (Vulkan has no blending of integer attachments). PPSA14632 (LEGO Horizon Adventures) writes a 2848x1600 R8_UINT target this way; those draws were dropped. agc_driver_pixel_uint16_export draws compressed MRT0 exports under mode 7 into a linear 16_16_16_16 UINT target, one row per pair of packed words (0x0000ffff/0xffff0000, 0x00010002/0xffffffff and four distinct halves), and reads every 16-bit component back under the identity, STD_REV and ALT_REV export mappings. agc_driver_graphics covers the decode and the blend refusal. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
7487092 to
4554135
Compare
|
@adriadam10 Done in 4554135, rebased onto current |
|
Uncharted: Legacy of Thieves Collection (PPSA05684) stops at this on I made the same change independently (format 7 into UINT targets only, |
|
@adriadam10 Could you take another look? The opcode-count test is replaced by the execution test you asked for (4554135, details above). CI is green and it still merges cleanly onto @XairRrR Thanks for the Uncharted report. |
adriadam10
left a comment
There was a problem hiding this comment.
Checked the decode/validation change against the recompiler's mode 7 handling, ran the new agc_driver_pixel_uint16_export execution test (all three export mappings) plus agc_driver_graphics, full ctest green on top of current main; the test does not build without the change. Merging.
Nit for later: the new TechnicalDebt line lacks a final period.
What
SPI_SHADER_COL_FORMAT7 (UINT16_ABGR) is accepted when the target'sCB_COLOR_INFOnumber type is UINT.DecodeStaterejected it although the recompiler already handles it: an MRT output under mode 7 is auvec4(SpirvEmitterHelpers), and a compressed export's 16-bit halves are taken withOpBitFieldUExtract(ExportVector).ColorTarget::uintExport). Shader validation then requires au32x4output at its location instead off32x4.SINT16_ABGR(8), whose halves would need sign extension.R8_UINT) is undefined in Vulkan and the console's behaviour is not confirmed.Tested
Linux 7.2.9 (Arch), Mesa 26.2.4 RADV, RX 9070 XT.
R8_UINTtarget with export format 7. About 290 draws per 3 minutes were dropped; they now record.agc_driver_pixel_uint16_exportdraws compressed MRT0 exports under mode 7 into a linear 16_16_16_16 UINT target, one row per pair of packed words (0x0000ffff/0xffff0000,0x00010002/0xffffffff, the two swapped, and four distinct halves), and reads every 16-bit component back under the identity, STD_REV (0x1b) and ALT_REV (0x93) export mappings. A sixth row stays untouched. It fails if the halves are swapped or the mapping is ignored.agc_driver_graphicscovers the decode, theuintExportflag and the blend refusal.ctest: 464 of 464 pass on currentmain.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed in TechnicalDebt.mdfiles or images added to the repository (attach them to this PR)🤖 Generated with Claude Code