Skip to content

fix(shader): accept both version 21 request layouts - #5137

Merged
oneandonlydean merged 3 commits into
boykopovar:mainfrom
MarcZX10:codex/request-v21-compat
Oct 14, 2026
Merged

oneandonlydean merged 3 commits into
boykopovar:mainfrom
MarcZX10:codex/request-v21-compat

Conversation

@MarcZX10

@MarcZX10 MarcZX10 commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

What

Fixes #5132 by accepting both request layouts that were emitted with serialization version 21: the original layout ends after vertexSubgroups and defaults float16DenormPreserve to false, while the later layout has exactly one boolean byte. Invalid boolean values and unexpected trailing bytes are rejected. Version 22 continues to require the F16 flag.

This change is stacked on #5097 because that PR advances the current request format to version 22. The #5097 implementation commit remains attributed to TideOPL; this follow-up commit is authored by MarcZX100.

Adds a registered request_serializer test covering both version-21 layouts (with and without the F16 tail), plus version-22 false/true round trips.

Tested

  • Linux x86-64, GCC 12.2.0, Release: configured with BUILD_TESTING=ON and ANYPS5_RELINKER_ONLY=ON, built request_serializer_tests, and ran ctest -R '^request_serializer$': 1/1 passed.
  • The same request_serializer test rejects the version-22 payload with its final F16 byte removed as RequestSerializer: truncated data; version-21 inputs with and without that tail and version-22 false/true round trips still pass.
  • git diff --check passed.
  • Full application build and title/runtime validation were not run.

Checklist

@Amoenus

Amoenus commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Current rebase status (2026-10-13)

@MarcZX100 #5097 has merged with layer export at request23. A read-only git merge-tree --write-tree --name-only of pinned main 7e4edf509 and this head 240411d exits1 with conflicts in RequestSerializer.cpp and libSceAgcDriver/tests/ShaderMemory.cpp. Please rebase the compatibility repair onto the merged layer23 layout, preserving upstream tessellation22 and the mandatory F16 tail for22+.

We already adapted your actual three commits with the author-updated layer23 stack in fork heada5a282, with authorship preserved, then alpha's dither24 field. Full build, repeated compatibility and NVIDIA validation pass on pinned main b0d; AMD failures are disclosed in the maintained alpha report. That is adapted-stack evidence, not an execution of unchanged head240411d. The new staged rebase on merged main7e4 now passes full build, compatibility20 repetitions and NVIDIA checks; full NVIDIA1082/0/8 of1090. AMD failures remain disclosed. This is the adapted compatibility+alpha stack, with no unchanged-head or new title claim.

The original missing-tail finding below is historical and was addressed by your240411d correction. #4451's PR head remains held until this compatibility PR also merges. AI-assisted: Codex.

Historical original-head proof and earlier adapted validation

Thanks for addressing #5132. I tested exact head b6854f0c9b9ecf7a0930d386dc4e2f307eda646f using its unmodified public serializer/headers on Windows with MinGW-w64 GCC 15.2.

One remaining case: version22 still accepts a missing final F16 flag. I used the actual writer22, decoded its Base64 output, removed only the last byte, and re-encoded it. All nine cases were accepted as F16=false, including captures written with F16=true. The optional-tail block currently applies to version >= 21, so it also permits a zero-byte tail for22.

A narrow correction, retaining your existing strict bool and extra-byte checks:

 if (version >= 21u) {
+    if (version >= 22u) reader.Require(1u);
     if (reader.Remaining() == 1u) {

On an external source copy with that one line, the eight actual version22 roundtrips and 48 independently preserved historical21 replays still pass; all nine missing22 tails throw exactly RequestSerializer: truncated data. Those historical cases include both zero-byte tails and false/true one-byte continuations.

Minimal input, generated from RecompileRequest{} with F16=true and only its final serialized byte removed:

auto replay = ShaderRecompiler::RequestSerializer{}.Deserialize(
    "NVNQQRYAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAQAAAAAAAAAAQA0DAAE=");

Current result: accepted with replay.request.target.float16DenormPreserve == false. Expected: truncated-data exception. This would also make a useful regression in request_serializer.

The same serializer blob was still live at the final check. This is standalone CPU serialization validation; full application/GPU validation is pending. AI-assisted: yes.

Later full-build validation (2026-10-13)

The exact unchanged b6854f0c CPU proof above remains scoped to that original head. We also adapted the credited #5097/#5137 stack onto pinned main 5aaa394cff37edb97ef83a7f98322e07e9e040c8 and applied the separate one-line mandatory-v22 correction: tested fork head40b. Full Windows application build/libraries and convention/registration checks pass. Actual registered request_serializer and agc_shader_memory each pass20 repetitions; register-key and PM4 pass5. This full-build evidence is for the adapted, corrected stack, not unchanged PR head.

All seven selected real Vulkan groups pass5 in both GPL modes on RTX5080; AMD retains alpha/layered-volume mismatches. Fresh untouched-main5aaa GPL-off controls reproduce the same original alpha and white-band vertex diagnostics5/5 each, and restoration reproduces them once. The new ancillary-layer case is not reached in the failing vertex run, so those results are not evidence of a new #5097/#5137 regression. The control's optional successful GPL-on alpha run triggered a truncated-JUnit harness assertion; it is one actual pass, not five. No full-suite/title/console result is claimed. AI-assisted: Codex.

Author correction received: latest head 240411d now includes if (version >= 22u) reader.Require(1u); and regression coverage. Source inspection confirms the reported missing-tail defect is addressed; this latest head has not been executed.

Later main changed: #4114 now uses canonical version22 for patchesPerGroup; schema coordination supersedes the layer22/dither23 plan. The full-build results above remain pinned to main5aaa and the corrected fork head40b.

@MarcZX10

Copy link
Copy Markdown
Contributor Author

Thanks for reproducing this and supplying the minimal truncated payload. Fixed in 240411d56: version 22 now requires its final F16 byte, while the version-21 tail remains optional. I added a regression using the truncated version-22 payload and checking for RequestSerializer: truncated data.

On Linux x86-64 with GCC 12.2 Release, request_serializer passes (1/1); both version-21 fixture layouts and version-22 false/true round trips still pass. CI has started for the updated head.

@MarcZX10
MarcZX10 force-pushed the codex/request-v21-compat branch from 240411d to 2ad825f Compare October 13, 2026 19:10
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 13, 2026
@MarcZX10

Copy link
Copy Markdown
Contributor Author

Rebased #5137 onto current main and preserved the now-merged layer-export format at version 23, while retaining the strict final F16-tail requirement for version 22 and later. The standalone request_serializer_tests executable compiled with GCC 12.2 and passed; conventions also pass. Current automatic relinker Linux/Windows/macOS jobs pass, while the full Linux and Windows build jobs are skipped by the PR workflow. This validates the compatibility repair on the rebased head, not GPU/title behavior.

@MarcZX10

Copy link
Copy Markdown
Contributor Author

Thanks for the status update. I rebased #5137 onto current main and pushed head 2ad825f6590e3e94bd836af5ca02db6199be315e, preserving the merged version-23 layer-export layout and the required F16 tail for v22+. The current PR diff is RequestSerializer.cpp, its standalone tests and the test target; no ShaderMemory.cpp conflict remains. GCC 12.2 serializer tests pass locally, and the current CI relinker jobs on Linux/macOS/Windows plus conventions pass. Full application and GPU validation are still not claimed on this head.

@github-actions

github-actions Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

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.

Pull request Conflicting files (lines on main) Same exports Same new files
#4706 core/shader/recompiler/ControlFlow/src/RequestSerializer.cpp L811

@oneandonlydean
oneandonlydean merged commit a1ca1e7 into boykopovar:main Oct 14, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agc libSceAgc* build docs shader core/shader

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RequestSerializer: two incompatible version-21 layouts after #3988

3 participants