Skip to content

fix(shader): keep the output modifier of the f16 min3, max3, med3 and div_fixup ops - #4097

Merged
oneandonlydean merged 1 commit into
boykopovar:mainfrom
kr4y4:fix/shader-f16-ternary-output-modifier
Oct 13, 2026
Merged

oneandonlydean merged 1 commit into
boykopovar:mainfrom
kr4y4:fix/shader-f16-ternary-output-modifier

Conversation

@kr4y4

@kr4y4 kr4y4 commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

What

The decoder drops the VOP3 output modifier (mul:2, mul:4, div:2) of v_min3_f16, v_max3_f16, v_med3_f16 and v_div_fixup_f16. checkNativeVop3Modifiers accepts any modifier on these four ops, but supportsNativeVop3ResultModifiers doesn't list them, so destination.omod is set to 0. The f32 versions (v_min3_f32, v_max3_f32, v_med3_f32, v_div_fixup_f32) and v_fma_f16 keep it.

d3f2611 applies the output modifier only in the float modes where the hardware does. On f16 results it is ignored, and in the modes where the hardware applies it (IEEE off, f16 output denormals flushed) the translator throws "output modifier on an f16 or f64 result with f16/f64 output denormals flushed is not implemented". These four ops never reach that check. In those modes they compute without the modifier instead of refusing.

This adds the native f16 ternary ops (isNativeVop3F16TernaryOpcode, which already includes VFmaF16) to supportsNativeVop3ResultModifiers. Their results already go through clampF16Bits, which ignores the modifier in the other modes and throws in these. Nothing changes in the default float mode.

LLVM 23.1.3 llvm-mc -mcpu=gfx1013 disassembles all four with the modifier, for example d7570001 14020100 as v_med3_f16 v1, v0, v0, v0 mul:4. I found this by comparing the decoder against LLVM's disassembler over random VOP3 modifier fields.

Tested

Windows 11 (26200), WinLibs GCC 15.2.0 r7, CI configure flags, main 325e0a8. main currently doesn't build on Windows (HostPoll in Socket.cpp, fixed by #4021), so #4021 was applied locally for these runs and isn't part of this change.

agc_driver_float16_modifiers gets four one-instruction shaders: v_min3_f16 div:2, v_max3_f16 mul:2, v_med3_f16 mul:4 and v_div_fixup_f16 mul:2. Each must compile and run in the default float mode and with FLOAT_MODE 0xf0, and must be refused with f16 output denormals flushed (FLOAT_MODE 0x00, IEEE off). Run now takes the shader code so the existing shader and the new ones share it.

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 (NotImplemented_nid_no_patch); silent stubs are listed in TechnicalDebt
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source (none added)
  • Generic behaviour, not specific to one title
  • Depends on: none
  • Unblocks: none
  • AI-assisted: yes

🤖 Generated with Claude Code

… div_fixup ops

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added shader core/shader agc libSceAgc* labels Oct 11, 2026
@Dextroid17

Copy link
Copy Markdown
Contributor

The decode fix is exactly scoped — these four opcodes join supportsNativeVop3ResultModifiers via the existing isNativeVop3F16TernaryOpcode umbrella (which already included v_fma_f16), and clampF16Bits itself is unchanged; this only changes which opcodes reach it with a nonzero output modifier instead of having it silently zeroed first.

Worth being explicit about the trade, since it's real: for these four ops, in the one FLOAT_MODE combination where the hardware actually applies the modifier (IEEE off, f16 output denormals flushed), the old code computed a wrong result with no signal — a silent factor-2/4 error. The new code throws there instead, matching the throw this project's own TechnicalDebt already documents for every other f16 op in that mode. That's narrowing a hole, not inventing a new refusal class, but it is a behavior change: a title that happens to set that exact mode combination with one of these four ops now fails shader preparation where it previously ran (incorrectly). Neither the PR nor this review has a title or log line pinning how often that combination occurs — worth a maintainer's explicit sign-off rather than an assumption either way.

One gap in the new test: it checks that the default and 0xf0 modes don't throw, but doesn't check that the value computed in those non-throwing modes with the modifier applied is correct — only "does not throw," not "produces the right answer with mul:2/mul:4/div:2." Worth closing if there's a cheap way to assert the computed value in those modes.

Written with AI assistance.

@kr4y4

kr4y4 commented Oct 12, 2026

Copy link
Copy Markdown
Contributor Author

On the value check: in the default float mode and with FLOAT_MODE 0xf0 the modifier is ignored on f16 results, so the right value there is the unmodified one. The test's existing shader already checks that for three of the four ops. Its v_max3_f16 mul:2, v_med3_f16 clamp mul:4 and v_min3_f16 div:2 lanes are compared with unmodified results by Check() after both runs; lane 0 of v_max3_f16 mul:2 expects 0x73ab, not 0x77ab.

On main the decoder zeroed the modifier before it reached the translator, so those lanes didn't cover that. With this change the modifier reaches clampF16Bits, and the same lanes now check that it's ignored there. They pass on both GPUs.

v_div_fixup_f16 has no lane with a modifier. In those modes its value would match agc_driver_div_fixup_f16, which runs with 0xf0. If a maintainer wants it, I can add a mul:2 copy of one of that test's instructions with the same expected values.

Written with AI assistance.

@oneandonlydean oneandonlydean self-assigned this Oct 13, 2026
@oneandonlydean
oneandonlydean merged commit f9821b7 into boykopovar:main Oct 13, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agc libSceAgc* shader core/shader

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants