From aa3c21ca8118d72ae2bc62d0d6f5a0c7d8a02fbf Mon Sep 17 00:00:00 2001 From: spencer-lunarg Date: Tue, 25 Feb 2025 23:20:26 -0500 Subject: [PATCH] layers: Fix warning for Storage Image --- layers/drawdispatch/descriptor_validator.cpp | 52 ++++++++--- .../generated/spirv_validation_helper.cpp | 89 +++++++++++++++++++ .../generated/spirv_validation_helper.h | 3 + .../generators/spirv_validation_generator.py | 14 +++ tests/unit/shader_storage_image.cpp | 8 +- tests/unit/shader_storage_image_positive.cpp | 46 +++++++++- tests/unit/shader_storage_texel.cpp | 2 +- 7 files changed, 192 insertions(+), 22 deletions(-) diff --git a/layers/drawdispatch/descriptor_validator.cpp b/layers/drawdispatch/descriptor_validator.cpp index 3891f8c2e1d..73a6ca0ce26 100644 --- a/layers/drawdispatch/descriptor_validator.cpp +++ b/layers/drawdispatch/descriptor_validator.cpp @@ -17,7 +17,9 @@ #include "descriptor_validator.h" #include +#include #include "generated/spirv_grammar_helper.h" +#include "generated/spirv_validation_helper.h" #include "state_tracker/shader_stage_state.h" #include "error_message/error_strings.h" #include "state_tracker/descriptor_sets.h" @@ -387,15 +389,27 @@ bool DescriptorValidator::ValidateDescriptor(const spirv::ResourceInterfaceVaria if (image_view_ci.format != VK_FORMAT_UNDEFINED && resource_variable.info.image_format != VK_FORMAT_UNDEFINED && image_view_ci.format != resource_variable.info.image_format) { + // This warning was added after being discussed in https://gitlab.khronos.org/vulkan/vulkan/-/issues/4128 auto set = descriptor_set.Handle(); const LogObjectList objlist(set, image_view); - skip |= dev_state.LogUndefinedValue( - "Undefined-Value-StorageImage-FormatMismatch", objlist, loc, - "the %s is accessed by a OpTypeImage that has a format operand of %s which doesn't match the %s format (%s). Any " - "loads or stores with the variable will produce undefined values.", - DescribeDescriptor(resource_variable, index, descriptor_type).c_str(), - string_VkFormat(resource_variable.info.image_format), dev_state.FormatHandle(image_view).c_str(), - string_VkFormat(image_view_ci.format)); + std::stringstream msg; + msg << "the " << DescribeDescriptor(resource_variable, index, descriptor_type) + << " is accessed by a OpTypeImage that has a Format operand " + << string_SpirvImageFormat(resource_variable.info.image_format) << " (equivalent to " + << string_VkFormat(resource_variable.info.image_format) << ") which doesn't match the " + << dev_state.FormatHandle(image_view) << " format (" << string_VkFormat(image_view_ci.format) + << "). Any loads or stores with the variable will produce undefined values."; + if (vkuFormatCompatibilityClass(image_view_ci.format) == + vkuFormatCompatibilityClass(resource_variable.info.image_format)) { + msg << " While the formats are compatible, Storage Images must exactly match. Two ways to resolve this are\n"; + msg << "1. Set your ImageView to " << string_VkFormat(resource_variable.info.image_format) + << " and swizzle the values in the shader to match the desired results.\n"; + msg << "2. Use the Unknown format in your shader (will need the widely supported " + "shaderStorageImageWriteWithoutFormat feature)"; + } + msg << "\nSpec information at https://docs.vulkan.org/spec/latest/chapters/textures.html#textures-format-validation"; + skip |= dev_state.LogUndefinedValue("Undefined-Value-StorageImage-FormatMismatch-ImageView", objlist, loc, "%s", + msg.str().c_str()); } const bool image_format_width_64 = vkuFormatHasComponentSize(image_view_ci.format, 64); @@ -941,15 +955,25 @@ bool DescriptorValidator::ValidateDescriptor(const spirv::ResourceInterfaceVaria if (buffer_view_format != VK_FORMAT_UNDEFINED && resource_variable.info.image_format != VK_FORMAT_UNDEFINED && buffer_view_format != resource_variable.info.image_format) { + // This warning was added after being discussed in https://gitlab.khronos.org/vulkan/vulkan/-/issues/4128 auto set = descriptor_set.Handle(); const LogObjectList objlist(set, buffer_view); - skip |= dev_state.LogUndefinedValue( - "Undefined-Value-StorageImage-FormatMismatch", objlist, loc, - "the %s is accessed by a OpTypeImage that has a format operand of %s which doesn't match the %s format (%s). Any loads " - "or stores with the variable will produce undefined values.", - DescribeDescriptor(resource_variable, index, descriptor_type).c_str(), - string_VkFormat(resource_variable.info.image_format), dev_state.FormatHandle(buffer_view).c_str(), - string_VkFormat(buffer_view_format)); + std::stringstream msg; + msg << "the " << DescribeDescriptor(resource_variable, index, descriptor_type) + << " is accessed by a OpTypeImage that has a Format operand " + << string_SpirvImageFormat(resource_variable.info.image_format) << " (equivalent to " + << string_VkFormat(resource_variable.info.image_format) << ") which doesn't match the " + << dev_state.FormatHandle(buffer_view) << " format (" << string_VkFormat(buffer_view_format) + << "). Any loads or stores with the variable will produce undefined values."; + if (vkuFormatCompatibilityClass(buffer_view_format) == vkuFormatCompatibilityClass(resource_variable.info.image_format)) { + msg << " While the formats are compatible, Texel Buffers must exactly match. Two ways to resolve this are\n"; + msg << "1. Set your ImageView to " << string_VkFormat(resource_variable.info.image_format) + << " and swizzle the values in the shader to match the desired results.\n"; + msg << "2. Use the Unknown format in your shader"; + } + msg << "\nSpec information at https://docs.vulkan.org/spec/latest/chapters/textures.html#textures-format-validation"; + skip |= dev_state.LogUndefinedValue("Undefined-Value-StorageImage-FormatMismatch-BufferView", objlist, loc, "%s", + msg.str().c_str()); } const bool buffer_format_width_64 = vkuFormatHasComponentSize(buffer_view_format, 64); diff --git a/layers/vulkan/generated/spirv_validation_helper.cpp b/layers/vulkan/generated/spirv_validation_helper.cpp index 5e9aa296d4a..271e1649a4a 100644 --- a/layers/vulkan/generated/spirv_validation_helper.cpp +++ b/layers/vulkan/generated/spirv_validation_helper.cpp @@ -882,6 +882,95 @@ VkFormat CompatibleSpirvImageFormat(uint32_t spirv_image_format) { }; } +const char *string_SpirvImageFormat(VkFormat format) { + switch (format) { + case VK_FORMAT_R8_UNORM: + return "R8"; + case VK_FORMAT_R8_SNORM: + return "R8Snorm"; + case VK_FORMAT_R8_UINT: + return "R8ui"; + case VK_FORMAT_R8_SINT: + return "R8i"; + case VK_FORMAT_R8G8_UNORM: + return "Rg8"; + case VK_FORMAT_R8G8_SNORM: + return "Rg8Snorm"; + case VK_FORMAT_R8G8_UINT: + return "Rg8ui"; + case VK_FORMAT_R8G8_SINT: + return "Rg8i"; + case VK_FORMAT_R8G8B8A8_UNORM: + return "Rgba8"; + case VK_FORMAT_R8G8B8A8_SNORM: + return "Rgba8Snorm"; + case VK_FORMAT_R8G8B8A8_UINT: + return "Rgba8ui"; + case VK_FORMAT_R8G8B8A8_SINT: + return "Rgba8i"; + case VK_FORMAT_A2B10G10R10_UNORM_PACK32: + return "Rgb10A2"; + case VK_FORMAT_A2B10G10R10_UINT_PACK32: + return "Rgb10a2ui"; + case VK_FORMAT_R16_UNORM: + return "R16"; + case VK_FORMAT_R16_SNORM: + return "R16Snorm"; + case VK_FORMAT_R16_UINT: + return "R16ui"; + case VK_FORMAT_R16_SINT: + return "R16i"; + case VK_FORMAT_R16_SFLOAT: + return "R16f"; + case VK_FORMAT_R16G16_UNORM: + return "Rg16"; + case VK_FORMAT_R16G16_SNORM: + return "Rg16Snorm"; + case VK_FORMAT_R16G16_UINT: + return "Rg16ui"; + case VK_FORMAT_R16G16_SINT: + return "Rg16i"; + case VK_FORMAT_R16G16_SFLOAT: + return "Rg16f"; + case VK_FORMAT_R16G16B16A16_UNORM: + return "Rgba16"; + case VK_FORMAT_R16G16B16A16_SNORM: + return "Rgba16Snorm"; + case VK_FORMAT_R16G16B16A16_UINT: + return "Rgba16ui"; + case VK_FORMAT_R16G16B16A16_SINT: + return "Rgba16i"; + case VK_FORMAT_R16G16B16A16_SFLOAT: + return "Rgba16f"; + case VK_FORMAT_R32_UINT: + return "R32ui"; + case VK_FORMAT_R32_SINT: + return "R32i"; + case VK_FORMAT_R32_SFLOAT: + return "R32f"; + case VK_FORMAT_R32G32_UINT: + return "Rg32ui"; + case VK_FORMAT_R32G32_SINT: + return "Rg32i"; + case VK_FORMAT_R32G32_SFLOAT: + return "Rg32f"; + case VK_FORMAT_R32G32B32A32_UINT: + return "Rgba32ui"; + case VK_FORMAT_R32G32B32A32_SINT: + return "Rgba32i"; + case VK_FORMAT_R32G32B32A32_SFLOAT: + return "Rgba32f"; + case VK_FORMAT_R64_UINT: + return "R64ui"; + case VK_FORMAT_R64_SINT: + return "R64i"; + case VK_FORMAT_B10G11R11_UFLOAT_PACK32: + return "R11fG11fB10f"; + default: + return "Unknown SPIR-V Format"; + }; +} + // clang-format off static inline const char* SpvCapabilityRequirements(uint32_t capability) { static const vvl::unordered_map table { diff --git a/layers/vulkan/generated/spirv_validation_helper.h b/layers/vulkan/generated/spirv_validation_helper.h index d772c1bf782..ab044494741 100644 --- a/layers/vulkan/generated/spirv_validation_helper.h +++ b/layers/vulkan/generated/spirv_validation_helper.h @@ -29,5 +29,8 @@ // This is the one function that requires mapping SPIR-V enums to Vulkan enums VkFormat CompatibleSpirvImageFormat(uint32_t spirv_image_format); +// Since we keep things in VkFormat for checking, we need a way to get the original SPIR-V +// Format name for any error message +const char* string_SpirvImageFormat(VkFormat format); // NOLINTEND diff --git a/scripts/generators/spirv_validation_generator.py b/scripts/generators/spirv_validation_generator.py index 6ca178b7a76..5fd31bd7083 100644 --- a/scripts/generators/spirv_validation_generator.py +++ b/scripts/generators/spirv_validation_generator.py @@ -158,6 +158,9 @@ def generateHeader(self): // This is the one function that requires mapping SPIR-V enums to Vulkan enums VkFormat CompatibleSpirvImageFormat(uint32_t spirv_image_format); + // Since we keep things in VkFormat for checking, we need a way to get the original SPIR-V + // Format name for any error message + const char* string_SpirvImageFormat(VkFormat format); ''') self.write("".join(out)) @@ -270,6 +273,17 @@ def generateSource(self): out.append(' };\n') out.append('}\n') + out.append(''' + const char* string_SpirvImageFormat(VkFormat format) { + switch (format) { + ''') + for format in [x for x in self.vk.formats.values() if x.spirvImageFormat]: + out.append(f' case {format.name}:\n') + out.append(f' return \"{format.spirvImageFormat}\";\n') + out.append(' default:\n') + out.append(' return "Unknown SPIR-V Format";\n') + out.append(' };\n') + out.append('}\n') out.append(''' // clang-format off diff --git a/tests/unit/shader_storage_image.cpp b/tests/unit/shader_storage_image.cpp index 0161bee0c91..6e2def8bd39 100644 --- a/tests/unit/shader_storage_image.cpp +++ b/tests/unit/shader_storage_image.cpp @@ -911,7 +911,7 @@ void NegativeShaderStorageImage::FormatComponentMismatchTest(std::string spirv_f vk::CmdBindDescriptorSets(m_command_buffer.handle(), VK_PIPELINE_BIND_POINT_COMPUTE, pipeline_layout.handle(), 0, 1, &descriptor_set.set_, 0, nullptr); - m_errorMonitor->SetDesiredWarning("Undefined-Value-StorageImage-FormatMismatch"); + m_errorMonitor->SetDesiredWarning("Undefined-Value-StorageImage-FormatMismatch-ImageView"); vk::CmdDispatch(m_command_buffer.handle(), 1, 1, 1); m_errorMonitor->VerifyFound(); m_command_buffer.End(); @@ -923,6 +923,8 @@ TEST_F(NegativeShaderStorageImage, FormatComponentTypeMismatch) { TEST_F(NegativeShaderStorageImage, FormatComponentSizeMismatch) { FormatComponentMismatchTest("Rgba8", VK_FORMAT_R8_UNORM); } -TEST_F(NegativeShaderStorageImage, FormatComponentNumericMismatch) { - FormatComponentMismatchTest("Rgba8", VK_FORMAT_R8G8B8A8_SNORM); +TEST_F(NegativeShaderStorageImage, FormatComponentWidthMismatch) { + FormatComponentMismatchTest("Rgba8", VK_FORMAT_R32G32B32A32_SFLOAT); } + +TEST_F(NegativeShaderStorageImage, FormatCompatibleMismatch) { FormatComponentMismatchTest("Rgba8", VK_FORMAT_B8G8R8A8_UNORM); } diff --git a/tests/unit/shader_storage_image_positive.cpp b/tests/unit/shader_storage_image_positive.cpp index a36f070b838..fa5c2b84e64 100644 --- a/tests/unit/shader_storage_image_positive.cpp +++ b/tests/unit/shader_storage_image_positive.cpp @@ -1,8 +1,8 @@ /* - * Copyright (c) 2015-2024 The Khronos Group Inc. - * Copyright (c) 2015-2024 Valve Corporation - * Copyright (c) 2015-2024 LunarG, Inc. - * Copyright (c) 2015-2024 Google, Inc. + * Copyright (c) 2015-2025 The Khronos Group Inc. + * Copyright (c) 2015-2025 Valve Corporation + * Copyright (c) 2015-2025 LunarG, Inc. + * Copyright (c) 2015-2025 Google, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -14,6 +14,7 @@ #include "../framework/layer_validation_tests.h" #include "../framework/pipeline_helper.h" #include "../framework/descriptor_helper.h" +#include "error_message/log_message_type.h" class PositiveShaderStorageImage : public VkLayerTest {}; @@ -365,3 +366,40 @@ TEST_F(PositiveShaderStorageImage, UnknownWriteLessComponentMultiEntrypoint) { m_command_buffer.EndRenderPass(); m_command_buffer.End(); } + +TEST_F(PositiveShaderStorageImage, FormatTypeMatch) { + TEST_DESCRIPTION("Will not produce warning as formats are the same"); + RETURN_IF_SKIP(Init()); + m_errorMonitor->ExpectSuccess(kErrorBit | kWarningBit); + std::string cs_source = R"glsl( + #version 450 + layout(set = 0, binding = 0, Rgba8) uniform image2D si0; + void main() { + imageStore(si0, ivec2(0), vec4(0)); + } + )glsl"; + + vkt::Image image(*m_device, 4, 4, 1, VK_FORMAT_R8G8B8A8_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(); + + CreateComputePipelineHelper pipe(*this); + pipe.cs_ = std::make_unique(this, cs_source.c_str(), VK_SHADER_STAGE_COMPUTE_BIT); + pipe.cp_ci_.layout = pipeline_layout.handle(); + pipe.CreateComputePipeline(); + + m_command_buffer.Begin(); + vk::CmdBindPipeline(m_command_buffer.handle(), VK_PIPELINE_BIND_POINT_COMPUTE, pipe.Handle()); + vk::CmdBindDescriptorSets(m_command_buffer.handle(), VK_PIPELINE_BIND_POINT_COMPUTE, pipeline_layout.handle(), 0, 1, + &descriptor_set.set_, 0, nullptr); + vk::CmdDispatch(m_command_buffer.handle(), 1, 1, 1); + m_errorMonitor->VerifyFound(); +} \ No newline at end of file diff --git a/tests/unit/shader_storage_texel.cpp b/tests/unit/shader_storage_texel.cpp index e755b95805f..d4f86a87cd7 100644 --- a/tests/unit/shader_storage_texel.cpp +++ b/tests/unit/shader_storage_texel.cpp @@ -235,7 +235,7 @@ TEST_F(NegativeShaderStorageTexel, FormatComponentTypeMismatch) { vk::CmdBindPipeline(m_command_buffer.handle(), VK_PIPELINE_BIND_POINT_COMPUTE, pipe.Handle()); vk::CmdBindDescriptorSets(m_command_buffer.handle(), VK_PIPELINE_BIND_POINT_COMPUTE, pipeline_layout, 0, 1, &descriptor_set.set_, 0, nullptr); - m_errorMonitor->SetDesiredWarning("Undefined-Value-StorageImage-FormatMismatch"); + m_errorMonitor->SetDesiredWarning("Undefined-Value-StorageImage-FormatMismatch-BufferView"); vk::CmdDispatch(m_command_buffer.handle(), 1, 1, 1); m_errorMonitor->VerifyFound(); m_command_buffer.End();