Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gl_PrimitiveID in GLSL fragment shader causes SPIR-V validation error #6007

Open
juliusikkala opened this issue Jan 5, 2025 · 3 comments
Open
Assignees
Labels
goal:quality & productivity Quality issues and issues that impact our productivity coding day to day inside slang

Comments

@juliusikkala
Copy link
Contributor

gl_PrimitiveID should be available in fragment shaders as well. Currently, if you try to use it, a SPIR-V validation issue occurs:

error: line 8: Operand 3 of Decorate requires one of these capabilities: Geometry Tessellation RayTracingKHR MeshShadingNV MeshShadingEXT RayTracingNV 
  OpDecorate %gl_PrimitiveID BuiltIn PrimitiveId

This is because OpCapability Geometry should be required if gl_PrimitiveID is used in a fragment shader.

Analysis:

This case would work correctly if glsl.meta.slang had just public in int gl_PrimitiveID : SV_PrimitiveID;, because there's already logic in slang-emit-spirv.cpp to handle SV_PrimitiveID correctly. Unfortunately, gl_PrimitiveID is also used in the ray tracing shaders, and to handle that use case, glsl.meta.slang actually has this instead:

public property int gl_PrimitiveID 
{
    [require(cuda_glsl_hlsl_spirv, raytracing_anyhit_closesthit_intersection)]
    get 
    {
        setupExtForRayTracingBuiltIn();
        return PrimitiveIndex();
    }
}

This PrimitiveIndex() generates PrimitiveId using spirv_asm. When done this way, the SPIR-V emitter does not add the required capability in a fragment shader, but behaves correctly in the ray tracing shaders as they don't need additional capabilities for this builtin.

It would probably not be too hard to just check through the SPIRVAsmOperandBuiltinVars and require the Geometry capability if PrimitiveId is encountered, but that feels "hacky" to me and doesn't fix the GLSL emitter unnecessarily requiring GL_EXT_ray_tracing because of setupExtForRayTracingBuiltIn(). I'm not yet super familiar with the intricacies here; is there a way to have this gl_PrimitiveID property behave differently depending on what shader stage is accessing it?

@juliusikkala juliusikkala changed the title gl_PrimitiveID in GLSL fragment shader gl_PrimitiveID in GLSL fragment shader causes SPIR-V validation error Jan 5, 2025
@juliusikkala
Copy link
Contributor Author

For now, I implemented the hacky solution outlined above that checks the SPIRVAsmOperandBuiltinVars in the SPIR-V emitter, it's here. This feels like the wrong solution for this, so I'm not submitting a PR of it yet.

@andreasschultes
Copy link

There is another validation problem with the SV_PrimitiveID

 msgNum: 115483881 - Validation Error: [ VUID-VkShaderModuleCreateInfo-pCode-08740 ] | MessageID = 0x6e224e9 | vkCreateShaderModule():  SPIR-V Capability Geometry was declared, but one of the following requirements is required (VkPhysicalDeviceFeatures::geometryShader).
The Vulkan spec states: If pCode is a pointer to SPIR-V code, and pCode declares any of the capabilities listed in the SPIR-V Environment appendix, one of the corresponding requirements must be satisfied (https://vulkan.lunarg.com/doc/view/1.3.296.0/windows/1.3-extensions/vkspec.html#VUID-VkShaderModuleCreateInfo-pCode-08740)

The spec says that:
The fragment shader using PrimitiveId will need to declare either the MeshShadingNV, MeshShadingEXT, Geometry or Tessellation capability to satisfy the requirement SPIR-V has to use PrimitiveId.
https://registry.khronos.org/vulkan/specs/latest/man/html/PrimitiveId.html

Currently the implementation add the Geometry shader if it in the fragment shader.
Better solution is to check for an existing valid capability and only add the capability as fallback.

Something like that would it be in slang:

[require(spvTessellation)]
[[shader("fragment")]]
float4 main(uint primitiveID: SV_PrimitiveID) {

Current Implementation:

else if (semanticName == "sv_primitiveid")

@bmillsNV bmillsNV added the goal:quality & productivity Quality issues and issues that impact our productivity coding day to day inside slang label Jan 7, 2025
@bmillsNV bmillsNV added this to the Q1 2025 (Winter) milestone Jan 7, 2025
@csyonghe
Copy link
Collaborator

csyonghe commented Jan 9, 2025

This is running into a corner case where the most elegant solution is to __target_switch on stages, but this isn't currently supported by the compiler yet.

Without __target_switch on stages, we can't easily implement this as pure library code, and will have to expose a new instruction like kIROp_GetPrimitiveId, and then implement the emit logic for this instruction for each backend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
goal:quality & productivity Quality issues and issues that impact our productivity coding day to day inside slang
Projects
None yet
Development

No branches or pull requests

5 participants