spirv-reader: Fix signedness for extended instructions

SClamp is the only implemented instruction that is affected, so far

Bug: tint:405
Change-Id: I21c1cdd3e70fc3a64046f0473569ba906048cd37
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/35240
Auto-Submit: David Neto <dneto@google.com>
Commit-Queue: dan sinclair <dsinclair@chromium.org>
Reviewed-by: dan sinclair <dsinclair@chromium.org>

src/reader/spirv/function.cc

@@ -2862,7 +2862,7 @@ TypedExpression FunctionEmitter::MakeOperand(
     const spvtools::opt::Instruction& inst,
     uint32_t operand_index) {
   auto expr = this->MakeExpression(inst.GetSingleWordInOperand(operand_index));
-  return parser_impl_.RectifyOperandSignedness(inst.opcode(), std::move(expr));
+  return parser_impl_.RectifyOperandSignedness(inst, std::move(expr));
 }
 
 TypedExpression FunctionEmitter::MaybeEmitCombinatorialValue(
@@ -2883,7 +2883,7 @@ TypedExpression FunctionEmitter::MaybeEmitCombinatorialValue(
     auto* binary_expr =
         create<ast::BinaryExpression>(binary_op, arg0.expr, arg1.expr);
     TypedExpression result{ast_type, binary_expr};
-    return parser_impl_.RectifyForcedResultType(result, opcode, arg0.type);
+    return parser_impl_.RectifyForcedResultType(result, inst, arg0.type);
   }
 
   auto unary_op = ast::UnaryOp::kNegation;
@@ -2891,7 +2891,7 @@ TypedExpression FunctionEmitter::MaybeEmitCombinatorialValue(
     auto arg0 = MakeOperand(inst, 0);
     auto* unary_expr = create<ast::UnaryOpExpression>(unary_op, arg0.expr);
     TypedExpression result{ast_type, unary_expr};
-    return parser_impl_.RectifyForcedResultType(result, opcode, arg0.type);
+    return parser_impl_.RectifyForcedResultType(result, inst, arg0.type);
   }
 
   const char* unary_builtin_name = GetUnaryBuiltInFunctionName(opcode);
@@ -3002,13 +3002,20 @@ TypedExpression FunctionEmitter::EmitGlslStd450ExtInst(
 
   auto* func = create<ast::IdentifierExpression>(name);
   ast::ExpressionList operands;
+  ast::type::Type* first_operand_type = nullptr;
   // All parameters to GLSL.std.450 extended instructions are IDs.
   for (uint32_t iarg = 2; iarg < inst.NumInOperands(); ++iarg) {
-    operands.emplace_back(MakeOperand(inst, iarg).expr);
+    TypedExpression operand = MakeOperand(inst, iarg);
+    if (first_operand_type == nullptr) {
+      first_operand_type = operand.type;
+    }
+    operands.emplace_back(operand.expr);
   }
   auto* ast_type = parser_impl_.ConvertType(inst.type_id());
   auto* call = create<ast::CallExpression>(func, std::move(operands));
-  return {ast_type, call};
+  TypedExpression call_expr{ast_type, call};
+  return parser_impl_.RectifyForcedResultType(call_expr, inst,
+                                              first_operand_type);
 }
 
 ast::IdentifierExpression* FunctionEmitter::Swizzle(uint32_t i) {

src/reader/spirv/function_glsl_std_450_test.cc

@@ -826,6 +826,66 @@ INSTANTIATE_TEST_SUITE_P(Samples,
                          SpvParserTest_GlslStd450_Uinting_UintingUintingUinting,
                          ::testing::Values(GlslStd450Case{"UClamp", "clamp"}));
 
+TEST_F(SpvParserTest, RectifyOperandsAndResult_GLSLstd450SClamp) {
+  const auto assembly = Preamble() + R"(
+     %1 = OpExtInst %uint %glsl SClamp %u1 %i2 %u3
+     %2 = OpExtInst %v2uint %glsl SClamp %v2u1 %v2i2 %v2u3
+     OpReturn
+     OpFunctionEnd
+  )";
+  auto p = parser(test::Assemble(assembly));
+  ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions());
+  FunctionEmitter fe(p.get(), *spirv_function(p.get(), 100));
+  EXPECT_TRUE(fe.EmitBody()) << p->error();
+  auto body = ToString(fe.ast_body());
+  EXPECT_THAT(body, HasSubstr(R"(
+  VariableConst{
+    x_1
+    none
+    __u32
+    {
+      Bitcast[not set]<__u32>{
+        Call[not set]{
+          Identifier[not set]{clamp}
+          (
+            Bitcast[not set]<__i32>{
+              Identifier[not set]{u1}
+            }
+            Identifier[not set]{i2}
+            Bitcast[not set]<__i32>{
+              Identifier[not set]{u3}
+            }
+          )
+        }
+      }
+    }
+  })"))
+      << body;
+  EXPECT_THAT(body, HasSubstr(R"(
+  VariableConst{
+    x_2
+    none
+    __vec_2__u32
+    {
+      Bitcast[not set]<__vec_2__u32>{
+        Call[not set]{
+          Identifier[not set]{clamp}
+          (
+            Bitcast[not set]<__vec_2__i32>{
+              Identifier[not set]{v2u1}
+            }
+            Identifier[not set]{v2i2}
+            Bitcast[not set]<__vec_2__i32>{
+              Identifier[not set]{v2u3}
+            }
+          )
+        }
+      }
+    }
+  })"))
+      << body;
+}
+
 }  // namespace
 }  // namespace spirv
 }  // namespace reader

src/reader/spirv/parser_impl.cc

@@ -159,6 +159,23 @@ bool AssumesSignedOperands(SpvOp opcode) {
   return false;
 }
 
+// Returns true if the GLSL extended instruction expects operands to be signed.
+// @param extended_opcode GLSL.std.450 opcode
+// @returns true if all operands must be signed integral type
+bool AssumesSignedOperands(GLSLstd450 extended_opcode) {
+  switch (extended_opcode) {
+    case GLSLstd450SAbs:
+    case GLSLstd450SSign:
+    case GLSLstd450SMin:
+    case GLSLstd450SMax:
+    case GLSLstd450SClamp:
+      return true;
+    default:
+      break;
+  }
+  return false;
+}
+
 // Returns true if the opcode operates as if its operands are unsigned integral.
 bool AssumesUnsignedOperands(SpvOp opcode) {
   switch (opcode) {
@@ -176,6 +193,22 @@ bool AssumesUnsignedOperands(SpvOp opcode) {
   return false;
 }
 
+// Returns true if the GLSL extended instruction expects operands to be
+// unsigned.
+// @param extended_opcode GLSL.std.450 opcode
+// @returns true if all operands must be unsigned integral type
+bool AssumesUnsignedOperands(GLSLstd450 extended_opcode) {
+  switch (extended_opcode) {
+    case GLSLstd450UMin:
+    case GLSLstd450UMax:
+    case GLSLstd450UClamp:
+      return true;
+    default:
+      break;
+  }
+  return false;
+}
+
 // Returns true if the operation is binary, and the WGSL operation requires
 // the signedness of the result to match the signedness of the first operand.
 bool AssumesResultSignednessMatchesBinaryFirstOperand(SpvOp opcode) {
@@ -190,6 +223,29 @@ bool AssumesResultSignednessMatchesBinaryFirstOperand(SpvOp opcode) {
   return false;
 }
 
+// Returns true if the extended instruction requires the signedness of the
+// result to match the signedness of the first operand to the operation.
+// @param extended_opcode GLSL.std.450 opcode
+// @returns true if the result type must match the first operand type.
+bool AssumesResultSignednessMatchesFirstOperand(GLSLstd450 extended_opcode) {
+  switch (extended_opcode) {
+    case GLSLstd450SAbs:
+    case GLSLstd450SSign:
+    case GLSLstd450SMin:
+    case GLSLstd450SMax:
+    case GLSLstd450SClamp:
+    case GLSLstd450UMin:
+    case GLSLstd450UMax:
+    case GLSLstd450UClamp:
+      // TODO(dneto): FindSMsb?
+      // TODO(dneto): FindUMsb?
+      return true;
+    default:
+      break;
+  }
+  return false;
+}
+
 }  // namespace
 
 ParserImpl::ParserImpl(const std::vector<uint32_t>& spv_binary)
@@ -561,6 +617,12 @@ bool ParserImpl::RegisterExtendedInstructionImports() {
   return true;
 }
 
+bool ParserImpl::IsGlslExtendedInstruction(
+    const spvtools::opt::Instruction& inst) const {
+  return (inst.opcode() == SpvOpExtInst) &&
+         (glsl_std_450_imports_.count(inst.GetSingleWordInOperand(0)) > 0);
+}
+
 bool ParserImpl::RegisterUserAndStructMemberNames() {
   if (!success_) {
     return false;
@@ -1389,10 +1451,21 @@ ast::Expression* ParserImpl::MakeNullValue(ast::type::Type* type) {
   return nullptr;
 }
 
-TypedExpression ParserImpl::RectifyOperandSignedness(SpvOp op,
+TypedExpression ParserImpl::RectifyOperandSignedness(
-                                                     TypedExpression&& expr) {
+    const spvtools::opt::Instruction& inst,
-  const bool requires_signed = AssumesSignedOperands(op);
+    TypedExpression&& expr) {
-  const bool requires_unsigned = AssumesUnsignedOperands(op);
+  bool requires_signed = false;
+  bool requires_unsigned = false;
+  if (IsGlslExtendedInstruction(inst)) {
+    const auto extended_opcode =
+        static_cast<GLSLstd450>(inst.GetSingleWordInOperand(1));
+    requires_signed = AssumesSignedOperands(extended_opcode);
+    requires_unsigned = AssumesUnsignedOperands(extended_opcode);
+  } else {
+    const auto opcode = inst.opcode();
+    requires_signed = AssumesSignedOperands(opcode);
+    requires_unsigned = AssumesUnsignedOperands(opcode);
+  }
   if (!requires_signed && !requires_unsigned) {
     // No conversion is required, assuming our tables are complete.
     return std::move(expr);
@@ -1425,14 +1498,26 @@ TypedExpression ParserImpl::RectifyOperandSignedness(SpvOp op,
 }
 
 ast::type::Type* ParserImpl::ForcedResultType(
-    SpvOp op,
+    const spvtools::opt::Instruction& inst,
     ast::type::Type* first_operand_type) {
-  const bool binary_match_first_operand =
+  const auto opcode = inst.opcode();
-      AssumesResultSignednessMatchesBinaryFirstOperand(op);
+  if ((opcode == SpvOpSNegate) || (opcode == SpvOpNot)) {
-  const bool unary_match_operand = (op == SpvOpSNegate) || (op == SpvOpNot);
+    // The unary operation cases that force the result type to match the
-  if (binary_match_first_operand || unary_match_operand) {
+    // first operand type.
     return first_operand_type;
   }
+  if (AssumesResultSignednessMatchesBinaryFirstOperand(opcode)) {
+    // The binary operation cases that force the result type to match
+    // the first operand type.
+    return first_operand_type;
+  }
+  if (IsGlslExtendedInstruction(inst)) {
+    const auto extended_opcode =
+        static_cast<GLSLstd450>(inst.GetSingleWordInOperand(1));
+    if (AssumesResultSignednessMatchesFirstOperand(extended_opcode)) {
+      return first_operand_type;
+    }
+  }
   return nullptr;
 }
 
@@ -1474,9 +1559,9 @@ ast::type::Type* ParserImpl::GetUnsignedIntMatchingShape(
 
 TypedExpression ParserImpl::RectifyForcedResultType(
     TypedExpression expr,
-    SpvOp op,
+    const spvtools::opt::Instruction& inst,
     ast::type::Type* first_operand_type) {
-  auto* forced_result_ty = ForcedResultType(op, first_operand_type);
+  auto* forced_result_ty = ForcedResultType(inst, first_operand_type);
   if ((forced_result_ty == nullptr) || (forced_result_ty == expr.type)) {
     return expr;
   }

src/reader/spirv/parser_impl.h

@@ -235,6 +235,12 @@ class ParserImpl : Reader {
   /// @returns true if parser is still successful.
   bool RegisterExtendedInstructionImports();
 
+  // Returns true when the given instruction is an extended instruction
+  // for GLSL.std.450.
+  // @param inst a SPIR-V instruction
+  // @returns true if its an SpvOpExtInst for GLSL.std.450
+  bool IsGlslExtendedInstruction(const spvtools::opt::Instruction& inst) const;
+
   /// Registers user names for SPIR-V objects, from OpName, and OpMemberName.
   /// Also synthesizes struct field names.  Ensures uniqueness for names for
   /// SPIR-V IDs, and uniqueness of names of fields within any single struct.
@@ -301,25 +307,27 @@ class ParserImpl : Reader {
   ast::Expression* MakeNullValue(ast::type::Type* type);
 
   /// Converts a given expression to the signedness demanded for an operand
-  /// of the given SPIR-V opcode, if required.  If the operation assumes
+  /// of the given SPIR-V instruction, if required.  If the instruction assumes
   /// signed integer operands, and `expr` is unsigned, then return an
   /// as-cast expression converting it to signed. Otherwise, return
   /// `expr` itself.  Similarly, convert as required from unsigned
   /// to signed. Assumes all SPIR-V types have been mapped to AST types.
-  /// @param op the SPIR-V opcode
+  /// @param inst the SPIR-V instruction
   /// @param expr an expression
   /// @returns expr, or a cast of expr
-  TypedExpression RectifyOperandSignedness(SpvOp op, TypedExpression&& expr);
+  TypedExpression RectifyOperandSignedness(
+      const spvtools::opt::Instruction& inst,
+      TypedExpression&& expr);
 
-  /// Returns the "forced" result type for the given SPIR-V opcode.
+  /// Returns the "forced" result type for the given SPIR-V instruction.
   /// If the WGSL result type for an operation has a more strict rule than
   /// requried by SPIR-V, then we say the result type is "forced".  This occurs
   /// for signed integer division (OpSDiv), for example, where the result type
   /// in WGSL must match the operand types.
-  /// @param op the SPIR-V opcode
+  /// @param inst the SPIR-V instruction
   /// @param first_operand_type the AST type for the first operand.
   /// @returns the forced AST result type, or nullptr if no forcing is required.
-  ast::type::Type* ForcedResultType(SpvOp op,
+  ast::type::Type* ForcedResultType(const spvtools::opt::Instruction& inst,
                                     ast::type::Type* first_operand_type);
 
   /// Returns a signed integer scalar or vector type matching the shape (scalar,
@@ -343,12 +351,13 @@ class ParserImpl : Reader {
   /// from the expression's result type. Otherwise, returns the given expression
   /// unchanged.
   /// @param expr the expression to pass through or to wrap
-  /// @param op the SPIR-V opcode
+  /// @param inst the SPIR-V instruction
   /// @param first_operand_type the AST type for the first operand.
   /// @returns the forced AST result type, or nullptr if no forcing is required.
-  TypedExpression RectifyForcedResultType(TypedExpression expr,
+  TypedExpression RectifyForcedResultType(
-                                          SpvOp op,
+      TypedExpression expr,
-                                          ast::type::Type* first_operand_type);
+      const spvtools::opt::Instruction& inst,
+      ast::type::Type* first_operand_type);
 
   /// @returns the registered boolean type.
   ast::type::Type* Bool() const { return bool_type_; }