Repository navigation
[SPIR-V] Replace pixel shader SV_CullPrimitive inputs with 0 - #9088
Abir Deol (apollo-2006) wants to merge 5 commits into
Conversation
DXIL replaces SV_CullPrimitive pixel shader inputs with 0. The SPIR-V backend mapped them to CullPrimitiveEXT, which needs MeshShadingEXT and isn't allowed in fragment shaders, so the output was invalid. Fixes microsoft#9084.
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟡 Changes recommended
The regression test does not verify that SV_Target is zero for both bool and uint runs, allowing incorrect true/1 behavior to pass undetected.
1 open issue
What changed in this PR
This PR fixes #9084 by making the SPIR-V backend replace pixel shader SV_CullPrimitive inputs with zero, matching DXIL and avoiding invalid fragment-shader builtins.
Changes:
- Emit a null constant of the declared input type and remove the obsolete pixel-shader builtin path.
- Add bool and uint regression cases with SPIR-V validation enabled.
- Document the behavior fix in the release notes.
| File | Description |
|---|---|
tools/clang/test/CodeGenSPIRV/meshshading.ext.cullprimitive.ps.hlsl |
Adds bool and uint pixel-shader regression cases. |
tools/clang/lib/SPIRV/DeclResultIdMapper.cpp |
Replaces pixel-shader cull inputs with typed null constants. |
docs/ReleaseNotes.md |
Documents the DXIL-compatible behavior. |
Review effort: Balanced
Jonathan Zakharov (jzakharovnv)
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround. Code looks good, the testing just needs a little more coverage, which I've left some ideas for.
Check with -fcgl that the input becomes a null constant of the declared type, for bool, uint, bool2 and bool[2]. Add a struct shared between a mesh shader primitive output and a pixel shader input, with SV_CullPrimitive first and not first: the other members keep the mesh shader's locations and the 0 lands in the right member.
|
Thanks for these, pushed tests for all three:
Both fail without the fix. Also, while I was in there I noticed DXIL crashes on a |
There was a problem hiding this comment.
🔵 Needs a closer look
The early return bypasses explicit vk::builtin overrides, silently replacing valid helper-invocation input with false; declarations with VKBuiltInAttr need to retain the existing mapping, with regression coverage.
2 open issues
Review effort: Balanced
An explicit vk::builtin took precedence over SV_CullPrimitive before the PS input was replaced with 0. Skip the replacement when the attribute is present.
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
# Conflicts: # docs/ReleaseNotes.md

Fixes #9084.
Thanks again for the quick go-ahead on this one. DXIL replaces a pixel shader
SV_CullPrimitiveinput with 0, but the SPIR-V backend mapped it toCullPrimitiveEXT, which needsMeshShadingEXTand isn't allowed in fragment shaders, so the output failed validation.This mirrors DXIL and uses a null constant of the declared type instead (
falsefor bool, 0 for uint). I also removed the old PSIn builtin path, since nothing reaches it anymore.I added
meshshading.ext.cullprimitive.ps.hlsl, which checks bool and uint without-fcglso spirv-val runs on the output. It fails without the change, and the CodeGenSPIRV suite passes.Jonathan Zakharov (@jzakharovnv) as promised, here it is :)