From 952da96742a57efabe38d8b8d29757b8e68f1477 Mon Sep 17 00:00:00 2001 From: spencer-lunarg Date: Mon, 30 Oct 2023 17:21:58 +0900 Subject: [PATCH 1/4] spirv-val: Add Vulkan Image Format check --- source/val/validate_image.cpp | 161 +++++++++++++++- source/val/validation_state.cpp | 2 + test/val/val_image_test.cpp | 326 ++++++++++++++++++++++++++++++-- 3 files changed, 466 insertions(+), 23 deletions(-) diff --git a/source/val/validate_image.cpp b/source/val/validate_image.cpp index 39eeb4bd7e..f25e4b4efa 100644 --- a/source/val/validate_image.cpp +++ b/source/val/validate_image.cpp @@ -178,6 +178,97 @@ bool IsValidGatherLodBiasAMD(const ValidationState_t& _, spv::Op opcode) { return false; } +bool IsImageFormatInt32Width(spv::ImageFormat format) { + switch (format) { + case spv::ImageFormat::Rgba32i: + case spv::ImageFormat::Rgba16i: + case spv::ImageFormat::Rgba8i: + case spv::ImageFormat::R32i: + case spv::ImageFormat::Rg32i: + case spv::ImageFormat::Rg16i: + case spv::ImageFormat::Rg8i: + case spv::ImageFormat::R16i: + case spv::ImageFormat::R8i: + case spv::ImageFormat::Rgba32ui: + case spv::ImageFormat::Rgba16ui: + case spv::ImageFormat::Rgba8ui: + case spv::ImageFormat::R32ui: + case spv::ImageFormat::Rgb10a2ui: + case spv::ImageFormat::Rg32ui: + case spv::ImageFormat::Rg16ui: + case spv::ImageFormat::Rg8ui: + case spv::ImageFormat::R16ui: + case spv::ImageFormat::R8ui: + return true; + default: + break; + } + return false; +} + +bool IsImageFormatInt64Width(spv::ImageFormat format) { + switch (format) { + case spv::ImageFormat::R64ui: + case spv::ImageFormat::R64i: + return true; + default: + break; + } + return false; +} + +bool IsImageFormatIntSigned(spv::ImageFormat format) { + switch (format) { + case spv::ImageFormat::Rgba32i: + case spv::ImageFormat::Rgba16i: + case spv::ImageFormat::Rgba8i: + case spv::ImageFormat::R32i: + case spv::ImageFormat::Rg32i: + case spv::ImageFormat::Rg16i: + case spv::ImageFormat::Rg8i: + case spv::ImageFormat::R16i: + case spv::ImageFormat::R8i: + case spv::ImageFormat::R64i: + return true; + default: + break; + } + return false; +} + +bool IsImageFormatFloat(spv::ImageFormat format) { + switch (format) { + case spv::ImageFormat::Rgba32f: + case spv::ImageFormat::Rgba16f: + case spv::ImageFormat::R32f: + case spv::ImageFormat::Rgba8: + case spv::ImageFormat::Rgba8Snorm: + case spv::ImageFormat::Rg32f: + case spv::ImageFormat::Rg16f: + case spv::ImageFormat::R11fG11fB10f: + case spv::ImageFormat::R16f: + case spv::ImageFormat::Rgba16: + case spv::ImageFormat::Rgb10A2: + case spv::ImageFormat::Rg16: + case spv::ImageFormat::Rg8: + case spv::ImageFormat::R16: + case spv::ImageFormat::R8: + case spv::ImageFormat::Rgba16Snorm: + case spv::ImageFormat::Rg16Snorm: + case spv::ImageFormat::Rg8Snorm: + case spv::ImageFormat::R16Snorm: + case spv::ImageFormat::R8Snorm: + return true; + default: + break; + } + return false; +} + +bool IsImageFormatInt(spv::ImageFormat format) { + return IsImageFormatInt32Width(format) || IsImageFormatInt64Width(format); +} + // Returns true if the opcode is a Image instruction which applies // homogenous projection to the coordinates. bool IsProj(spv::Op opcode) { @@ -255,6 +346,8 @@ spv_result_t ValidateImageOperands(ValidationState_t& _, const bool have_explicit_mask = (word_index - 1 < num_words); const uint32_t mask = have_explicit_mask ? inst->word(word_index - 1) : 0u; + const auto target_env = _.context()->target_env; + if (have_explicit_mask) { // NonPrivate, Volatile, SignExtend, ZeroExtend take no operand words. const uint32_t mask_bits_having_operands = @@ -287,6 +380,41 @@ spv_result_t ValidateImageOperands(ValidationState_t& _, "multi-sampled image"; } + // The following OpTypeImage checks are done here as they depend of if the + // SignExtend and ZeroExtend are used to override the signedness + const bool is_sign_extend = + mask & uint32_t(spv::ImageOperandsMask::SignExtend); + const bool is_zero_extend = + mask & uint32_t(spv::ImageOperandsMask::ZeroExtend); + if (spvIsVulkanEnv(target_env)) { + if (info.format != spv::ImageFormat::Unknown && + _.IsIntScalarType(info.sampled_type)) { + const bool is_format_signed = IsImageFormatIntSigned(info.format); + // (vkspec.html#spirvenv-image-signedness) has order signedness is set by + bool is_sampled_type_signed = + is_sign_extend + ? true + : (is_zero_extend + ? false + : (_.IsSignedIntScalarType(info.sampled_type) ? true + : false)); + if (is_format_signed != is_sampled_type_signed) { + return _.diag(SPV_ERROR_INVALID_DATA, inst) + << _.VkErrorID(4965) + << "Image Format signedness does not match Sample Type operand " + "including possible SignExtend or ZeroExtend operand"; + } + } + + // Format might be Unknown, so check type + if (_.IsFloatScalarType(info.sampled_type) && + (is_sign_extend || is_zero_extend)) { + return _.diag(SPV_ERROR_INVALID_DATA, inst) + << _.VkErrorID(4965) + << "Can't use SignExtend or ZeroExtend on float images"; + } + } + // After this point, only set bits in the image operands mask can cause // the module to be invalid. if (mask == 0) return SPV_SUCCESS; @@ -454,8 +582,7 @@ spv_result_t ValidateImageOperands(ValidationState_t& _, << " components, but given " << offset_size; } - if (!_.options()->before_hlsl_legalization && - spvIsVulkanEnv(_.context()->target_env)) { + if (!_.options()->before_hlsl_legalization && spvIsVulkanEnv(target_env)) { if (opcode != spv::Op::OpImageGather && opcode != spv::Op::OpImageDrefGather && opcode != spv::Op::OpImageSparseGather && @@ -610,7 +737,7 @@ spv_result_t ValidateImageOperands(ValidationState_t& _, if (auto error = ValidateMemoryScope(_, inst, visible_scope)) return error; } - if (mask & uint32_t(spv::ImageOperandsMask::SignExtend)) { + if (is_sign_extend) { // Checked elsewhere: SPIR-V 1.4 version or later. // "The texel value is converted to the target value via sign extension. @@ -623,7 +750,7 @@ spv_result_t ValidateImageOperands(ValidationState_t& _, // setup. } - if (mask & uint32_t(spv::ImageOperandsMask::ZeroExtend)) { + if (is_zero_extend) { // Checked elsewhere: SPIR-V 1.4 version or later. // "The texel value is converted to the target value via zero extension. @@ -916,6 +1043,32 @@ spv_result_t ValidateTypeImage(ValidationState_t& _, const Instruction* inst) { return _.diag(SPV_ERROR_INVALID_DATA, inst) << _.VkErrorID(6214) << "Dim SubpassData requires Arrayed to be 0"; } + + // Can't check signedness here due to image operands able to override + // sampled type + if (info.format != spv::ImageFormat::Unknown) { + // validated above so can assume this is a 32-bit float, 32-bit int, or + // 64-bit int + const bool is_int = _.IsIntScalarType(info.sampled_type); + const bool is_float = !is_int; + if ((is_float && !IsImageFormatFloat(info.format)) || + (is_int && !IsImageFormatInt(info.format))) { + return _.diag(SPV_ERROR_INVALID_DATA, inst) + << _.VkErrorID(4965) + << "Image Format type (float or int) does not match Sample Type " + "operand"; + } + if (is_int) { + const uint32_t bit_width = _.GetBitWidth(info.sampled_type); + if ((bit_width == 32 && !IsImageFormatInt32Width(info.format)) || + (bit_width == 64 && !IsImageFormatInt64Width(info.format))) { + return _.diag(SPV_ERROR_INVALID_DATA, inst) + << _.VkErrorID(4965) + << "Image Format width (32 or 64) does not match Sample Type " + "operand"; + } + } + } } return SPV_SUCCESS; diff --git a/source/val/validation_state.cpp b/source/val/validation_state.cpp index 6685985b6e..287781d998 100644 --- a/source/val/validation_state.cpp +++ b/source/val/validation_state.cpp @@ -2225,6 +2225,8 @@ std::string ValidationState_t::VkErrorID(uint32_t id, return VUID_WRAP(VUID-StandaloneSpirv-Component-04923); case 4924: return VUID_WRAP(VUID-StandaloneSpirv-Component-04924); + case 4965: + return VUID_WRAP(VUID-StandaloneSpirv-Image-04965); case 6201: return VUID_WRAP(VUID-StandaloneSpirv-Flat-06201); case 6202: diff --git a/test/val/val_image_test.cpp b/test/val/val_image_test.cpp index 9a704098de..3506b0665f 100644 --- a/test/val/val_image_test.cpp +++ b/test/val/val_image_test.cpp @@ -82,7 +82,7 @@ OpCapability ImageBuffer %uniform_sampler %private_image_u32_buffer_0002_r32ui %private_image_u32_spd_0002 -%private_image_f32_buffer_0002_r32ui +%private_image_f32_buffer_0002_r32f %input_flat_u32 )"; @@ -311,10 +311,10 @@ OpDecorate %input_flat_u32 Location 0 %ptr_image_u32_spd_0002 = OpTypePointer Private %type_image_u32_spd_0002 %private_image_u32_spd_0002 = OpVariable %ptr_image_u32_spd_0002 Private -%type_image_f32_buffer_0002_r32ui = OpTypeImage %f32 Buffer 0 0 0 2 R32ui +%type_image_f32_buffer_0002_r32f = OpTypeImage %f32 Buffer 0 0 0 2 R32f %ptr_Image_f32 = OpTypePointer Image %f32 -%ptr_image_f32_buffer_0002_r32ui = OpTypePointer Private %type_image_f32_buffer_0002_r32ui -%private_image_f32_buffer_0002_r32ui = OpVariable %ptr_image_f32_buffer_0002_r32ui Private +%ptr_image_f32_buffer_0002_r32f = OpTypePointer Private %type_image_f32_buffer_0002_r32f +%private_image_f32_buffer_0002_r32f = OpVariable %ptr_image_f32_buffer_0002_r32f Private %ptr_input_flat_u32 = OpTypePointer Input %u32 %input_flat_u32 = OpVariable %ptr_input_flat_u32 Input @@ -1397,7 +1397,7 @@ TEST_F(ValidateImage, ImageTexelPointerResultTypeNotNumericNorVoid) { TEST_F(ValidateImage, ImageTexelPointerImageNotResultTypePointer) { const std::string body = R"( -%texel_ptr = OpImageTexelPointer %ptr_Image_u32 %type_image_f32_buffer_0002_r32ui %u32_0 %u32_0 +%texel_ptr = OpImageTexelPointer %ptr_Image_u32 %type_image_f32_buffer_0002_r32f %u32_0 %u32_0 %sum = OpAtomicIAdd %u32 %texel_ptr %u32_1 %u32_0 %u32_1 )"; @@ -1450,7 +1450,7 @@ TEST_F(ValidateImage, ImageTexelPointerImageDimSubpassDataBad) { TEST_F(ValidateImage, ImageTexelPointerImageCoordTypeBad) { const std::string body = R"( -%texel_ptr = OpImageTexelPointer %ptr_Image_f32 %private_image_f32_buffer_0002_r32ui %f32_0 %f32_0 +%texel_ptr = OpImageTexelPointer %ptr_Image_f32 %private_image_f32_buffer_0002_r32f %f32_0 %f32_0 %sum = OpAtomicIAdd %f32 %texel_ptr %f32_1 %f32_0 %f32_1 )"; @@ -6390,20 +6390,13 @@ TEST_F(ValidateImage, ImageTexelPointerR64iSuccessVulkan) { } TEST_F(ValidateImage, ImageTexelPointerR32fSuccessVulkan) { - const std::string& declarations = R"( -%type_image_f32_buffer_0002_r32f = OpTypeImage %f32 Buffer 0 0 0 2 R32f -%ptr_image_f32_buffer_0002_r32f = OpTypePointer Private %type_image_f32_buffer_0002_r32f -%private_image_f32_buffer_0002_r32f = OpVariable %ptr_image_f32_buffer_0002_r32f Private -)"; - const std::string body = R"( %texel_ptr = OpImageTexelPointer %ptr_Image_f32 %private_image_f32_buffer_0002_r32f %u32_0 %u32_0 )"; spv_target_env env = SPV_ENV_VULKAN_1_0; CompileSuccessfully( - GenerateShaderCode(body, "", "Fragment", "", env, "GLSL450", declarations) - .c_str(), + GenerateShaderCode(body, "", "Fragment", "", env, "GLSL450").c_str(), env); ASSERT_EQ(SPV_SUCCESS, ValidateInstructions(env)); } @@ -6435,14 +6428,13 @@ TEST_F(ValidateImage, ImageTexelPointerRgba32iVulkan) { TEST_F(ValidateImage, ImageTexelPointerRgba16fVulkan) { const std::string& declarations = R"( -%type_image_s32_buffer_0002_rgba16f = OpTypeImage %s32 Buffer 0 0 0 2 Rgba16f -%ptr_Image_s32 = OpTypePointer Image %s32 -%ptr_image_s32_buffer_0002_rgba16f = OpTypePointer Private %type_image_s32_buffer_0002_rgba16f -%private_image_s32_buffer_0002_rgba16f = OpVariable %ptr_image_s32_buffer_0002_rgba16f Private +%type_image_f32_buffer_0002_rgba16f = OpTypeImage %f32 Buffer 0 0 0 2 Rgba16f +%ptr_image_f32_buffer_0002_rgba16f = OpTypePointer Private %type_image_f32_buffer_0002_rgba16f +%private_image_f32_buffer_0002_rgba16f = OpVariable %ptr_image_f32_buffer_0002_rgba16f Private )"; const std::string body = R"( -%texel_ptr = OpImageTexelPointer %ptr_Image_s32 %private_image_s32_buffer_0002_rgba16f %u32_0 %u32_0 +%texel_ptr = OpImageTexelPointer %ptr_Image_f32 %private_image_f32_buffer_0002_rgba16f %u32_0 %u32_0 )"; spv_target_env env = SPV_ENV_VULKAN_1_0; @@ -8046,6 +8038,302 @@ TEST_F(ValidateImage, ImageMSArray_ArrayedTypeDoesNotRequireCapability) { EXPECT_THAT(getDiagnosticString(), Eq("")); } +TEST_F(ValidateImage, TypeImageVulkanStorageNotFloat) { + const std::string code = GetShaderHeader() + R"( +%img_type = OpTypeImage %f32 2D 0 0 0 2 R32i +)" + TrivialMain(); + + CompileSuccessfully(code.c_str()); + ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(SPV_ENV_VULKAN_1_0)); + EXPECT_THAT(getDiagnosticString(), + AnyVUID("VUID-StandaloneSpirv-Image-04965")); + EXPECT_THAT(getDiagnosticString(), + HasSubstr("Image Format type (float or int) does not match " + "Sample Type operand")); +} + +TEST_F(ValidateImage, TypeImageVulkanStorageNotInt) { + const std::string code = GetShaderHeader() + R"( +%img_type = OpTypeImage %s32 2D 0 0 0 2 R32f +)" + TrivialMain(); + + CompileSuccessfully(code.c_str()); + ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(SPV_ENV_VULKAN_1_0)); + EXPECT_THAT(getDiagnosticString(), + AnyVUID("VUID-StandaloneSpirv-Image-04965")); + EXPECT_THAT(getDiagnosticString(), + HasSubstr("Image Format type (float or int) does not match " + "Sample Type operand")); +} + +TEST_F(ValidateImage, TypeImageVulkanStorageNot64Width) { + const std::string code = GetShaderHeader( + "OpCapability Int64ImageEXT\nOpExtension " + "\"SPV_EXT_shader_image_int64\"\n") + + R"( +%img_type = OpTypeImage %s64 2D 0 0 0 2 R32i +)" + TrivialMain(); + + CompileSuccessfully(code.c_str()); + ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(SPV_ENV_VULKAN_1_0)); + EXPECT_THAT(getDiagnosticString(), + AnyVUID("VUID-StandaloneSpirv-Image-04965")); + EXPECT_THAT( + getDiagnosticString(), + HasSubstr( + "Image Format width (32 or 64) does not match Sample Type operand")); +} + +TEST_F(ValidateImage, TypeImageVulkanStorageNot32Width) { + const std::string code = GetShaderHeader( + "OpCapability Int64ImageEXT\nOpExtension " + "\"SPV_EXT_shader_image_int64\"\n") + + R"( +%img_type = OpTypeImage %s32 2D 0 0 0 2 R64i +)" + TrivialMain(); + + CompileSuccessfully(code.c_str()); + ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(SPV_ENV_VULKAN_1_0)); + EXPECT_THAT(getDiagnosticString(), + AnyVUID("VUID-StandaloneSpirv-Image-04965")); + EXPECT_THAT( + getDiagnosticString(), + HasSubstr( + "Image Format width (32 or 64) does not match Sample Type operand")); +} + +TEST_F(ValidateImage, TypeImageVulkanStorageNot64Signedness) { + const std::string code = R"( + OpCapability Shader + OpCapability Int64 + OpCapability Int64ImageEXT + OpExtension "SPV_EXT_shader_image_int64" + OpMemoryModel Logical GLSL450 + OpEntryPoint Fragment %main "main" %image_var + OpExecutionMode %main OriginUpperLeft + OpDecorate %image_var Location 0 + %void = OpTypeVoid + %func = OpTypeFunction %void + %u32 = OpTypeInt 32 0 + %s64 = OpTypeInt 64 1 + %u32_0 = OpConstant %u32 0 + %u32_1 = OpConstant %u32 1 + %u32vec2 = OpTypeVector %u32 2 + %s64vec4 = OpTypeVector %s64 4 +%u32vec2_01 = OpConstantComposite %u32vec2 %u32_0 %u32_1 + %img_type = OpTypeImage %s64 2D 0 0 0 1 R64ui +%ptr_image = OpTypePointer Input %img_type +%image_var = OpVariable %ptr_image Input + %main = OpFunction %void None %func + %label = OpLabel + %img = OpLoad %img_type %image_var + %res1 = OpImageFetch %s64vec4 %img %u32vec2_01 + OpReturn + OpFunctionEnd +)"; + + CompileSuccessfully(code.c_str(), SPV_ENV_VULKAN_1_0); + ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(SPV_ENV_VULKAN_1_0)); + EXPECT_THAT(getDiagnosticString(), + AnyVUID("VUID-StandaloneSpirv-Image-04965")); + EXPECT_THAT( + getDiagnosticString(), + HasSubstr("Image Format signedness does not match Sample Type operand " + "including possible SignExtend or ZeroExtend operand")); +} + +TEST_F(ValidateImage, TypeImageVulkanStorageNot32Signedness) { + const std::string code = R"( + OpCapability Shader + OpMemoryModel Logical GLSL450 + OpEntryPoint Fragment %main "main" %image_var + OpExecutionMode %main OriginUpperLeft + OpDecorate %image_var Location 0 + %void = OpTypeVoid + %func = OpTypeFunction %void + %u32 = OpTypeInt 32 0 + %u32_0 = OpConstant %u32 0 + %u32_1 = OpConstant %u32 1 + %u32vec2 = OpTypeVector %u32 2 + %u32vec4 = OpTypeVector %u32 4 +%u32vec2_01 = OpConstantComposite %u32vec2 %u32_0 %u32_1 + %img_type = OpTypeImage %u32 2D 0 0 0 1 R32i +%ptr_image = OpTypePointer Input %img_type +%image_var = OpVariable %ptr_image Input + %main = OpFunction %void None %func + %label = OpLabel + %img = OpLoad %img_type %image_var + %res1 = OpImageFetch %u32vec4 %img %u32vec2_01 + OpReturn + OpFunctionEnd +)"; + + CompileSuccessfully(code.c_str()); + ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(SPV_ENV_VULKAN_1_0)); + EXPECT_THAT(getDiagnosticString(), + AnyVUID("VUID-StandaloneSpirv-Image-04965")); + EXPECT_THAT( + getDiagnosticString(), + HasSubstr("Image Format signedness does not match Sample Type operand " + "including possible SignExtend or ZeroExtend operand")); +} + +TEST_F(ValidateImage, TypeImageVulkanStorageSignExtendOverride) { + const std::string code = R"( + OpCapability Shader + OpMemoryModel Logical GLSL450 + OpEntryPoint Fragment %main "main" %uniform_image + OpExecutionMode %main OriginUpperLeft + OpDecorate %uniform_image DescriptorSet 0 + OpDecorate %uniform_image Binding 0 + %void = OpTypeVoid + %func = OpTypeFunction %void + %u32 = OpTypeInt 32 0 + %u32_0 = OpConstant %u32 0 + %u32_1 = OpConstant %u32 1 + %u32vec2 = OpTypeVector %u32 2 + %u32vec4 = OpTypeVector %u32 4 +%u32vec2_01 = OpConstantComposite %u32vec2 %u32_0 %u32_1 + %img_type = OpTypeImage %u32 2D 0 0 0 1 R32i +%ptr_image = OpTypePointer UniformConstant %img_type +%uniform_image = OpVariable %ptr_image UniformConstant + %main = OpFunction %void None %func + %label = OpLabel + %img = OpLoad %img_type %uniform_image + %res1 = OpImageFetch %u32vec4 %img %u32vec2_01 SignExtend + OpReturn + OpFunctionEnd +)"; + + CompileSuccessfully(code.c_str(), SPV_ENV_VULKAN_1_2); + ASSERT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_VULKAN_1_2)); +} + +TEST_F(ValidateImage, TypeImageVulkanStorageZeroExtendOverride) { + const std::string code = R"( + OpCapability Shader + OpMemoryModel Logical GLSL450 + OpEntryPoint Fragment %main "main" %uniform_image + OpExecutionMode %main OriginUpperLeft + OpDecorate %uniform_image DescriptorSet 0 + OpDecorate %uniform_image Binding 0 + %void = OpTypeVoid + %func = OpTypeFunction %void + %i32 = OpTypeInt 32 1 + %u32 = OpTypeInt 32 0 + %u32_0 = OpConstant %u32 0 + %u32_1 = OpConstant %u32 1 + %u32vec2 = OpTypeVector %u32 2 + %i32vec4 = OpTypeVector %i32 4 +%u32vec2_01 = OpConstantComposite %u32vec2 %u32_0 %u32_1 + %img_type = OpTypeImage %i32 2D 0 0 0 2 R32ui +%ptr_image = OpTypePointer UniformConstant %img_type +%uniform_image = OpVariable %ptr_image UniformConstant + %main = OpFunction %void None %func + %label = OpLabel + %img = OpLoad %img_type %uniform_image + %res1 = OpImageRead %i32vec4 %img %u32vec2_01 ZeroExtend + OpReturn + OpFunctionEnd +)"; + + CompileSuccessfully(code.c_str(), SPV_ENV_VULKAN_1_2); + ASSERT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_VULKAN_1_2)); +} + +TEST_F(ValidateImage, TypeImageVulkanStorageZeroExtendRedundant) { + // use ZeroExtend when sample type is already unsigned but still has signed + // format + const std::string code = R"( + OpCapability Shader + OpMemoryModel Logical GLSL450 + OpEntryPoint Fragment %main "main" %uniform_image + OpExecutionMode %main OriginUpperLeft + OpDecorate %uniform_image DescriptorSet 0 + OpDecorate %uniform_image Binding 0 + %void = OpTypeVoid + %func = OpTypeFunction %void + %u32 = OpTypeInt 32 0 + %u32_0 = OpConstant %u32 0 + %u32_1 = OpConstant %u32 1 + %u32vec2 = OpTypeVector %u32 2 + %u32vec4 = OpTypeVector %u32 4 +%u32vec2_01 = OpConstantComposite %u32vec2 %u32_0 %u32_1 + %img_type = OpTypeImage %u32 2D 0 0 0 1 R32i +%ptr_image = OpTypePointer UniformConstant %img_type +%uniform_image = OpVariable %ptr_image UniformConstant + %main = OpFunction %void None %func + %label = OpLabel + %img = OpLoad %img_type %uniform_image + %res1 = OpImageFetch %u32vec4 %img %u32vec2_01 ZeroExtend + OpReturn + OpFunctionEnd +)"; + + CompileSuccessfully(code.c_str(), SPV_ENV_VULKAN_1_2); + ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(SPV_ENV_VULKAN_1_2)); + EXPECT_THAT(getDiagnosticString(), + AnyVUID("VUID-StandaloneSpirv-Image-04965")); + EXPECT_THAT( + getDiagnosticString(), + HasSubstr("Image Format signedness does not match Sample Type operand " + "including possible SignExtend or ZeroExtend operand")); +} + +TEST_F(ValidateImage, TypeImageVulkanStorageZeroExtendFloat) { + // use ZeroExtend on Float image + const std::string code = R"( + OpCapability Shader + OpCapability StorageImageWriteWithoutFormat + OpMemoryModel Logical GLSL450 + OpEntryPoint GLCompute %main "main" %var + OpExecutionMode %main LocalSize 1 1 1 + OpDecorate %var DescriptorSet 0 + OpDecorate %var Binding 0 + OpDecorate %var NonReadable + %void = OpTypeVoid + %func = OpTypeFunction %void + %int = OpTypeInt 32 1 + %float = OpTypeFloat 32 + %image = OpTypeImage %float 2D 0 0 0 2 Unknown + %ptr = OpTypePointer UniformConstant %image + %var = OpVariable %ptr UniformConstant + %v2int = OpTypeVector %int 2 + %int_1 = OpConstant %int 1 + %coord = OpConstantComposite %v2int %int_1 %int_1 + %v3float = OpTypeVector %float 3 + %float_1 = OpConstant %float 1 +%texelU3 = OpConstantComposite %v3float %float_1 %float_1 %float_1 + %main = OpFunction %void None %func + %label = OpLabel + %load = OpLoad %image %var + OpImageWrite %load %coord %texelU3 ZeroExtend + OpReturn + OpFunctionEnd +)"; + + CompileSuccessfully(code.c_str(), SPV_ENV_VULKAN_1_2); + ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(SPV_ENV_VULKAN_1_2)); + EXPECT_THAT(getDiagnosticString(), + AnyVUID("VUID-StandaloneSpirv-Image-04965")); + EXPECT_THAT(getDiagnosticString(), + HasSubstr("Can't use SignExtend or ZeroExtend on float images")); +} + +TEST_F(ValidateImage, BothSignExtendZeroExtendTogether) { + const std::string body = R"( +%img = OpLoad %type_image_s32_2d_0002 %uniform_image_s32_2d_0002 +%res1 = OpImageRead %s32vec4 %img %u32vec2_01 SignExtend ZeroExtend +)"; + const std::string extra = "\nOpCapability StorageImageReadWithoutFormat\n"; + + EXPECT_THAT(CompileFailure(GenerateShaderCode(body, extra, "Fragment", "", + SPV_ENV_UNIVERSAL_1_4), + SPV_ENV_UNIVERSAL_1_4), + HasSubstr("Expected or at the beginning of " + "an instruction")); +} + } // namespace } // namespace val } // namespace spvtools From 5b66a419cea5a9ce98146e63a67f4b5f5ce2b48e Mon Sep 17 00:00:00 2001 From: spencer-lunarg Date: Tue, 31 Oct 2023 17:50:12 +0900 Subject: [PATCH 2/4] spirv-val: Add Check for valid SignExtend/ZeroExtend --- source/val/validate_image.cpp | 47 +++++++++++++++++++++-------------- test/val/val_image_test.cpp | 44 ++++++++++++++++---------------- 2 files changed, 49 insertions(+), 42 deletions(-) diff --git a/source/val/validate_image.cpp b/source/val/validate_image.cpp index f25e4b4efa..858e67b171 100644 --- a/source/val/validate_image.cpp +++ b/source/val/validate_image.cpp @@ -178,7 +178,7 @@ bool IsValidGatherLodBiasAMD(const ValidationState_t& _, spv::Op opcode) { return false; } -bool IsImageFormatInt32Width(spv::ImageFormat format) { +bool IsSignedInt32ImageFormat(spv::ImageFormat format) { switch (format) { case spv::ImageFormat::Rgba32i: case spv::ImageFormat::Rgba16i: @@ -206,7 +206,7 @@ bool IsImageFormatInt32Width(spv::ImageFormat format) { return false; } -bool IsImageFormatInt64Width(spv::ImageFormat format) { +bool IsSignedInt64ImageFormat(spv::ImageFormat format) { switch (format) { case spv::ImageFormat::R64ui: case spv::ImageFormat::R64i: @@ -217,7 +217,7 @@ bool IsImageFormatInt64Width(spv::ImageFormat format) { return false; } -bool IsImageFormatIntSigned(spv::ImageFormat format) { +bool IsSignedIntImageFormat(spv::ImageFormat format) { switch (format) { case spv::ImageFormat::Rgba32i: case spv::ImageFormat::Rgba16i: @@ -236,7 +236,7 @@ bool IsImageFormatIntSigned(spv::ImageFormat format) { return false; } -bool IsImageFormatFloat(spv::ImageFormat format) { +bool IsFloatImageFormat(spv::ImageFormat format) { switch (format) { case spv::ImageFormat::Rgba32f: case spv::ImageFormat::Rgba16f: @@ -265,8 +265,8 @@ bool IsImageFormatFloat(spv::ImageFormat format) { return false; } -bool IsImageFormatInt(spv::ImageFormat format) { - return IsImageFormatInt32Width(format) || IsImageFormatInt64Width(format); +bool IsIntImageFormat(spv::ImageFormat format) { + return IsSignedInt32ImageFormat(format) || IsSignedInt64ImageFormat(format); } // Returns true if the opcode is a Image instruction which applies @@ -389,7 +389,7 @@ spv_result_t ValidateImageOperands(ValidationState_t& _, if (spvIsVulkanEnv(target_env)) { if (info.format != spv::ImageFormat::Unknown && _.IsIntScalarType(info.sampled_type)) { - const bool is_format_signed = IsImageFormatIntSigned(info.format); + const bool is_format_signed = IsSignedIntImageFormat(info.format); // (vkspec.html#spirvenv-image-signedness) has order signedness is set by bool is_sampled_type_signed = is_sign_extend @@ -405,14 +405,6 @@ spv_result_t ValidateImageOperands(ValidationState_t& _, "including possible SignExtend or ZeroExtend operand"; } } - - // Format might be Unknown, so check type - if (_.IsFloatScalarType(info.sampled_type) && - (is_sign_extend || is_zero_extend)) { - return _.diag(SPV_ERROR_INVALID_DATA, inst) - << _.VkErrorID(4965) - << "Can't use SignExtend or ZeroExtend on float images"; - } } // After this point, only set bits in the image operands mask can cause @@ -737,6 +729,7 @@ spv_result_t ValidateImageOperands(ValidationState_t& _, if (auto error = ValidateMemoryScope(_, inst, visible_scope)) return error; } + const uint32_t result_type = inst->type_id(); if (is_sign_extend) { // Checked elsewhere: SPIR-V 1.4 version or later. @@ -748,6 +741,12 @@ spv_result_t ValidateImageOperands(ValidationState_t& _, // void, and the Format is Unknown. // In Vulkan, the texel type is only known in all cases by the pipeline // setup. + if (!_.IsIntScalarOrVectorType(result_type)) { + return _.diag(SPV_ERROR_INVALID_DATA, inst) + << _.VkErrorID(4965) + << "Using SignExtend, but result type is not a scalar or vector " + "integer type."; + } } if (is_zero_extend) { @@ -761,6 +760,16 @@ spv_result_t ValidateImageOperands(ValidationState_t& _, // void, and the Format is Unknown. // In Vulkan, the texel type is only known in all cases by the pipeline // setup. + if (!_.IsIntScalarOrVectorType(result_type)) { + return _.diag(SPV_ERROR_INVALID_DATA, inst) + << _.VkErrorID(4965) + << "Using SignExtend, but result type is not a scalar or vector " + "integer type."; + } else if (!_.IsUnsignedIntScalarOrVectorType(result_type)) { + return _.diag(SPV_ERROR_INVALID_DATA, inst) + << _.VkErrorID(4965) + << "Using SignExtend, but result type is a signed integer type."; + } } if (mask & uint32_t(spv::ImageOperandsMask::Offsets)) { @@ -1051,8 +1060,8 @@ spv_result_t ValidateTypeImage(ValidationState_t& _, const Instruction* inst) { // 64-bit int const bool is_int = _.IsIntScalarType(info.sampled_type); const bool is_float = !is_int; - if ((is_float && !IsImageFormatFloat(info.format)) || - (is_int && !IsImageFormatInt(info.format))) { + if ((is_float && !IsFloatImageFormat(info.format)) || + (is_int && !IsIntImageFormat(info.format))) { return _.diag(SPV_ERROR_INVALID_DATA, inst) << _.VkErrorID(4965) << "Image Format type (float or int) does not match Sample Type " @@ -1060,8 +1069,8 @@ spv_result_t ValidateTypeImage(ValidationState_t& _, const Instruction* inst) { } if (is_int) { const uint32_t bit_width = _.GetBitWidth(info.sampled_type); - if ((bit_width == 32 && !IsImageFormatInt32Width(info.format)) || - (bit_width == 64 && !IsImageFormatInt64Width(info.format))) { + if ((bit_width == 32 && !IsSignedInt32ImageFormat(info.format)) || + (bit_width == 64 && !IsSignedInt64ImageFormat(info.format))) { return _.diag(SPV_ERROR_INVALID_DATA, inst) << _.VkErrorID(4965) << "Image Format width (32 or 64) does not match Sample Type " diff --git a/test/val/val_image_test.cpp b/test/val/val_image_test.cpp index 3506b0665f..574e255f5a 100644 --- a/test/val/val_image_test.cpp +++ b/test/val/val_image_test.cpp @@ -5950,7 +5950,7 @@ TEST_F(ValidateImage, ZeroExtendScalarUIntTexelV14Good) { EXPECT_THAT(getDiagnosticString(), Eq("")); } -TEST_F(ValidateImage, ZeroExtendScalarSIntTexelV14Good) { +TEST_F(ValidateImage, ZeroExtendScalarSIntTexelV14) { // Zeroed int sampled type const std::string body = R"( %img = OpLoad %type_image_s32_2d_0002 %uniform_image_s32_2d_0002 @@ -5961,8 +5961,11 @@ TEST_F(ValidateImage, ZeroExtendScalarSIntTexelV14Good) { CompileSuccessfully( GenerateShaderCode(body, extra, "Fragment", "", SPV_ENV_UNIVERSAL_1_4), SPV_ENV_UNIVERSAL_1_4); - EXPECT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_UNIVERSAL_1_4)); - EXPECT_THAT(getDiagnosticString(), Eq("")); + EXPECT_EQ(SPV_ERROR_INVALID_DATA, + ValidateInstructions(SPV_ENV_UNIVERSAL_1_4)); + EXPECT_THAT( + getDiagnosticString(), + HasSubstr("Using SignExtend, but result type is a signed integer type.")); } TEST_F(ValidateImage, ZeroExtendScalarVectorUIntTexelV14Good) { @@ -5979,7 +5982,7 @@ TEST_F(ValidateImage, ZeroExtendScalarVectorUIntTexelV14Good) { EXPECT_THAT(getDiagnosticString(), Eq("")); } -TEST_F(ValidateImage, ZeroExtendVectorSIntTexelV14Good) { +TEST_F(ValidateImage, ZeroExtendVectorSIntTexelV14) { const std::string body = R"( %img = OpLoad %type_image_s32_2d_0002 %uniform_image_s32_2d_0002 %res1 = OpImageRead %s32vec4 %img %u32vec2_01 ZeroExtend @@ -5989,8 +5992,11 @@ TEST_F(ValidateImage, ZeroExtendVectorSIntTexelV14Good) { CompileSuccessfully( GenerateShaderCode(body, extra, "Fragment", "", SPV_ENV_UNIVERSAL_1_4), SPV_ENV_UNIVERSAL_1_4); - EXPECT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_UNIVERSAL_1_4)); - EXPECT_THAT(getDiagnosticString(), Eq("")); + EXPECT_EQ(SPV_ERROR_INVALID_DATA, + ValidateInstructions(SPV_ENV_UNIVERSAL_1_4)); + EXPECT_THAT( + getDiagnosticString(), + HasSubstr("Using SignExtend, but result type is a signed integer type.")); } TEST_F(ValidateImage, ReadLodAMDSuccess1) { @@ -8209,7 +8215,7 @@ TEST_F(ValidateImage, TypeImageVulkanStorageSignExtendOverride) { ASSERT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_VULKAN_1_2)); } -TEST_F(ValidateImage, TypeImageVulkanStorageZeroExtendOverride) { +TEST_F(ValidateImage, TypeImageVulkanStorageZeroExtendSigned) { const std::string code = R"( OpCapability Shader OpMemoryModel Logical GLSL450 @@ -8238,7 +8244,12 @@ TEST_F(ValidateImage, TypeImageVulkanStorageZeroExtendOverride) { )"; CompileSuccessfully(code.c_str(), SPV_ENV_VULKAN_1_2); - ASSERT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_VULKAN_1_2)); + ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(SPV_ENV_VULKAN_1_2)); + EXPECT_THAT(getDiagnosticString(), + AnyVUID("VUID-StandaloneSpirv-Image-04965")); + EXPECT_THAT( + getDiagnosticString(), + HasSubstr("Using SignExtend, but result type is a signed integer type")); } TEST_F(ValidateImage, TypeImageVulkanStorageZeroExtendRedundant) { @@ -8317,21 +8328,8 @@ TEST_F(ValidateImage, TypeImageVulkanStorageZeroExtendFloat) { EXPECT_THAT(getDiagnosticString(), AnyVUID("VUID-StandaloneSpirv-Image-04965")); EXPECT_THAT(getDiagnosticString(), - HasSubstr("Can't use SignExtend or ZeroExtend on float images")); -} - -TEST_F(ValidateImage, BothSignExtendZeroExtendTogether) { - const std::string body = R"( -%img = OpLoad %type_image_s32_2d_0002 %uniform_image_s32_2d_0002 -%res1 = OpImageRead %s32vec4 %img %u32vec2_01 SignExtend ZeroExtend -)"; - const std::string extra = "\nOpCapability StorageImageReadWithoutFormat\n"; - - EXPECT_THAT(CompileFailure(GenerateShaderCode(body, extra, "Fragment", "", - SPV_ENV_UNIVERSAL_1_4), - SPV_ENV_UNIVERSAL_1_4), - HasSubstr("Expected or at the beginning of " - "an instruction")); + HasSubstr("Using SignExtend, but result type is not a scalar or " + "vector integer type.")); } } // namespace From 7895cbffb271bad23fa5ee18fbe2762538dfa8f3 Mon Sep 17 00:00:00 2001 From: spencer-lunarg Date: Wed, 1 Nov 2023 14:58:05 +0900 Subject: [PATCH 3/4] spirv-val: Fix naming for ImageFormat functions --- source/val/validate_image.cpp | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/source/val/validate_image.cpp b/source/val/validate_image.cpp index 858e67b171..bc051e6231 100644 --- a/source/val/validate_image.cpp +++ b/source/val/validate_image.cpp @@ -178,7 +178,8 @@ bool IsValidGatherLodBiasAMD(const ValidationState_t& _, spv::Op opcode) { return false; } -bool IsSignedInt32ImageFormat(spv::ImageFormat format) { +// Signed or Unsigned Integer Format +bool IsIntImageFormat(spv::ImageFormat format) { switch (format) { case spv::ImageFormat::Rgba32i: case spv::ImageFormat::Rgba16i: @@ -199,6 +200,8 @@ bool IsSignedInt32ImageFormat(spv::ImageFormat format) { case spv::ImageFormat::Rg8ui: case spv::ImageFormat::R16ui: case spv::ImageFormat::R8ui: + case spv::ImageFormat::R64ui: + case spv::ImageFormat::R64i: return true; default: break; @@ -206,7 +209,7 @@ bool IsSignedInt32ImageFormat(spv::ImageFormat format) { return false; } -bool IsSignedInt64ImageFormat(spv::ImageFormat format) { +bool IsInt64ImageFormat(spv::ImageFormat format) { switch (format) { case spv::ImageFormat::R64ui: case spv::ImageFormat::R64i: @@ -265,10 +268,6 @@ bool IsFloatImageFormat(spv::ImageFormat format) { return false; } -bool IsIntImageFormat(spv::ImageFormat format) { - return IsSignedInt32ImageFormat(format) || IsSignedInt64ImageFormat(format); -} - // Returns true if the opcode is a Image instruction which applies // homogenous projection to the coordinates. bool IsProj(spv::Op opcode) { @@ -1066,11 +1065,11 @@ spv_result_t ValidateTypeImage(ValidationState_t& _, const Instruction* inst) { << _.VkErrorID(4965) << "Image Format type (float or int) does not match Sample Type " "operand"; - } - if (is_int) { + } else if (is_int) { const uint32_t bit_width = _.GetBitWidth(info.sampled_type); - if ((bit_width == 32 && !IsSignedInt32ImageFormat(info.format)) || - (bit_width == 64 && !IsSignedInt64ImageFormat(info.format))) { + // format check above to be int + if ((bit_width == 32 && IsInt64ImageFormat(info.format)) || + (bit_width == 64 && !IsInt64ImageFormat(info.format))) { return _.diag(SPV_ERROR_INVALID_DATA, inst) << _.VkErrorID(4965) << "Image Format width (32 or 64) does not match Sample Type " From 03698f1b54e8b385a59b596ed1a84dc3643b5834 Mon Sep 17 00:00:00 2001 From: spencer-lunarg Date: Sun, 5 Nov 2023 01:03:59 +0900 Subject: [PATCH 4/4] spirv-val: Collapse ZeroExtend error message --- source/val/validate_image.cpp | 9 ++------- test/val/val_image_test.cpp | 12 ++++++------ 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/source/val/validate_image.cpp b/source/val/validate_image.cpp index bc051e6231..9d26a66521 100644 --- a/source/val/validate_image.cpp +++ b/source/val/validate_image.cpp @@ -759,15 +759,10 @@ spv_result_t ValidateImageOperands(ValidationState_t& _, // void, and the Format is Unknown. // In Vulkan, the texel type is only known in all cases by the pipeline // setup. - if (!_.IsIntScalarOrVectorType(result_type)) { - return _.diag(SPV_ERROR_INVALID_DATA, inst) - << _.VkErrorID(4965) - << "Using SignExtend, but result type is not a scalar or vector " - "integer type."; - } else if (!_.IsUnsignedIntScalarOrVectorType(result_type)) { + if (!_.IsUnsignedIntScalarOrVectorType(result_type)) { return _.diag(SPV_ERROR_INVALID_DATA, inst) << _.VkErrorID(4965) - << "Using SignExtend, but result type is a signed integer type."; + << "Using ZeroExtend, but result type is a signed integer type."; } } diff --git a/test/val/val_image_test.cpp b/test/val/val_image_test.cpp index 574e255f5a..f9e4a4495d 100644 --- a/test/val/val_image_test.cpp +++ b/test/val/val_image_test.cpp @@ -5965,7 +5965,7 @@ TEST_F(ValidateImage, ZeroExtendScalarSIntTexelV14) { ValidateInstructions(SPV_ENV_UNIVERSAL_1_4)); EXPECT_THAT( getDiagnosticString(), - HasSubstr("Using SignExtend, but result type is a signed integer type.")); + HasSubstr("Using ZeroExtend, but result type is a signed integer type.")); } TEST_F(ValidateImage, ZeroExtendScalarVectorUIntTexelV14Good) { @@ -5996,7 +5996,7 @@ TEST_F(ValidateImage, ZeroExtendVectorSIntTexelV14) { ValidateInstructions(SPV_ENV_UNIVERSAL_1_4)); EXPECT_THAT( getDiagnosticString(), - HasSubstr("Using SignExtend, but result type is a signed integer type.")); + HasSubstr("Using ZeroExtend, but result type is a signed integer type.")); } TEST_F(ValidateImage, ReadLodAMDSuccess1) { @@ -8249,7 +8249,7 @@ TEST_F(ValidateImage, TypeImageVulkanStorageZeroExtendSigned) { AnyVUID("VUID-StandaloneSpirv-Image-04965")); EXPECT_THAT( getDiagnosticString(), - HasSubstr("Using SignExtend, but result type is a signed integer type")); + HasSubstr("Using ZeroExtend, but result type is a signed integer type")); } TEST_F(ValidateImage, TypeImageVulkanStorageZeroExtendRedundant) { @@ -8327,9 +8327,9 @@ TEST_F(ValidateImage, TypeImageVulkanStorageZeroExtendFloat) { ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(SPV_ENV_VULKAN_1_2)); EXPECT_THAT(getDiagnosticString(), AnyVUID("VUID-StandaloneSpirv-Image-04965")); - EXPECT_THAT(getDiagnosticString(), - HasSubstr("Using SignExtend, but result type is not a scalar or " - "vector integer type.")); + EXPECT_THAT( + getDiagnosticString(), + HasSubstr("Using ZeroExtend, but result type is a signed integer type.")); } } // namespace