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

OpSampledImage extra validation check does not properly handle shadow samplers. #5770

Closed
martin-ejdestig opened this issue Aug 20, 2024 · 4 comments

Comments

@martin-ejdestig
Copy link

martin-ejdestig commented Aug 20, 2024

Noticed this when upgrading to Vulkan SDK 1.3.290 from 1.3.280 that Vulkan-ValidationLayers, that includes SPIRV-Tools with #5695 merged, started warning about:

layout(location = 0) out vec4 out_color;

layout(set = 0, binding = 3) uniform sampler shadow_linear_compare_sampler;
layout(set = 0, binding = 4) uniform texture2D shadow_map_atlas_directional;

void main()
{
    float x = texture(sampler2DShadow(shadow_map_atlas_directional, shadow_linear_compare_sampler), vec3(0.5));
    out_color = vec4(vec3(x), 1.0);
}

Error log message:

Expected Image to have the same type as Result Type Image
  %20 = OpSampledImage %19 %13 %17
. The Vulkan spec states: If pCode is a pointer to SPIR-V code, pCode must adhere to the validation rules described by the Validation Rules within a Module section of the SPIR-V Environment appendix (https://www.khronos.org/registry/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-VkShaderModuleCreateInfo-pCode-08737)

Passing a sampler to sampler2DShadow() is valid though? Looks like it when reading GLSL 4.60.8 specification section 5.4.5. Texture-Combined Sampler Constructors.

Test for it in glslang: https://github.com/KhronosGroup/glslang/blob/592de6cf78e0fb359fc3e2854c9dc0f3cf6d4820/Test/vulkan.frag#L25
glslang commit that made it valid: KhronosGroup/glslang@7d4c9a0
glslang commit message links to: KhronosGroup/GLSL#22
Original VVL issue: KhronosGroup/Vulkan-ValidationLayers#8413

@alan-baker
Copy link
Contributor

I think this comes down to the compiler (glslang in this case) not generating correct SPIR-V. Glslang should be declaring the image with a depth operand of either 1 or 2 in this case.

@arcady-lunarg
Copy link

This is a bug in glslang codegen, being tracked in this glslang issue: KhronosGroup/glslang#3653
The GLSL construct is legal, it's just that the SPIR-V being generated by glslang is wrong.

@martin-ejdestig
Copy link
Author

martin-ejdestig commented Aug 21, 2024

I know too little about SPIR-V. I figured the OpTypeImage came (partly?) from the sampler. Changing to samplerShadow shadow_linear_compare_sampler makes it so depth is set to 1 (at least in the simple shader pasted above, not in the real shader code is copied from, which is weird).

But if this is something that can be fixed in glslang only, feel free to close this issue. :)

@martin-ejdestig
Copy link
Author

Closing since duplicated with #5781 .

@martin-ejdestig martin-ejdestig closed this as not planned Won't fix, can't repro, duplicate, stale Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants