Repository navigation
fix(recompiler): avoid illegal narrow constants during optimization - #1343
adriadam10 merged 2 commits into
Conversation
Dextroid17
left a comment
There was a problem hiding this comment.
Built and ran it here — external PRs get no CI in this repo, so this is a local run: Linux/CachyOS, GCC, Release, -DANYPS5_ENABLE_SPIRV_TOOLS=ON, NVIDIA RTX 3080 Ti. This is a comment, not an approval — I don't have merge rights here.
It works, and the new tests bite
Built agc_spirv_narrow_constant_store_tests, agc_spirv_optimizer_tests and agc_driver_constant_store_tests. All nine registered names (3 targets × default/full/none) pass; full suite 417/417 (408 on main, +9).
The part worth recording is the negative: taking only SpirvOptimizer.cpp from upstream/main and keeping your tests, 4 of the 6 SPIR-V tests fail with the issue's exact line —
i16 vector storage only: SPIR-V validation after optimization failed:
level=2 word=22: Cannot form constants of 8- or 16-bit types
%short_0 = OpConstant %short 0
and the two _none runs pass, correctly, since they never optimize. So the regression is real on main and the new tests are load-bearing rather than presence checks.
The default pipeline is unchanged
The restructure is the risky half, so I extracted the pass sequence for the folding-allowed default path from both revisions and compared them mechanically: 22 passes, identical, same order. Registering PerformancePasses only for full && allowFolding does not change what a module without narrow types already got.
DotProductInput4x8Bit is right, and it is the subtle bit
I expected this to be a false negative and it is not. spirv-val accepts a module that declares DotProductInput4x8Bit with no literal OpCapability Int8, and the reason is ValidationState_t::RegisterCapability (vendored SPIRV-Tools, source/val/validation_state.cpp:386) recursing over the grammar's required capabilities — the grammar gives DotProductInput4x8Bit "capabilities": ["Int8"]. So the validator has Int8 while a scan of the declared lines alone would not. Treating it as int8-available avoids dropping folding for those modules.
The direction is also right: the scan's capability set is a subset of the validator's, so allowFolding can only be more conservative than validation — it can never wave through a fold that the validator would then reject. That is the property that matters, and your test matrix (widths 8/16, signed and unsigned, float16, StorageBuffer8BitAccess, StorageBuffer16BitAccess, DotProductInput4x8BitKHR) is what pins it.
Non-blocking
HasLimitedUseTypesreadsspirv[0]andwordAt(offset + 1 | 2)with no size or instruction-length guard; a zero word count would also not advanceoffset. It is unreachable today only because the scan runs aftertools.Validate(...), which rejects a truncated module first. If that ordering is not meant to be load-bearing, a cheap guard would stop a later move of the call from reading past the span.- The two
_noneregistrations are always green by construction (nothing optimizes), so of the six SPIR-V tests four are the ones that can fail. Fine as a mode matrix, just worth knowing when reading a red job.
Caveat: the GPU-backed agc_driver_constant_store* runs here used APS5_WRITE_WATCH_IMPORTS=watch, because this branch predates #1329 and the NVIDIA dma-buf probe aborts those tests otherwise. The SPIR-V-only tests need no device and ran normally. Not run on Windows.
Written with AI assistance.
Additional Windows / NVIDIA validationTested PR head TestedWindows 11 Pro (26300), MinGW-w64 GCC 15.2.0 UCRT/posix/SEH, NVIDIA RTX 3060, Windows driver 32.0.16.1742. Release, Both runs used this PR's existing fixtures. For the baseline, only
ScopeThis is additional test evidence. No source change is proposed. No game execution, optimization performance benchmark, sanitizer run or other GPU vendor was tested here. The unmodified Windows conventions checker reports a false AI-assisted: yes — Codex set up and ran the validation and drafted this summary. |
|
Recording that the negative control now reproduces on two platforms, since that is what the fix rests on: @gunerky measured 407 passed / 6 failed with the main optimizer on Windows 11 + RTX 3060, and I measured the same 6 failures with the same signature ( Written with AI assistance. |
c7d4638 to
d3dc02a
Compare
Skip CCP and Simplification for storage-only narrow types while keeping the remaining passes and arithmetic-capable folding. Add compiler and Vulkan readback regressions for default/full/none modes. Tested: Linux/llvmpipe, full CTest with SPIRV-Tools on/off, ASan/UBSan optimizer checks, and cold-cache Vulkan wave32/wave64 readback. Fixes boykopovar#1276
Check module headers, instruction lengths and required operands before scanning narrow types. Cover malformed and byte-swapped modules while retaining the native SPIR-V 1.3 conversion matrix. Tested: Linux/llvmpipe CTest three times with SPIRV-Tools on and off, cold-cache Vulkan regressions, and ASan/UBSan in all optimization modes.
|
Rebased on current
Merging once CI is green on the rebased head. |
d3dc02a to
39e8e70
Compare
What
Fixes #1276.
Skip CCP and Simplification for modules with storage-only 8- or 16-bit types, preventing the optimizer from introducing illegal narrow constants. Keep the remaining passes and arithmetic-capable folding without adding device capabilities.
Add optimizer, compiler and Vulkan readback regressions for default, full and none modes.
Tested
Linux x86_64, GCC 14.2.0, Mesa 26.1.6 llvmpipe; the issue's vertex shader and a constant-store compute fixture.
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)