Descriptor validation for OpTypeImage prematurely checks signedness

[aitor-lunarg]

Describe the Issue As stated in the title, if we have an OpTypeImage (https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#OpTypeImage) with a mismatching Sampled Type of the bound image we will get VUID-vkCmdDispatch-None-02699 even if the access respects Image Format's signedness.

The spec (https://registry.khronos.org/vulkan/specs/1.3-extensions/html/vkspec.html#spirvenv-format-type-matching) states that The signedness must match the signedness of any access to the image. Spir-v 1.4 introduces SignExtend and ZeroExtend to force signedness when accessing the resource, so until resource access, we won't know if the access is correct as stated in https://registry.khronos.org/vulkan/specs/1.3-extensions/html/vkspec.html#spirvenv-image-signedness

Valid Usage ID VUID-vkCmdDispatch-None-02699

Environment:

• OS: Any
• GPU: Any
• SDK or header version if building from repo: ToT
• Options enabled (synchronization, best practices, etc.):

Additional context Here's a test to replicate the issue:

TEST_F(VkPositiveLayerTest, ImageSampleResultMismatch) {
TEST_DESCRIPTION("Try to sample an image with mismatching sample result from image format.");

    m_errorMonitor->ExpectSuccess();

    SetTargetApiVersion(VK_API_VERSION_1_1);
    AddRequiredExtensions(VK_KHR_SPIRV_1_4_EXTENSION_NAME);
    ASSERT_NO_FATAL_FAILURE(InitFramework(m_errorMonitor));

    if (DeviceValidationVersion() < VK_API_VERSION_1_1) {
        GTEST_SKIP() << "At least Vulkan version 1.1 is required";
    }

    ASSERT_NO_FATAL_FAILURE(InitState());

    if (!AreRequiredExtensionsEnabled()) {
        GTEST_SKIP() << RequiredExtensionsNotSupported() << " not supported.";
    }

    const char *csSource = R"(
                          OpCapability Shader
                          OpCapability StorageImageExtendedFormats
                          %std = OpExtInstImport "GLSL.std.450"
                          OpMemoryModel Logical GLSL450
                          OpEntryPoint GLCompute %main "main" %image_ptr
                          OpExecutionMode %main LocalSize 1 1 1
                          OpSource GLSL 450
                          OpDecorate %const_vec3_u32_111 BuiltIn WorkgroupSize
                          OpDecorate %image_ptr DescriptorSet 0
                          OpDecorate %image_ptr Binding 0
                          OpDecorate %image_ptr NonReadable
%type_void              = OpTypeVoid
%type_u32               = OpTypeInt 32 0
%type_i32               = OpTypeInt 32 1
%type_vec3_u32          = OpTypeVector %type_u32 3
%type_vec4_u32          = OpTypeVector %type_u32 4
%type_vec2_i32          = OpTypeVector %type_i32 2
%type_fn_void           = OpTypeFunction %type_void
%type_ptr_fn_vec4_u32   = OpTypePointer Function %type_vec4_u32
%type_image             = OpTypeImage %type_u32 2D 0 0 0 2 Rgba32i
%type_ptr_image         = OpTypePointer UniformConstant %type_image
%image_ptr              = OpVariable %type_ptr_image UniformConstant
%const_i32_0            = OpConstant %type_i32 0
%const_vec2_i32_00      = OpConstantComposite %type_vec2_i32 %const_i32_0 %const_i32_0
%const_u32_1            = OpConstant %type_u32 1
%const_vec3_u32_111     = OpConstantComposite %type_vec3_u32 %const_u32_1 %const_u32_1 %const_u32_1
%main                   = OpFunction %type_void None %type_fn_void
%label                  = OpLabel
%store_location         = OpVariable %type_ptr_fn_vec4_u32 Function
%image                  = OpLoad %type_image %image_ptr
%value                  = OpImageRead %type_vec4_u32 %image %const_vec2_i32_00 SignExtend
                          OpStore %store_location %value
                          OpReturn
                          OpFunctionEnd
              )";

    OneOffDescriptorSet ds(m_device, {
                                         {0, VK_DESCRIPTOR_TYPE_STORAGE_IMAGE, 1, VK_SHADER_STAGE_COMPUTE_BIT, nullptr},
                                     });

    CreateComputePipelineHelper cs_pipeline(*this);
    cs_pipeline.InitInfo();
    cs_pipeline.cs_.reset(new VkShaderObj(this, csSource, VK_SHADER_STAGE_COMPUTE_BIT, SPV_ENV_VULKAN_1_1_SPIRV_1_4, SPV_SOURCE_ASM));
    cs_pipeline.InitState();
    cs_pipeline.pipeline_layout_ = VkPipelineLayoutObj(m_device, {&ds.layout_});
    cs_pipeline.LateBindPipelineInfo();
    cs_pipeline.cp_ci_.stage.stage = VK_SHADER_STAGE_COMPUTE_BIT;  // override with wrong value
    cs_pipeline.CreateComputePipeline(true, false);                // need false to prevent late binding

    VkFormat format = VK_FORMAT_R32G32B32A32_SINT;

    VkImageObj image(m_device);
    image.Init(32, 32, 1, format, VK_IMAGE_USAGE_STORAGE_BIT, VK_IMAGE_TILING_OPTIMAL);

    VkDescriptorImageInfo image_info = {};
    image_info.imageView = image.targetView(format);
    image_info.imageLayout = VK_IMAGE_LAYOUT_GENERAL;

    VkWriteDescriptorSet descriptor_write = LvlInitStruct<VkWriteDescriptorSet>();
    descriptor_write.dstSet = ds.set_;
    descriptor_write.dstBinding = 0;
    descriptor_write.descriptorCount = 1;
    descriptor_write.descriptorType = VK_DESCRIPTOR_TYPE_STORAGE_IMAGE;
    descriptor_write.pImageInfo = &image_info;
    vk::UpdateDescriptorSets(m_device->device(), 1, &descriptor_write, 0, NULL);

    m_commandBuffer->begin();

    {
        VkImageMemoryBarrier img_barrier = LvlInitStruct<VkImageMemoryBarrier>();
        img_barrier.srcAccessMask = VK_ACCESS_HOST_READ_BIT;
        img_barrier.dstAccessMask = VK_ACCESS_SHADER_READ_BIT;
        img_barrier.oldLayout = VK_IMAGE_LAYOUT_UNDEFINED;
        img_barrier.newLayout = VK_IMAGE_LAYOUT_GENERAL;
        img_barrier.image = image.handle();
        img_barrier.srcQueueFamilyIndex = VK_QUEUE_FAMILY_IGNORED;
        img_barrier.dstQueueFamilyIndex = VK_QUEUE_FAMILY_IGNORED;
        img_barrier.subresourceRange.aspectMask = VK_IMAGE_ASPECT_COLOR_BIT;
        img_barrier.subresourceRange.baseArrayLayer = 0;
        img_barrier.subresourceRange.baseMipLevel = 0;
        img_barrier.subresourceRange.layerCount = 1;
        img_barrier.subresourceRange.levelCount = 1;
        vk::CmdPipelineBarrier(m_commandBuffer->handle(), VK_PIPELINE_STAGE_HOST_BIT, VK_PIPELINE_STAGE_COMPUTE_SHADER_BIT, 0,
                                0, nullptr, 0, nullptr, 1, &img_barrier);
    }

    vk::CmdBindPipeline(m_commandBuffer->handle(), VK_PIPELINE_BIND_POINT_COMPUTE, cs_pipeline.pipeline_);
    vk::CmdBindDescriptorSets(m_commandBuffer->handle(), VK_PIPELINE_BIND_POINT_COMPUTE, cs_pipeline.pipeline_layout_.handle(),
                                0, 1, &ds.set_, 0, nullptr);

    vk::CmdDispatch(m_commandBuffer->handle(), 1, 1, 1);

    m_commandBuffer->end();

    m_errorMonitor->VerifyNotFound();
}

The main issue being %type_image = OpTypeImage %type_u32 2D 0 0 0 2 Rgba32i since the sample result is unsigned instead of signed.

Can also be replicated using CTS dEQP-VK.image.extend_operands_spirv1p4.*.mismatched_sign.*

[ncesario-lunarg]

I feel like this has been reported elsewhere as well, but can't find it at the moment, but yeah, pretty sure this is a known problem unfortunately :/

[ziga-lunarg]

@ncesario-lunarg I've asked this in the chat about a week ago, maybe that's what you're thinking of.

[ncesario-lunarg]

Yep @ziga-lunarg, that's what I was thinking of, just completely forgot where I saw it :)

[sjfricke]

So I looked into this, I think this would benefit from the Instruction class I have talked to @ncesario-lunarg about. The main issue is if a shader looks like

%image  = OpLoad %type_image %image_ptr
%value1 = OpImageRead %type_vec4_u32 %image %const_vec2_i32_00 SignExtend
%value2 = OpImageRead %type_vec4_u32 %image %const_vec2_i32_00

would still be invalid, so currently at draw time we have data stored in the descriptors (ex is_write_without_format) but what is really needed is a way to look at the access instructions (OpImageReads) at draw time, but the SHADER_MODULE_STATE is not around. Having a separate Instruction class could be copied to hold and in this case, it could be used to check if SignExtend is set... I hit this similar issue of needing the actual instruction for some other SPIRV validation not yet implemented (VUID-vkCmdDispatch-None-04115 and VUID-vkCmdDispatch-OpImageWrite-04469)

[spencer-lunarg]

@aitor-lunarg I still need to add the spirv-val code to catch cases where you mix signedness, but can you confirm the false positive are gone from CTS now