Repository navigation
fix(shader): keep s_cselect_b64 of two masks as a lane bit and fold constant masks - #1640
Conversation
klosixgaming-arch
left a comment
There was a problem hiding this comment.
LGTM. Well-structured fix for the s_cselect_b64 mask handling issue on Metal/MoltenVK. The three-part approach is clean: (1) scalarSelectMask64 properly stores lane bits and marks pairs as masks, (2) foldConstantBitTest eliminates LaneId usage for constant all-zero/all-one masks, and (3) threadBit recognizes constant masks upfront. The test specifically validates the folding cases that were failing before. Good end-to-end testing on a retail title showing the MoltenVK error is resolved.
e575bba to
af97509
Compare
af97509 to
8dc1a82
Compare
|
Native Windows run, since no Windows results were posted. Setup: Windows 10 (19045), RTX 3060 Laptop (NVIDIA 616.92), MinGW-w64 GCC 15.2.0 (WinLibs r7), Release. Head
No title was run. 🤖 Addressed by Claude Code |
8dc1a82 to
11cbb4c
Compare
adriadam10
left a comment
There was a problem hiding this comment.
Checked: build + ctest on Linux are green, and agc_constant_bit_test fails when the non-test changes are reverted, so the folder part is covered. The translation looks right to me (sources read before the destination is written, same mask-tag pattern as v_cmp).
One thing needed before merging:
scalarSelectMask64(ControlFlowInstructions.cpp:247) and the constant paths inreadMask/threadBit(ExecMask.cpp:21,:41) are not covered by any test; the PR description says so too. Please add a test (execution, or IR-level next toagc_masked_select_tests) that pins values:scc= 0 and 1, a non-constant source (lanes differ), destination equal to one of the sources, and a wave64 case where the two halves differ.
11cbb4c to
1fc9679
Compare
|
The requested test is in:
The constant cases also check that the select keeps both sources as IR constants, which is how I removed the two constant returns in To check that the test can fail, I changed the fix four ways: with I ran the test on macOS by linking the recompiler objects without libc ( @adriadam10 could you take another look? |
…onstant masks A select of exec and zero gives a mask that is the same for every lane. Reading it back went through a lane number, which Metal does not have in a vertex shader, so MoltenVK rejected the shader. The select now stores the lane bit and marks the pair as a mask, a constant mask word reads as a constant, and the constant folder removes the bit test on an all-zero or all-one word. agc_scalar_select_mask runs the translated IR over a wave and pins the lane values for scc set and clear, a source that differs per lane, the two wave64 halves differing, a destination that is also a source, and constant and literal sources. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
1fc9679 to
93b0f48
Compare
adriadam10
left a comment
There was a problem hiding this comment.
Checked on Linux (build + ctest): agc_constant_bit_test and agc_scalar_select_mask pass, and both fail with the non-test changes reverted. Measured s_cselect_b64 on hardware (gfx1035) with vcc/exec/0/-1/literal/SGPR-pair sources and scc set and clear: the result is scc ? source1 : source2 and a literal is zero-extended, which matches what the tests pin. Thanks for adding the translation test; merging.
What
s_cselect_b64 sd, exec, 0(and the same form with vcc or all ones) gives a mask that is the same for every lane. The translation stored it as a plain register pair, so a later read of the pair went throughLaneId. Metal has no lane number in a vertex shader, and MoltenVK rejects such a shader ("subgroup capability 61 is unsupported for shader stage 1"), so the draw is skipped.s_cselect_b64with two mask sources now stores the lane bitscc ? source1 : source2and marks the pair as a mask, the same wayv_cmpresults are kept. The sources are read before the destination is written.((W >> (x & 31)) & 1) != 0to a constant whenWis all zero or all one. Some masks only become constant after folding.The translation is exact. Nothing changes when the lanes differ.
Tested
agc_constant_bit_test: fails without the fold (all-zero and all-one cases) and passes with it.link.h), so I ran the test and the existing masked-select, control-flow, shader-disk-cache and DPP-write tests on a macOS port tree that carries the same shader code: all pass. The four changed shader files compile on this branch.scalarSelectMask64translation by a unit test; it was only checked end to end on the title above.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)🤖 Generated with Claude Code