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 <dneto@google.com>
Auto-Submit: David Neto <dneto@google.com>
Reviewed-by: dan sinclair <dsinclair@chromium.org>

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
+    auto* result_component_type = result_type;
-    EmitConstDefOrWriteToHoistedVar(inst, {result_type, call_expr});
+    if (auto* result_vector_type = result_type->As<ast::type::Vector>()) {
+      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<ast::BitcastExpression>(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.

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) {
+  if (!ptr_type || (ptr_type->opcode() != SpvOpTypePointer)) {
-    Fail() << "Invalid type for variable " << var.PrettyPrint();
+    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;
   }

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

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<ImageAccessCase>{
         // 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<ImageAccessCase>{
+        // 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,
+INSTANTIATE_TEST_SUITE_P(
-                         SpvParserTest_ImageCoordsTest,
+    BadInstructions,
-                         ::testing::ValuesIn(std::vector<ImageCoordsCase>{
+    SpvParserTest_ImageCoordsTest,
-                             {"%float 1D 0 0 0 1 Unknown",
+    ::testing::ValuesIn(std::vector<ImageCoordsCase>{
-                              "OpNop",
+        {"%float 1D 0 0 0 1 Unknown",
-                              "internal error: not an image access "
+         "OpNop",
-                              "instruction: OpNop",
+         "internal error: not an image access "
-                              {}},
+         "instruction: OpNop",
-                             {"%float 1D 0 0 0 1 Unknown",
+         {}},
-                              "%50 = OpCopyObject %float %float_1",
+        {"%float 1D 0 0 0 1 Unknown",
-                              "internal error: couldn't find image for "
+         "%50 = OpCopyObject %float %float_1",
-                              "%50 = OpCopyObject %9 %36",
+         "internal error: couldn't find image for "
-                              {}},
+         "%50 = OpCopyObject %9 %36",
-                             {"%float 1D 0 0 0 1 Unknown",
+         {}},
-                              "OpStore %float_var %float_1",
+        {"%float 1D 0 0 0 1 Unknown",
-                              "invalid type for image or sampler "
+         "OpStore %float_var %float_1",
-                              "variable: %1 = OpVariable %2 Function",
+         "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.
+        // An example with a missing coordinate
-                         }));
+        // won't assemble, so we skip it.
+    }));
 
 INSTANTIATE_TEST_SUITE_P(
     Bad_Coordinate,