Skip to content

fix(shader): preserve integer narrowing in constant folding - #4195

Merged
oneandonlydean merged 1 commit into
boykopovar:mainfrom
rugbedbugg:fix/shader-narrowing-fold
Oct 13, 2026
Merged

fix(shader): preserve integer narrowing in constant folding#4195
oneandonlydean merged 1 commit into
boykopovar:mainfrom
rugbedbugg:fix/shader-narrowing-fold

Conversation

@rugbedbugg

@rugbedbugg rugbedbugg commented Oct 12, 2026

Copy link
Copy Markdown
Contributor

What

Prevent the shader constant folder from cancelling unsigned narrowing followed by widening for nonconstant operands.

ConvertU32U16(ConvertU16U32(x)) and ConvertU32U8(ConvertU8U32(x)) are not identity operations for arbitrary 32-bit values: the conversions must discard the upper 16 or 24 bits respectively.

Remove the two unsafe folding rules while preserving the existing immediate-conversion and bitcast optimizations.

Add a Release-active IR regression test covering nonconstant operands, narrowing boundaries, immediates, bitcasts, and dead-code cleanup.

Tested

  • Arch Linux, x86-64, GCC 16, Release build (BUILD_TESTING=ON, ANYPS5_ENABLE_SPIRV_TOOLS=ON).
  • agc_integer_narrowing: 975 cases, 0 failures.
  • Focused shader suite: 7/8 CTests passed, including narrowing, constant-bit, scalar-select, memory, structurizer, SPIR-V module, and shader control-flow tests.
  • agc_vector_window failed its function-scope array assertion. The identical failure was reproduced after rebuilding with the original origin/main ConstantFolder.cpp, then the proposed fix was restored and rebuilt.
  • python3 tools/check_conventions.py --base origin/main: passed.
  • No PS5 hardware or game-title runtime validation performed.

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw; no silent stubs added
  • No notes, investigation .md files or images added to the repository
  • No third-party code added
  • Generic behaviour, not specific to one title
  • Depends on: none
  • AI-assisted: yes (Codex-reviewed. Rebased and tested locally by the contributor)

@rugbedbugg
fix(shader): preserve integer narrowing in constant folding
3df53d9 
Remove unsafe U16/U8 round-trip cancellation and cover nonconstant, immediate, bitcast and dead-code cleanup paths with Release-active IR regressions.
@github-actions github-actions Bot added build shader core/shader agc libSceAgc* labels Oct 12, 2026
@Dextroid17

Dextroid17 commented Oct 12, 2026

Copy link
Copy Markdown
Contributor

The bug is real and the fix is minimal and correct. Worked through it with a concrete value: for a nonconstant x = 0x12345678, the old rule matched purely on the operand's opcode (ConvertU16U32) with no check that the value underneath was actually constant, so ConvertU32U16(ConvertU16U32(x)) folded straight to x itself — 0x12345678, silently keeping the upper 16 bits that the narrowing was supposed to discard. Removing just those two opcode-check blocks (nothing else) means both conversions now survive and get evaluated normally, producing the correct 0x00005678.

Two things worth confirming, and both hold: the optimization isn't lost for the case where it's actually safe — a genuinely constant operand still gets caught by the earlier immediate-folding rules in the same pass, before it would ever reach the removed code, so ConvertU32U16(ConvertU16U32(constant)) still folds straight to the correctly-truncated constant. And the fix is scope-complete — there's no parallel signed-narrowing opcode (ConvertI32I16 or similar) in this IR at all, so there's no sibling bug left unaddressed.

No overlap with #3951, reviewed earlier tonight in this same file: that PR touches foldPhi (dominance in Phi-input folding), this one touches tryFoldValue's narrowing rules, roughly 250 lines apart with zero shared lines. Either can land independently of the other with no rebase conflict.

Written with AI assistance.

@github-actions

github-actions Bot commented Oct 12, 2026 •
edited
Loading

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
#1341 core/libs/prx/libSceAgcDriver/CMakeLists.txt L1159
#4271 core/libs/prx/libSceAgcDriver/CMakeLists.txt L1159
#4339 core/libs/prx/libSceAgcDriver/CMakeLists.txt L1159
#4891 core/libs/prx/libSceAgcDriver/CMakeLists.txt L1159

This was referenced Oct 12, 2026
Merged
Closed
Merged
Merged
Closed
@kr4y4

kr4y4 commented Oct 12, 2026

Copy link
Copy Markdown
Contributor

Windows run, to go with the Linux one in the description.

Setup: Windows 11 (26200), WinLibs GCC 15.2.0 r7, Release, CI configure flags, an RTX 4070 Laptop GPU and an Intel Raptor Lake-S iGPU. The head 3df53d9 merged into main 42496c5.

  • agc_integer_narrowing, the agc_vector_window tests and the agc_shader* tests (15 in all) pass on both GPUs with --repeat until-fail:3. agc_vector_window passes on current main, so the failure in the description is gone there.
  • With main's ConstantFolder.cpp and the new test, agc_integer_narrowing fails on both GPUs with the upper half kept, for example F16 bitcast round trip: pattern=0000ffff lane=1 dynamic=1 cleanup=0 expected=00000000 actual=00010000.
  • Full suite (ctest --timeout 120 --parallel 4): 1008 of 1008 on both GPUs.

Written with AI assistance.

@github-actions github-actions Bot mentioned this pull request Oct 13, 2026
Merged
12 tasks done
@oneandonlydean oneandonlydean self-assigned this Oct 13, 2026
@oneandonlydean
oneandonlydean merged commit 02f5c0b into boykopovar:main Oct 13, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@oneandonlydean oneandonlydean

Labels

agc libSceAgc* build shader core/shader

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@rugbedbugg @Dextroid17 @kr4y4 @oneandonlydean