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

OpTypeImage's format mismatching with that of the bound VkImageView is ignored. #9129

Open
nicebyte opened this issue Jan 4, 2025 · 3 comments
Assignees
Labels
Incomplete Missing Validation VUs to be added ShaderVal Shader Validation (SPIR-V related) SpecChange Issues that require a change in the Vulkan Spec

Comments

@nicebyte
Copy link

nicebyte commented Jan 4, 2025

Environment:
SDK ver 1.3.296

Describe the Issue

Consider the following HLSL:

[[vk::binding(0, 0)]] RWTexture2D<float4> outputImage;

[numthreads(4, 4, 1)] void CSMain(uint3 tid
                                  : SV_DispatchThreadID) {
  outputImage[tid.xy] = float4(sin(tid.x),cos(tid.y),0.0,1.0);
}

note the absence of a format qualifier for outputImage.

This code produces the following SPIR-V:

               OpCapability Shader
          %1 = OpExtInstImport "GLSL.std.450"
               OpMemoryModel Logical GLSL450
               OpEntryPoint GLCompute %CSMain "CSMain" %gl_GlobalInvocationID
               OpExecutionMode %CSMain LocalSize 4 4 1
               OpDecorate %gl_GlobalInvocationID BuiltIn GlobalInvocationId
               OpDecorate %outputImage DescriptorSet 0
               OpDecorate %outputImage Binding 0
      %float = OpTypeFloat 32
    %float_0 = OpConstant %float 0
    %float_1 = OpConstant %float 1
%type_2d_image = OpTypeImage %float 2D 2 0 0 2 Rgba32f
%_ptr_UniformConstant_type_2d_image = OpTypePointer UniformConstant %type_2d_image
       %uint = OpTypeInt 32 0
     %v3uint = OpTypeVector %uint 3
%_ptr_Input_v3uint = OpTypePointer Input %v3uint
       %void = OpTypeVoid
         %15 = OpTypeFunction %void
    %v4float = OpTypeVector %float 4
     %v2uint = OpTypeVector %uint 2
%outputImage = OpVariable %_ptr_UniformConstant_type_2d_image UniformConstant
%gl_GlobalInvocationID = OpVariable %_ptr_Input_v3uint Input
     %CSMain = OpFunction %void None %15
         %18 = OpLabel
         %19 = OpLoad %v3uint %gl_GlobalInvocationID
         %20 = OpCompositeExtract %uint %19 0
         %21 = OpConvertUToF %float %20
         %22 = OpExtInst %float %1 Sin %21
         %23 = OpCompositeExtract %uint %19 1
         %24 = OpConvertUToF %float %23
         %25 = OpExtInst %float %1 Cos %24
         %26 = OpCompositeConstruct %v4float %22 %25 %float_0 %float_1
         %27 = OpVectorShuffle %v2uint %19 %19 0 1
         %28 = OpLoad %type_2d_image %outputImage
               OpImageWrite %28 %27 %26 None
               OpReturn
               OpFunctionEnd

Note the line: %type_2d_image = OpTypeImage %float 2D 2 0 0 2 Rgba32f

According to the table from here: https://docs.vulkan.org/spec/latest/appendices/spirvenv.html#spirvenv-image-formats , the SPIR-V Rgba32f format maps to VK_FORMAT_R32G32B32A32_SFLOAT.

However, I am using this shader with VK_FORMAT_R8G8B8A8_UNORM, and not getting any validation errors at all.

As far as I am aware, on hardware that supports StorageImageWriteWithoutFormat capability (which is what I'm using), the format of OpTypeImage is simply ignored, so no harm done. However, on GPUs that do care about the format qualifier, my usage could potentially corrupt the image. Should this be something that VVL warns about?

Expected behavior

I'd expect a validation error/warning saying that the format of the view is not matching what the SPIR-V module expects.

Valid Usage ID

I don't actually know if it's violating any specific VUIDs, but still seems like a clear case of erroneous API usage.

@nicebyte
Copy link
Author

nicebyte commented Jan 4, 2025

note: i did try with GPU-assisted validation and still got no error messages.

@spencer-lunarg
Copy link
Contributor

We definitely have this check (I have added many tests for it) but I bet the HLSL here is producing something we might be not detecting, will take a look today and hopefully its a quick fix to sneak into the next SDK

@spencer-lunarg
Copy link
Contributor

spencer-lunarg commented Jan 6, 2025

so I was mistaken, there is no errors for this as you mentioned

It seems things like VUID-RuntimeSpirv-OpImageWrite-07112 and VUID-vkCmdDraw-OpImageWrite-08795 show that only thing that matters is the number of components (which your example has 4 and 4)

It seems there is a section in the spec about this (vkspec.html#textures-output-validation) and seems there are no VUs because this is no undefined behavior but rather undefined value that are written

We 100% should add these as it is not obvious to not notice this (this might take a bit as I will need to add a lot of tests as there are a lot of edge cases to get correct)

(simple test for start with later)

TEST_F(NegativeShaderImageAccess, xxx) {
    AddRequiredFeature(vkt::Feature::fragmentStoresAndAtomics);
    RETURN_IF_SKIP(Init());
    InitRenderTarget();

    char const *fsSource = R"glsl(
        #version 450
        layout(set = 0, binding = 0, rgba32f) uniform image2D si0;
        void main() {
            imageStore(si0, ivec2(0), vec4(0));
        }
    )glsl";
    VkShaderObj fs(this, fsSource, VK_SHADER_STAGE_FRAGMENT_BIT);

    vkt::Image image(*m_device, 16, 16, 1, VK_FORMAT_B8G8R8A8_UNORM, VK_IMAGE_USAGE_STORAGE_BIT);
    image.SetLayout(VK_IMAGE_LAYOUT_GENERAL);
    vkt::ImageView image_view = image.CreateView();

    OneOffDescriptorSet descriptor_set(m_device,
                                       {
                                           {0, VK_DESCRIPTOR_TYPE_STORAGE_IMAGE, 1, VK_SHADER_STAGE_ALL, nullptr},
                                       });
    vkt::PipelineLayout pipeline_layout(*m_device, {&descriptor_set.layout_});

    descriptor_set.WriteDescriptorImageInfo(0, image_view, VK_NULL_HANDLE, VK_DESCRIPTOR_TYPE_STORAGE_IMAGE, VK_IMAGE_LAYOUT_GENERAL);
    descriptor_set.UpdateDescriptorSets();

    CreatePipelineHelper pipe(*this);
    pipe.shader_stages_ = {pipe.vs_->GetStageCreateInfo(), fs.GetStageCreateInfo()};
    pipe.gp_ci_.layout = pipeline_layout.handle();
    pipe.CreateGraphicsPipeline();

    m_command_buffer.Begin();
    m_command_buffer.BeginRenderPass(m_renderPassBeginInfo);

    vk::CmdBindPipeline(m_command_buffer.handle(), VK_PIPELINE_BIND_POINT_GRAPHICS, pipe.Handle());
    vk::CmdBindDescriptorSets(m_command_buffer.handle(), VK_PIPELINE_BIND_POINT_GRAPHICS, pipeline_layout.handle(), 0, 1,
                              &descriptor_set.set_, 0, nullptr);
    vk::CmdDraw(m_command_buffer.handle(), 3, 1, 0, 0);
    m_command_buffer.EndRenderPass();
    m_command_buffer.End();
}

@spencer-lunarg spencer-lunarg added Incomplete Missing Validation VUs to be added ShaderVal Shader Validation (SPIR-V related) SpecChange Issues that require a change in the Vulkan Spec labels Jan 6, 2025
@spencer-lunarg spencer-lunarg self-assigned this Jan 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Incomplete Missing Validation VUs to be added ShaderVal Shader Validation (SPIR-V related) SpecChange Issues that require a change in the Vulkan Spec
Projects
None yet
Development

No branches or pull requests

2 participants