From 5213c400eb6f2720ae4d3bf3bf4bd87a37ae3d51 Mon Sep 17 00:00:00 2001 From: David Neto Date: Wed, 9 Dec 2020 17:09:50 +0000 Subject: [PATCH] spirv-reader: convert signedness of texturing result when needed Fixed: tint:382 Change-Id: I786e1f5d5d122a82ef29c733e514bf3b45651c5d Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/35180 Commit-Queue: David Neto Auto-Submit: David Neto Reviewed-by: dan sinclair --- src/reader/spirv/function.cc | 27 +- src/reader/spirv/parser_impl.cc | 18 +- src/reader/spirv/parser_impl.h | 8 +- src/reader/spirv/parser_impl_handle_test.cc | 561 +++++++++++++++++++- 4 files changed, 579 insertions(+), 35 deletions(-) diff --git a/src/reader/spirv/function.cc b/src/reader/spirv/function.cc index 607dc39cfa..6a0c855a6c 100644 --- a/src/reader/spirv/function.cc +++ b/src/reader/spirv/function.cc @@ -3940,9 +3940,32 @@ bool FunctionEmitter::EmitImageAccess(const spvtools::opt::Instruction& inst) { if (inst.type_id() != 0) { // It returns a value. + ast::Expression* value = call_expr; + + // If necessary, convert the result to the signedness of the instruction + // result type. Compare the SPIR-V image's sampled component type with the + // component of the result type of the SPIR-V instruction. auto* result_type = parser_impl_.ConvertType(inst.type_id()); - // TODO(dneto): Convert result signedness if needed. crbug.com/tint/382 - EmitConstDefOrWriteToHoistedVar(inst, {result_type, call_expr}); + auto* result_component_type = result_type; + if (auto* result_vector_type = result_type->As()) { + result_component_type = result_vector_type->type(); + } + auto* spirv_image_type = + parser_impl_.GetSpirvTypeForHandleMemoryObjectDeclaration(*image); + if (!spirv_image_type || (spirv_image_type->opcode() != SpvOpTypeImage)) { + return Fail() << "invalid image type for image memory object declaration " + << image->PrettyPrint(); + } + auto* expected_component_type = + parser_impl_.ConvertType(spirv_image_type->GetSingleWordInOperand(0)); + if (expected_component_type != result_component_type) { + // This occurs if one is signed integer and the other is unsigned integer, + // or vice versa. Perform a bitcast. + value = + ast_module_.create(result_type, call_expr); + } + + EmitConstDefOrWriteToHoistedVar(inst, {result_type, value}); } else { // It's an image write. No value is returned, so make a statement out // of the call. diff --git a/src/reader/spirv/parser_impl.cc b/src/reader/spirv/parser_impl.cc index 3543a77be6..71e93196c9 100644 --- a/src/reader/spirv/parser_impl.cc +++ b/src/reader/spirv/parser_impl.cc @@ -1683,7 +1683,8 @@ ParserImpl::GetMemoryObjectDeclarationForHandle(uint32_t id, } } -const spvtools::opt::Instruction* ParserImpl::GetSpirvTypeForHandleVar( +const spvtools::opt::Instruction* +ParserImpl::GetSpirvTypeForHandleMemoryObjectDeclaration( const spvtools::opt::Instruction& var) { if (!success()) { return nullptr; @@ -1700,14 +1701,16 @@ const spvtools::opt::Instruction* ParserImpl::GetSpirvTypeForHandleVar( // Get the SPIR-V handle type. const auto* ptr_type = def_use_mgr_->GetDef(var.type_id()); - if (!ptr_type) { - Fail() << "Invalid type for variable " << var.PrettyPrint(); + if (!ptr_type || (ptr_type->opcode() != SpvOpTypePointer)) { + Fail() << "Invalid type for variable or function parameter " + << var.PrettyPrint(); return nullptr; } const auto* raw_handle_type = def_use_mgr_->GetDef(ptr_type->GetSingleWordInOperand(1)); if (!raw_handle_type) { - Fail() << "Invalid pointer type for variable " << var.PrettyPrint(); + Fail() << "Invalid pointer type for variable or function parameter " + << var.PrettyPrint(); return nullptr; } switch (raw_handle_type->opcode()) { @@ -1719,11 +1722,12 @@ const spvtools::opt::Instruction* ParserImpl::GetSpirvTypeForHandleVar( case SpvOpTypeRuntimeArray: Fail() << "arrays of textures or samplers are not supported in WGSL; can't " - "translate variable " + "translate variable or function parameter: " << var.PrettyPrint(); return nullptr; default: - Fail() << "invalid type for image or sampler variable: " + Fail() << "invalid type for image or sampler variable or function " + "parameter: " << var.PrettyPrint(); return nullptr; } @@ -1738,7 +1742,7 @@ ast::type::Pointer* ParserImpl::GetTypeForHandleVar( } const spvtools::opt::Instruction* raw_handle_type = - GetSpirvTypeForHandleVar(var); + GetSpirvTypeForHandleMemoryObjectDeclaration(var); if (!raw_handle_type) { return nullptr; } diff --git a/src/reader/spirv/parser_impl.h b/src/reader/spirv/parser_impl.h index 4d3b22a497..2e9e9d720e 100644 --- a/src/reader/spirv/parser_impl.h +++ b/src/reader/spirv/parser_impl.h @@ -431,11 +431,13 @@ class ParserImpl : Reader { Usage GetHandleUsage(uint32_t id) const; /// Returns the SPIR-V type for the sampler or image type for the given - /// variable in UniformConstant storage class. Returns null and emits an + /// variable in UniformConstant storage class, or function parameter pointing + /// into the UniformConstant storage class . Returns null and emits an /// error on failure. - /// @param var the OpVariable instruction + /// @param var the OpVariable instruction or OpFunctionParameter /// @returns the Tint AST type for the sampler or texture, or null on error - const spvtools::opt::Instruction* GetSpirvTypeForHandleVar( + const spvtools::opt::Instruction* + GetSpirvTypeForHandleMemoryObjectDeclaration( const spvtools::opt::Instruction& var); /// Returns the AST type for the pointer-to-sampler or pointer-to-texture type diff --git a/src/reader/spirv/parser_impl_handle_test.cc b/src/reader/spirv/parser_impl_handle_test.cc index cb40386efa..81726fe99c 100644 --- a/src/reader/spirv/parser_impl_handle_test.cc +++ b/src/reader/spirv/parser_impl_handle_test.cc @@ -1229,6 +1229,14 @@ TEST_P(SpvParserTest_SampledImageAccessTest, Variable) { OpName %vf12 "vf12" OpName %vf123 "vf123" OpName %vf1234 "vf1234" + OpName %u1 "u1" + OpName %vu12 "vu12" + OpName %vu123 "vu123" + OpName %vu1234 "vu1234" + OpName %i1 "i1" + OpName %vi12 "vi12" + OpName %vi123 "vi123" + OpName %vi1234 "vi1234" OpName %coords1 "coords1" OpName %coords12 "coords12" OpName %coords123 "coords123" @@ -1261,6 +1269,16 @@ TEST_P(SpvParserTest_SampledImageAccessTest, Variable) { %vf123 = OpCopyObject %v3float %the_vf123 %vf1234 = OpCopyObject %v4float %the_vf1234 + %i1 = OpCopyObject %int %int_1 + %vi12 = OpCopyObject %v2int %the_vi12 + %vi123 = OpCopyObject %v3int %the_vi123 + %vi1234 = OpCopyObject %v4int %the_vi1234 + + %u1 = OpCopyObject %uint %uint_1 + %vu12 = OpCopyObject %v2uint %the_vu12 + %vu123 = OpCopyObject %v3uint %the_vu123 + %vu1234 = OpCopyObject %v4uint %the_vu1234 + %coords1 = OpCopyObject %float %float_1 %coords12 = OpCopyObject %v2float %vf12 %coords123 = OpCopyObject %v3float %vf123 @@ -2889,7 +2907,7 @@ INSTANTIATE_TEST_SUITE_P( SpvParserTest_ImageAccessTest, ::testing::ValuesIn(std::vector{ // OpImageFetch with no extra params - {"%float 2D 0 0 0 1 Rgba32f", "%99 = OpImageFetch %v4float %im %vu12", + {"%float 2D 0 0 0 1 Unknown", "%99 = OpImageFetch %v4float %im %vu12", R"(DecoratedVariable{ Decorations{ SetDecoration{2} @@ -2916,7 +2934,7 @@ INSTANTIATE_TEST_SUITE_P( } })"}, // OpImageFetch with ConstOffset - {"%float 2D 0 0 0 1 Rgba32f", + {"%float 2D 0 0 0 1 Unknown", "%99 = OpImageFetch %v4float %im %vu12 ConstOffset %offsets2d", R"(DecoratedVariable{ Decorations{ @@ -2945,6 +2963,502 @@ INSTANTIATE_TEST_SUITE_P( } })"}})); +INSTANTIATE_TEST_SUITE_P( + ConvertResultSignedness, + SpvParserTest_SampledImageAccessTest, + ::testing::ValuesIn(std::vector{ + // Valid SPIR-V only has: + // float scalar sampled type vs. floating result + // integral scalar sampled type vs. integral result + // Any of the sampling, reading, or fetching use the same codepath. + + // We'll test with: + // OpImageFetch + // OpImageRead + // OpImageSampleImplicitLod - representative of sampling + + // + // OpImageRead + // + + // OpImageFetch requires no conversion, float -> v4float + {"%float 2D 0 0 0 1 Unknown", "%99 = OpImageFetch %v4float %im %vu12", + R"(DecoratedVariable{ + Decorations{ + SetDecoration{2} + BindingDecoration{1} + } + x_20 + uniform_constant + __sampled_texture_2d__f32 + })", + R"(VariableDeclStatement{ + VariableConst{ + x_99 + none + __vec_4__f32 + { + Call[not set]{ + Identifier[not set]{textureLoad} + ( + Identifier[not set]{x_20} + Identifier[not set]{vu12} + ) + } + } + } + })"}, + // OpImageFetch requires no conversion, uint -> v4uint + {"%uint 2D 0 0 0 1 Unknown", "%99 = OpImageFetch %v4uint %im %vu12", + R"(DecoratedVariable{ + Decorations{ + SetDecoration{2} + BindingDecoration{1} + } + x_20 + uniform_constant + __sampled_texture_2d__u32 + })", + R"(VariableDeclStatement{ + VariableConst{ + x_99 + none + __vec_4__u32 + { + Call[not set]{ + Identifier[not set]{textureLoad} + ( + Identifier[not set]{x_20} + Identifier[not set]{vu12} + ) + } + } + } + })"}, + // OpImageFetch requires conversion, uint -> v4int + {"%uint 2D 0 0 0 1 Unknown", "%99 = OpImageFetch %v4int %im %vu12", + R"(DecoratedVariable{ + Decorations{ + SetDecoration{2} + BindingDecoration{1} + } + x_20 + uniform_constant + __sampled_texture_2d__u32 + })", + R"(VariableDeclStatement{ + VariableConst{ + x_99 + none + __vec_4__i32 + { + Bitcast[not set]<__vec_4__i32>{ + Call[not set]{ + Identifier[not set]{textureLoad} + ( + Identifier[not set]{x_20} + Identifier[not set]{vu12} + ) + } + } + } + } + })"}, + // OpImageFetch requires no conversion, int -> v4int + {"%int 2D 0 0 0 1 Unknown", "%99 = OpImageFetch %v4int %im %vu12", + R"(DecoratedVariable{ + Decorations{ + SetDecoration{2} + BindingDecoration{1} + } + x_20 + uniform_constant + __sampled_texture_2d__i32 + })", + R"(VariableDeclStatement{ + VariableConst{ + x_99 + none + __vec_4__i32 + { + Call[not set]{ + Identifier[not set]{textureLoad} + ( + Identifier[not set]{x_20} + Identifier[not set]{vu12} + ) + } + } + } + })"}, + // OpImageFetch requires conversion, int -> v4uint + {"%int 2D 0 0 0 1 Unknown", "%99 = OpImageFetch %v4uint %im %vu12", + R"(DecoratedVariable{ + Decorations{ + SetDecoration{2} + BindingDecoration{1} + } + x_20 + uniform_constant + __sampled_texture_2d__i32 + })", + R"(VariableDeclStatement{ + VariableConst{ + x_99 + none + __vec_4__u32 + { + Bitcast[not set]<__vec_4__u32>{ + Call[not set]{ + Identifier[not set]{textureLoad} + ( + Identifier[not set]{x_20} + Identifier[not set]{vu12} + ) + } + } + } + } + })"}, + + // + // OpImageRead + // + + // OpImageRead requires no conversion, float -> v4float + {"%float 2D 0 0 0 1 Rgba32f", "%99 = OpImageRead %v4float %im %vu12", + R"(DecoratedVariable{ + Decorations{ + SetDecoration{2} + BindingDecoration{1} + } + x_20 + uniform_constant + __storage_texture_read_only_2d_rgba32float + })", + R"(VariableDeclStatement{ + VariableConst{ + x_99 + none + __vec_4__f32 + { + Call[not set]{ + Identifier[not set]{textureLoad} + ( + Identifier[not set]{x_20} + Identifier[not set]{vu12} + ) + } + } + } + })"}, + // OpImageRead requires no conversion, uint -> v4uint + {"%uint 2D 0 0 0 1 Rgba32ui", "%99 = OpImageRead %v4uint %im %vu12", + R"(DecoratedVariable{ + Decorations{ + SetDecoration{2} + BindingDecoration{1} + } + x_20 + uniform_constant + __storage_texture_read_only_2d_rgba32uint + })", + R"(VariableDeclStatement{ + VariableConst{ + x_99 + none + __vec_4__u32 + { + Call[not set]{ + Identifier[not set]{textureLoad} + ( + Identifier[not set]{x_20} + Identifier[not set]{vu12} + ) + } + } + } + })"}, + // OpImageRead requires conversion, uint -> v4int + {"%uint 2D 0 0 0 1 Rgba32ui", "%99 = OpImageRead %v4int %im %vu12", + R"(DecoratedVariable{ + Decorations{ + SetDecoration{2} + BindingDecoration{1} + } + x_20 + uniform_constant + __storage_texture_read_only_2d_rgba32uint + })", + R"(VariableDeclStatement{ + VariableConst{ + x_99 + none + __vec_4__i32 + { + Bitcast[not set]<__vec_4__i32>{ + Call[not set]{ + Identifier[not set]{textureLoad} + ( + Identifier[not set]{x_20} + Identifier[not set]{vu12} + ) + } + } + } + } + })"}, + // OpImageRead requires no conversion, int -> v4int + {"%int 2D 0 0 0 1 Rgba32i", "%99 = OpImageRead %v4int %im %vu12", + R"(DecoratedVariable{ + Decorations{ + SetDecoration{2} + BindingDecoration{1} + } + x_20 + uniform_constant + __storage_texture_read_only_2d_rgba32sint + })", + R"(VariableDeclStatement{ + VariableConst{ + x_99 + none + __vec_4__i32 + { + Call[not set]{ + Identifier[not set]{textureLoad} + ( + Identifier[not set]{x_20} + Identifier[not set]{vu12} + ) + } + } + } + })"}, + // OpImageRead requires conversion, int -> v4uint + {"%int 2D 0 0 0 1 Rgba32i", "%99 = OpImageRead %v4uint %im %vu12", + R"(DecoratedVariable{ + Decorations{ + SetDecoration{2} + BindingDecoration{1} + } + x_20 + uniform_constant + __storage_texture_read_only_2d_rgba32sint + })", + R"(VariableDeclStatement{ + VariableConst{ + x_99 + none + __vec_4__u32 + { + Bitcast[not set]<__vec_4__u32>{ + Call[not set]{ + Identifier[not set]{textureLoad} + ( + Identifier[not set]{x_20} + Identifier[not set]{vu12} + ) + } + } + } + } + })"}, + + // + // Sampling operations, using OpImageSampleImplicitLod as an example. + // + + // OpImageSampleImplicitLod requires no conversion, float -> v4float + {"%float 2D 0 0 0 1 Unknown", + "%99 = OpImageSampleImplicitLod %v4float %sampled_image %vu12", + R"( + DecoratedVariable{ + Decorations{ + SetDecoration{0} + BindingDecoration{0} + } + x_10 + uniform_constant + __sampler_sampler + } + DecoratedVariable{ + Decorations{ + SetDecoration{2} + BindingDecoration{1} + } + x_20 + uniform_constant + __sampled_texture_2d__f32 + })", + R"(VariableDeclStatement{ + VariableConst{ + x_99 + none + __vec_4__f32 + { + Call[not set]{ + Identifier[not set]{textureSample} + ( + Identifier[not set]{x_20} + Identifier[not set]{x_10} + Identifier[not set]{vu12} + ) + } + } + } + })"}, + // OpImageSampleImplicitLod requires no conversion, uint -> v4uint + {"%uint 2D 0 0 0 1 Unknown", + "%99 = OpImageSampleImplicitLod %v4uint %sampled_image %vu12", + R"( + DecoratedVariable{ + Decorations{ + SetDecoration{0} + BindingDecoration{0} + } + x_10 + uniform_constant + __sampler_sampler + } + DecoratedVariable{ + Decorations{ + SetDecoration{2} + BindingDecoration{1} + } + x_20 + uniform_constant + __sampled_texture_2d__u32 + })", + R"(VariableDeclStatement{ + VariableConst{ + x_99 + none + __vec_4__u32 + { + Call[not set]{ + Identifier[not set]{textureSample} + ( + Identifier[not set]{x_20} + Identifier[not set]{x_10} + Identifier[not set]{vu12} + ) + } + } + } + })"}, + // OpImageSampleImplicitLod requires conversion, uint -> v4int + {"%uint 2D 0 0 0 1 Unknown", + "%99 = OpImageSampleImplicitLod %v4int %sampled_image %vu12", + R"( + DecoratedVariable{ + Decorations{ + SetDecoration{0} + BindingDecoration{0} + } + x_10 + uniform_constant + __sampler_sampler + } + DecoratedVariable{ + Decorations{ + SetDecoration{2} + BindingDecoration{1} + } + x_20 + uniform_constant + __sampled_texture_2d__u32 + })", + R"(VariableDeclStatement{ + VariableConst{ + x_99 + none + __vec_4__i32 + { + Bitcast[not set]<__vec_4__i32>{ + Call[not set]{ + Identifier[not set]{textureSample} + ( + Identifier[not set]{x_20} + Identifier[not set]{x_10} + Identifier[not set]{vu12} + ) + } + } + } + } + })"}, + // OpImageSampleImplicitLod requires no conversion, int -> v4int + {"%int 2D 0 0 0 1 Unknown", + "%99 = OpImageSampleImplicitLod %v4int %sampled_image %vu12", + R"( + DecoratedVariable{ + Decorations{ + SetDecoration{0} + BindingDecoration{0} + } + x_10 + uniform_constant + __sampler_sampler + } + DecoratedVariable{ + Decorations{ + SetDecoration{2} + BindingDecoration{1} + } + x_20 + uniform_constant + __sampled_texture_2d__i32 + })", + R"(VariableDeclStatement{ + VariableConst{ + x_99 + none + __vec_4__i32 + { + Call[not set]{ + Identifier[not set]{textureSample} + ( + Identifier[not set]{x_20} + Identifier[not set]{x_10} + Identifier[not set]{vu12} + ) + } + } + } + })"}, + // OpImageSampleImplicitLod requires conversion, int -> v4uint + {"%int 2D 0 0 0 1 Unknown", + "%99 = OpImageSampleImplicitLod %v4uint %sampled_image %vu12", + R"(DecoratedVariable{ + Decorations{ + SetDecoration{2} + BindingDecoration{1} + } + x_20 + uniform_constant + __sampled_texture_2d__i32 + })", + R"(VariableDeclStatement{ + VariableConst{ + x_99 + none + __vec_4__u32 + { + Bitcast[not set]<__vec_4__u32>{ + Call[not set]{ + Identifier[not set]{textureSample} + ( + Identifier[not set]{x_20} + Identifier[not set]{x_10} + Identifier[not set]{vu12} + ) + } + } + } + } + })"}})); + INSTANTIATE_TEST_SUITE_P( // The SPIR-V result type could be integral but of different signedness // than the sampled texel type. In these cases the result should be @@ -3297,27 +3811,28 @@ INSTANTIATE_TEST_SUITE_P(Good_CubeArray, } )"}}})); -INSTANTIATE_TEST_SUITE_P(BadInstructions, - SpvParserTest_ImageCoordsTest, - ::testing::ValuesIn(std::vector{ - {"%float 1D 0 0 0 1 Unknown", - "OpNop", - "internal error: not an image access " - "instruction: OpNop", - {}}, - {"%float 1D 0 0 0 1 Unknown", - "%50 = OpCopyObject %float %float_1", - "internal error: couldn't find image for " - "%50 = OpCopyObject %9 %36", - {}}, - {"%float 1D 0 0 0 1 Unknown", - "OpStore %float_var %float_1", - "invalid type for image or sampler " - "variable: %1 = OpVariable %2 Function", - {}}, - // An example with a missing coordinate - // won't assemble, so we skip it. - })); +INSTANTIATE_TEST_SUITE_P( + BadInstructions, + SpvParserTest_ImageCoordsTest, + ::testing::ValuesIn(std::vector{ + {"%float 1D 0 0 0 1 Unknown", + "OpNop", + "internal error: not an image access " + "instruction: OpNop", + {}}, + {"%float 1D 0 0 0 1 Unknown", + "%50 = OpCopyObject %float %float_1", + "internal error: couldn't find image for " + "%50 = OpCopyObject %9 %36", + {}}, + {"%float 1D 0 0 0 1 Unknown", + "OpStore %float_var %float_1", + "invalid type for image or sampler " + "variable or function parameter: %1 = OpVariable %2 Function", + {}}, + // An example with a missing coordinate + // won't assemble, so we skip it. + })); INSTANTIATE_TEST_SUITE_P( Bad_Coordinate,