search

KhronosGroup / Vulkan-ValidationLayers

Vulkan Validation Layers (VVL)
https://vulkan.lunarg.com/doc/sdk/latest/linux/khronos_validation_layer.html
Other
748 stars 400 forks source link

SPIR-V validation fails for shadow sampler #8413

Closed martin-ejdestig closed 2 days ago

martin-ejdestig commented 3 weeks ago

Environment:

Describe the Issue

After upgrading to Vulkan SDK 1.3.290, VVL outputs the following to debug callback when passing SPIR-V generated from GLSL that uses a shadow sampler to vkCreateShaderModule() and vkCreateGraphicsPipelines():

Validation Error: [ VUID-VkShaderModuleCreateInfo-pCode-08737 ] | MessageID = 0xa5625282 | vkCreateShaderModule(): pCreateInfo->pCode (spirv-val produced an error):
Expected Image to have the same type as Result Type Image
  %373 = OpSampledImage %372 %368 %370
. 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://vulkan.lunarg.com/doc/view/1.3.290.0/windows/1.3-extensions/vkspec.html#VUID-VkShaderModuleCreateInfo-pCode-08737)
Validation Error: [ VUID-VkPipelineShaderStageCreateInfo-pSpecializationInfo-06849 ] | MessageID = 0x437c19d3 | vkCreateGraphicsPipelines(): pCreateInfos[0].pStages[1] After specialization was applied, VkShaderModule 0x5b26f60000000100[] produces a spirv-val error (stage VK_SHADER_STAGE_FRAGMENT_BIT):
Expected Image to have the same type as Result Type Image
  %373 = OpSampledImage %372 %368 %370
. The Vulkan spec states: If a shader module identifier is not specified, the shader code used by the pipeline must be valid as described by the Khronos SPIR-V Specification after applying the specializations provided in pSpecializationInfo, if any, and then converting all specialization constants into fixed constants (https://vulkan.lunarg.com/doc/view/1.3.290.0/windows/1.3-extensions/vkspec.html#VUID-VkPipelineShaderStageCreateInfo-pSpecializationInfo-06849)

Relevant SPIR-V code:

         %57 = OpTypeImage %float 2D 0 0 0 1 Unknown
%_ptr_UniformConstant_57 = OpTypePointer UniformConstant %57
...
%shadow_linear_compare_sampler = OpVariable %_ptr_UniformConstant_357 UniformConstant
        %371 = OpTypeImage %float 2D 1 0 0 1 Unknown
        %372 = OpTypeSampledImage %371
...
%shadow_map_sample_pcf2x2_t21_vf2_f1_ = OpFunction %float None %66
%shadow_map_atlas_0 = OpFunctionParameter %_ptr_UniformConstant_57
       %uv_0 = OpFunctionParameter %_ptr_Function_v2float
%shadow_pos_z = OpFunctionParameter %_ptr_Function_float
         %71 = OpLabel
               OpLine %10 188 0
        %368 = OpLoad %57 %shadow_map_atlas_0
        %370 = OpLoad %357 %shadow_linear_compare_sampler
        %373 = OpSampledImage %372 %368 %370
        %374 = OpLoad %v2float %uv_0
        %375 = OpLoad %float %shadow_pos_z
        %376 = OpCompositeExtract %float %374 0
        %377 = OpCompositeExtract %float %374 1
        %378 = OpCompositeConstruct %v3float %376 %377 %375
        %379 = OpCompositeExtract %float %378 2
        %380 = OpImageSampleDrefImplicitLod %float %373 %378 %379
               OpReturnValue %380
               OpFunctionEnd

Generated from the following GLSL code:

...
layout(set = 0, binding = 2) uniform sampler shadow_nearest_sampler;
layout(set = 0, binding = 3) uniform sampler shadow_linear_compare_sampler;
layout(set = 0, binding = 4) uniform texture2D shadow_map_atlas_directional;
layout(set = 0, binding = 5) uniform texture2D shadow_map_atlas_punctual;
...

// shadow_map_atlas_directional (and shadow_map_atlas_punctual) passed as argument.
float shadow_map_sample_pcf2x2(texture2D shadow_map_atlas, vec2 uv, float shadow_pos_z)
{
    return texture(sampler2DShadow(shadow_map_atlas, shadow_linear_compare_sampler), vec3(uv, shadow_pos_z));
}

I have not had time to test with a minimum example, the above is extracted from a much larger shader. But I think I have included all the relevant SPIR-V (and GLSL it is generated from) that triggers the error messages.

martin-ejdestig commented 3 weeks ago

Searched through external/ as well and it looks like the "Expected Image to have the same type as Result Type Image" error message was added in https://github.com/KhronosGroup/SPIRV-Tools/pull/5695 .

martin-ejdestig commented 3 weeks ago

Below is complete debug SPIR-V assembly of minimal shader that warns when passed to vkCreateShaderModule(). There is no longer any warning when calling vkCreateGraphicsPipelines() since specialization constants are removed (I assume).

; SPIR-V
; Version: 1.6
; Generator: Google Shaderc over Glslang; 11
; Bound: 36
; Schema: 0
               OpCapability Shader
          %2 = OpExtInstImport "GLSL.std.450"
               OpMemoryModel Logical GLSL450
               OpEntryPoint Fragment %main "main" %shadow_map_atlas_directional %shadow_linear_compare_sampler %out_color
               OpExecutionMode %main OriginUpperLeft
          %1 = OpString "shaders/mesh_custom_test.frag"
               OpSource GLSL 460 %1 "#version 460

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);
}
"
               OpSourceExtension "GL_GOOGLE_cpp_style_line_directive"
               OpSourceExtension "GL_GOOGLE_include_directive"
               OpName %main "main"
               OpName %x "x"
               OpName %shadow_map_atlas_directional "shadow_map_atlas_directional"
               OpName %shadow_linear_compare_sampler "shadow_linear_compare_sampler"
               OpName %out_color "out_color"
               OpModuleProcessed "entry-point main"
               OpModuleProcessed "client vulkan100"
               OpModuleProcessed "target-env spirv1.6"
               OpModuleProcessed "target-env vulkan1.3"
               OpModuleProcessed "entry-point main"
               OpDecorate %shadow_map_atlas_directional DescriptorSet 0
               OpDecorate %shadow_map_atlas_directional Binding 4
               OpDecorate %shadow_linear_compare_sampler DescriptorSet 0
               OpDecorate %shadow_linear_compare_sampler Binding 3
               OpDecorate %out_color Location 0
       %void = OpTypeVoid
          %4 = OpTypeFunction %void
      %float = OpTypeFloat 32
%_ptr_Function_float = OpTypePointer Function %float
         %10 = OpTypeImage %float 2D 0 0 0 1 Unknown
%_ptr_UniformConstant_10 = OpTypePointer UniformConstant %10
%shadow_map_atlas_directional = OpVariable %_ptr_UniformConstant_10 UniformConstant
         %14 = OpTypeSampler
%_ptr_UniformConstant_14 = OpTypePointer UniformConstant %14
%shadow_linear_compare_sampler = OpVariable %_ptr_UniformConstant_14 UniformConstant
         %18 = OpTypeImage %float 2D 1 0 0 1 Unknown
         %19 = OpTypeSampledImage %18
    %v3float = OpTypeVector %float 3
  %float_0_5 = OpConstant %float 0.5
         %23 = OpConstantComposite %v3float %float_0_5 %float_0_5 %float_0_5
    %v4float = OpTypeVector %float 4
%_ptr_Output_v4float = OpTypePointer Output %v4float
  %out_color = OpVariable %_ptr_Output_v4float Output
    %float_1 = OpConstant %float 1
               OpLine %1 8 11
       %main = OpFunction %void None %4
          %6 = OpLabel
          %x = OpVariable %_ptr_Function_float Function
               OpLine %1 10 0
         %13 = OpLoad %10 %shadow_map_atlas_directional
         %17 = OpLoad %14 %shadow_linear_compare_sampler
         %20 = OpSampledImage %19 %13 %17
         %24 = OpCompositeExtract %float %23 2
         %25 = OpImageSampleDrefImplicitLod %float %20 %23 %24
               OpStore %x %25
               OpLine %1 11 0
         %29 = OpLoad %float %x
         %30 = OpCompositeConstruct %v3float %29 %29 %29
         %32 = OpCompositeExtract %float %30 0
         %33 = OpCompositeExtract %float %30 1
         %34 = OpCompositeExtract %float %30 2
         %35 = OpCompositeConstruct %v4float %32 %33 %34 %float_1
               OpStore %out_color %35
               OpReturn
               OpFunctionEnd

Error 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)
martin-ejdestig commented 3 weeks ago

Two things:

  1. I wonder if passing sampler instead of samplerShadow to sampler2DShadow() is valid. Reading the GLSL 4.60.8 specification section 5.4.5. Texture-Combined Sampler Constructors, it looks like it should be.
  2. If I change shadow_linear_compare_sampler to a samplerShadow in the simplified code (included in SPIR-V output), Depth in %18 = OpTypeImage changes to 1 (https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#OpTypeImage) and the VVL error message goes away. This does not work in the full shader. There is no difference in the SPIR-V generated assembly and I do not understand why. shadow_map_sample_pcf2x2() in extracted code from first post is the only place where shadow_linear_compare_sampler is used.

2 is moot if 1 is valid? There should be no warning here? I would also expect glslang/shaderc to error out if it was not valid. :shrug:

martin-ejdestig commented 3 weeks ago

Looks like it should be valid. Found test for it in https://github.com/KhronosGroup/glslang/blob/592de6cf78e0fb359fc3e2854c9dc0f3cf6d4820/Test/vulkan.frag#L25 . git blame on that line leads to https://github.com/KhronosGroup/glslang/commit/7d4c9a07b52e27fa80bf321782f4ac27dd22dad3 which links to https://github.com/KhronosGroup/GLSL/pull/22 .

spencer-lunarg commented 3 weeks ago

So I am like 95% certain you hit this https://github.com/KhronosGroup/glslang/issues/3653

martin-ejdestig commented 3 weeks ago

Is it not "just" that the validation added in https://github.com/KhronosGroup/SPIRV-Tools/pull/5695 is too strict? Is there something in the SPIR-V specification that says valid SPIR-V must be what SPIRV-Tools checks for? But then, that would be hard to align with what GLSL (and HLSL?) allow?

spencer-lunarg commented 3 weeks ago

I guess it will depend if there was a case where some hardware vendor actually wasn't doing it correctly, therefor the old "valid" SPIR-V was invalid actually and glsl/hlsl will need to generate different logic... this for sure will require some discussion internally in a SPIR-V meeting to sort out, but hopefully will be sorted out soon

spencer-lunarg commented 2 days ago

@martin-ejdestig we pulled in the spirv-val fix from https://github.com/KhronosGroup/SPIRV-Tools/pull/5789 in https://github.com/KhronosGroup/Vulkan-ValidationLayers/pull/8516 and I am now 99% sure this is fixed now

this hit a LOT of people in the ecosystem

martin-ejdestig commented 2 days ago

Verified that I do not get any warnings with ef846ac0883cde5e69ced0e7d7af59fe92f34e25 .