Skip to content

fix(shader): evaluate signed comparisons in SRT expressions - #2072

Merged
adriadam10 merged 1 commit into
boykopovar:mainfrom
STUPIDCONTEM:fix/srt-signed-comparison
Oct 10, 2026
Merged

adriadam10 merged 1 commit into
boykopovar:mainfrom
STUPIDCONTEM:fix/srt-signed-comparison

Conversation

@STUPIDCONTEM

@STUPIDCONTEM STUPIDCONTEM commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Evidence review (2026-10-09)

Game/title evidence has not been established. Validation below is host regression/build evidence only; it does not establish title compatibility. Older checked items and test results describe their stated revisions, not current merge readiness.

Game/title ID: none verified. No PS5 title execution is claimed.

What

The generic shader-resource evaluator cannot resolve descriptor expressions containing SLessThan32, even though the IR translator and SPIR-V backend support that comparison. Add signed 32-bit comparison of the operand bit patterns, returning the existing Boolean encoding.

This implements the specific missing operation identified by johnmaia in the EVO PLAYER report. Semantics follow the normative SPIR-V OpSLessThan instruction, which the existing EmitSLessThan32 backend emits. No new instruction decoding or shader fallback is introduced.

Tested

Windows 10 Pro 19045, WinLibs GCC 15.2.0 UCRT posix SEH r7, Release, BUILD_TESTING=ON. Independent branch from f145139676a48306681d3a4d5fabf83938759daf.

  • Extended agc_shader_memory calls the actual runtime descriptor evaluator with signed comparisons beneath SelectU32 descriptor dwords. Inputs INT_MIN, -1, 0 and INT_MAX exercise both signed order and equality boundaries for x<0, INT_MIN<x and x<INT_MAX.
  • The regression fails on unchanged upstream with an unevaluable descriptor expression and passes three focused runs with this change.
  • Default and explicit libs builds passed; full CTest: 488/488, 27.59 seconds. Committed-head conventions, whitespace and test-registration checks passed.
  • Parent independently reviewed source semantics, wired the regression into the test runner and verified baseline/fixed behavior; another agent reviewed the signed boundary expectations.

No native RDNA2/PS5 measurement or title run was performed. The hardware-validation limitation is recorded in TechnicalDebt. This is a narrow evaluator fix, not a claim that EVO PLAYER renders end to end.

Open #1341 adds a compiled SRT-expression path but does not whitelist this signed opcode; this remains available through its generic fallback. #757 changes image-table offsets and #2071 prepares combined shader layouts; neither implements this operator.

Checklist

  • Based on current main; no other open PR implements the same change
  • One topic per PR; follow-ups go in a new PR
  • No comments added to code; hardware-validation limitation recorded in TechnicalDebt
  • Unimplemented paths throw; no silent stubs added
  • No investigation notes, reports or images added to the repository
  • No third-party code added
  • Generic behaviour, not specific to one title
  • Depends on: none
  • AI-assisted: yes; parent independently reviewed and tested the final change.

@adriadam10 adriadam10 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked: the SLessThan32 case in SrtEvaluator.cpp is correct and the new verifySignedSrtComparison test fails on main and passes with the change (built and ran agc_shader_memory plus the AGC/AgcDriver ctest scope locally, all green).

Two things to fix before this can merge:

  • The conventions check fails: the checklist item "Based on current main; no other open PR implements the same change" is unchecked. Please verify it against current main and tick it, and drop the "(historical base; current readiness not asserted)" qualifier, which the check reads as unchecked.
  • docs/dev/TechnicalDebt.md:259 sits right after line 258, which #1873 and #1924 also edit, so it will conflict as soon as one of them lands. Move it a few lines away from that spot (not at the end of the section).

@github-actions github-actions Bot added needs rebase Conflicts with main, rebase needed labels Oct 10, 2026
@STUPIDCONTEM
STUPIDCONTEM force-pushed the fix/srt-signed-comparison branch from ec3e34c to 5f6da63 Compare October 10, 2026 07:08
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 10, 2026

@adriadam10 adriadam10 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-checked on the new head: SLessThan32 in SrtEvaluator.cpp is correct (signed compare of the 32-bit patterns), verifySignedSrtComparison fails without the change and passes with it, agc_shader_memory and the AGC/AgcDriver ctest scope are green, no other open PR adds this opcode, and the TechnicalDebt line is now clear of #1873/#1924.

The conventions failure was only the unticked "Based on current main" checklist item with the "historical base" qualifier; I verified it (clean merge onto current main, no overlap) and ticked it in the description. Merging once CI is green.

@adriadam10
adriadam10 merged commit 2c4828a into boykopovar:main Oct 10, 2026
12 of 13 checks passed
adriadam10 pushed a commit that referenced this pull request Oct 11, 2026
#2072 taught the SRT evaluator SLessThan32, but IsRuntimeUniformOp still
left it out, so the runtime validator rejected any descriptor dword computed
through a signed comparison before the evaluator could compute it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agc libSceAgc* docs shader core/shader

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants