From bf184310b23c70b05b213dbe80083ace0930f558 Mon Sep 17 00:00:00 2001 From: Andrey Tuganov Date: Mon, 4 Dec 2017 12:50:47 -0500 Subject: [PATCH] Fix some of the known issues in image validation Applied some of the spec clarifications made in conversation with @johnkslang. --- source/validate_image.cpp | 35 +++++++++++++++------------ test/val/val_image_test.cpp | 47 +++++++++++++++++++++++++++++++------ 2 files changed, 60 insertions(+), 22 deletions(-) diff --git a/source/validate_image.cpp b/source/validate_image.cpp index 414128688..ffd410680 100644 --- a/source/validate_image.cpp +++ b/source/validate_image.cpp @@ -270,12 +270,10 @@ spv_result_t ValidateImageOperands(ValidationState_t& _, } if (mask & SpvImageOperandsLodMask) { - // TODO(atgoo@github.com) Check which opcodes are allowed to use this - // ImageOperand. - if (is_implicit_lod) { + if (!is_explicit_lod && opcode != SpvOpImageFetch) { return _.diag(SPV_ERROR_INVALID_DATA) - << "Image Operand Lod cannot be used with ImplicitLod opcodes: " - << spvOpcodeString(opcode); + << "Image Operand Lod can only be used with ExplicitLod opcodes " + << "and OpImageFetch: " << spvOpcodeString(opcode); }; if (mask & SpvImageOperandsGradMask) { @@ -286,12 +284,18 @@ spv_result_t ValidateImageOperands(ValidationState_t& _, } const uint32_t type_id = _.GetTypeId(inst.words[word_index++]); - // TODO(atgoo@github.com) Check which opcode can work with floats and which - // with ints. The spec is unclear. - if (!_.IsFloatScalarType(type_id) && !_.IsIntScalarType(type_id)) { - return _.diag(SPV_ERROR_INVALID_DATA) - << "Expected Image Operand Lod to be int or float scalar: " - << spvOpcodeString(opcode); + if (is_explicit_lod) { + if (!_.IsFloatScalarType(type_id)) { + return _.diag(SPV_ERROR_INVALID_DATA) + << "Expected Image Operand Lod to be float scalar when used with " + << "ExplicitLod: " << spvOpcodeString(opcode); + } + } else { + if (!_.IsIntScalarType(type_id)) { + return _.diag(SPV_ERROR_INVALID_DATA) + << "Expected Image Operand Lod to be int scalar when used with " + << "OpImageFetch"; + } } if (info.dim != SpvDim1D && info.dim != SpvDim2D && info.dim != SpvDim3D && @@ -1009,9 +1013,10 @@ spv_result_t ImagePass(ValidationState_t& _, if (opcode == SpvOpImageGather) { const uint32_t component_index_type = _.GetOperandTypeId(inst, 4); - if (!_.IsIntScalarType(component_index_type)) { + if (!_.IsIntScalarType(component_index_type) || + _.GetBitWidth(component_index_type) != 32) { return _.diag(SPV_ERROR_INVALID_DATA) - << "Expected Component to be int scalar: " + << "Expected Component to be 32-bit int scalar: " << spvOpcodeString(opcode); } } else { @@ -1302,9 +1307,9 @@ spv_result_t ImagePass(ValidationState_t& _, } const uint32_t lod_type = _.GetOperandTypeId(inst, 3); - if (!_.IsIntScalarType(lod_type) && !_.IsFloatScalarType(lod_type)) { + if (!_.IsIntScalarType(lod_type)) { return _.diag(SPV_ERROR_INVALID_DATA) - << "Expected Level of Detail to be int or float scalar: " + << "Expected Level of Detail to be int scalar: " << spvOpcodeString(opcode); } diff --git a/test/val/val_image_test.cpp b/test/val/val_image_test.cpp index aafa83b69..282621e37 100644 --- a/test/val/val_image_test.cpp +++ b/test/val/val_image_test.cpp @@ -42,6 +42,7 @@ OpCapability MinLod OpCapability Sampled1D OpCapability SampledRect OpCapability ImageQuery +OpCapability Int64 )"; ss << capabilities_and_extensions; @@ -56,6 +57,7 @@ OpCapability ImageQuery %f32 = OpTypeFloat 32 %u32 = OpTypeInt 32 0 %s32 = OpTypeInt 32 1 +%u64 = OpTypeInt 64 0 %s32vec2 = OpTypeVector %s32 2 %u32vec2 = OpTypeVector %u32 2 %f32vec2 = OpTypeVector %f32 2 @@ -88,6 +90,8 @@ OpCapability ImageQuery %u32_3 = OpConstant %u32 3 %u32_4 = OpConstant %u32 4 +%u64_0 = OpConstant %u64 0 + %u32vec2arr4 = OpTypeArray %u32vec2 %u32_4 %u32vec2arr3 = OpTypeArray %u32vec2 %u32_3 %u32arr4 = OpTypeArray %u32 %u32_4 @@ -805,8 +809,8 @@ TEST_F(ValidateImage, ImplicitLodWithLod) { ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions()); EXPECT_THAT( getDiagnosticString(), - HasSubstr("Image Operand Lod cannot be used with ImplicitLod opcodes: " - "ImageSampleImplicitLod")); + HasSubstr("Image Operand Lod can only be used with ExplicitLod opcodes " + "and OpImageFetch: ImageSampleImplicitLod")); } TEST_F(ValidateImage, LodWrongType) { @@ -819,8 +823,8 @@ TEST_F(ValidateImage, LodWrongType) { CompileSuccessfully(GenerateShaderCode(body).c_str()); ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions()); EXPECT_THAT(getDiagnosticString(), - HasSubstr("Expected Image Operand Lod to be int or float scalar: " - "ImageSampleExplicitLod")); + HasSubstr("Expected Image Operand Lod to be float scalar when " + "used with ExplicitLod: ImageSampleExplicitLod")); } TEST_F(ValidateImage, LodWrongDim) { @@ -2078,6 +2082,19 @@ TEST_F(ValidateImage, FetchCoordinateSizeTooSmall) { "ImageFetch")); } +TEST_F(ValidateImage, FetchLodNotInt) { + const std::string body = R"( +%img = OpLoad %type_image_f32_2d_0001 %uniform_image_f32_2d_0001 +%res1 = OpImageFetch %f32vec4 %img %u32vec2_01 Lod %f32_1 +)"; + + CompileSuccessfully(GenerateShaderCode(body).c_str()); + ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions()); + EXPECT_THAT(getDiagnosticString(), + HasSubstr("Expected Image Operand Lod to be int scalar when used " + "with OpImageFetch")); +} + TEST_F(ValidateImage, GatherSuccess) { const std::string body = R"( %img = OpLoad %type_image_f32_2d_0001 %uniform_image_f32_2d_0001 @@ -2205,7 +2222,23 @@ TEST_F(ValidateImage, GatherWrongComponentType) { CompileSuccessfully(GenerateShaderCode(body).c_str()); ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions()); EXPECT_THAT(getDiagnosticString(), - HasSubstr("Expected Component to be int scalar: ImageGather")); + HasSubstr("Expected Component to be 32-bit int scalar: " + "ImageGather")); +} + +TEST_F(ValidateImage, GatherComponentNot32Bit) { + const std::string body = R"( +%img = OpLoad %type_image_f32_cube_0101 %uniform_image_f32_cube_0101 +%sampler = OpLoad %type_sampler %uniform_sampler +%simg = OpSampledImage %type_sampled_image_f32_cube_0101 %img %sampler +%res1 = OpImageGather %f32vec4 %simg %f32vec4_0000 %u64_0 +)"; + + CompileSuccessfully(GenerateShaderCode(body).c_str()); + ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions()); + EXPECT_THAT(getDiagnosticString(), + HasSubstr("Expected Component to be 32-bit int scalar: " + "ImageGather")); } TEST_F(ValidateImage, GatherDimCube) { @@ -3027,13 +3060,13 @@ TEST_F(ValidateImage, QuerySizeLodMultisampled) { TEST_F(ValidateImage, QuerySizeLodWrongLodType) { const std::string body = R"( %img = OpLoad %type_image_f32_2d_0001 %uniform_image_f32_2d_0001 -%res1 = OpImageQuerySizeLod %u32vec2 %img %u32vec2_01 +%res1 = OpImageQuerySizeLod %u32vec2 %img %f32_0 )"; CompileSuccessfully(GenerateKernelCode(body).c_str()); ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions()); EXPECT_THAT(getDiagnosticString(), - HasSubstr("Expected Level of Detail to be int or float scalar: " + HasSubstr("Expected Level of Detail to be int scalar: " "ImageQuerySizeLod")); }