Restore "MSL writer: make signed int overflow defined behaviour"

This reverts commit e33b0baa08.

Added tests/expressions/literals/intmin.wgsl test.

Bug: tint:124
Change-Id: I3d46f939ff20fa377ddb5fcb52f9afe728b8e430
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/60441
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Antonio Maiorano <amaiorano@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>
Reviewed-by: David Neto <dneto@google.com>

src/writer/msl/generator_impl.cc

@@ -75,6 +75,32 @@ bool last_is_break_or_fallthrough(const ast::BlockStatement* stmts) {
          stmts->last()->Is<ast::FallthroughStatement>();
 }
 
+class ScopedBitCast {
+ public:
+  ScopedBitCast(GeneratorImpl* generator,
+                std::ostream& stream,
+                const sem::Type* curr_type,
+                const sem::Type* target_type)
+      : s(stream) {
+    auto* target_vec_type = target_type->As<sem::Vector>();
+
+    // If we need to promote from scalar to vector, bitcast the scalar to the
+    // vector element type.
+    if (curr_type->is_scalar() && target_vec_type) {
+      target_type = target_vec_type->type();
+    }
+
+    // Bit cast
+    s << "as_type<";
+    generator->EmitType(s, target_type, "");
+    s << ">(";
+  }
+
+  ~ScopedBitCast() { s << ")"; }
+
+ private:
+  std::ostream& s;
+};
 }  // namespace
 
 GeneratorImpl::GeneratorImpl(const Program* program) : TextGenerator(program) {}
@@ -227,8 +253,104 @@ bool GeneratorImpl::EmitAssign(ast::AssignmentStatement* stmt) {
 }
 
 bool GeneratorImpl::EmitBinary(std::ostream& out, ast::BinaryExpression* expr) {
+  auto emit_op = [&] {
+    out << " ";
+
+    switch (expr->op()) {
+      case ast::BinaryOp::kAnd:
+        out << "&";
+        break;
+      case ast::BinaryOp::kOr:
+        out << "|";
+        break;
+      case ast::BinaryOp::kXor:
+        out << "^";
+        break;
+      case ast::BinaryOp::kLogicalAnd:
+        out << "&&";
+        break;
+      case ast::BinaryOp::kLogicalOr:
+        out << "||";
+        break;
+      case ast::BinaryOp::kEqual:
+        out << "==";
+        break;
+      case ast::BinaryOp::kNotEqual:
+        out << "!=";
+        break;
+      case ast::BinaryOp::kLessThan:
+        out << "<";
+        break;
+      case ast::BinaryOp::kGreaterThan:
+        out << ">";
+        break;
+      case ast::BinaryOp::kLessThanEqual:
+        out << "<=";
+        break;
+      case ast::BinaryOp::kGreaterThanEqual:
+        out << ">=";
+        break;
+      case ast::BinaryOp::kShiftLeft:
+        out << "<<";
+        break;
+      case ast::BinaryOp::kShiftRight:
+        // TODO(dsinclair): MSL is based on C++14, and >> in C++14 has
+        // implementation-defined behaviour for negative LHS.  We may have to
+        // generate extra code to implement WGSL-specified behaviour for
+        // negative LHS.
+        out << R"(>>)";
+        break;
+
+      case ast::BinaryOp::kAdd:
+        out << "+";
+        break;
+      case ast::BinaryOp::kSubtract:
+        out << "-";
+        break;
+      case ast::BinaryOp::kMultiply:
+        out << "*";
+        break;
+      case ast::BinaryOp::kDivide:
+        out << "/";
+        break;
+      case ast::BinaryOp::kModulo:
+        out << "%";
+        break;
+      case ast::BinaryOp::kNone:
+        diagnostics_.add_error(diag::System::Writer,
+                               "missing binary operation type");
+        return false;
+    }
+    out << " ";
+    return true;
+  };
+
+  auto signed_type_of = [&](const sem::Type* ty) -> const sem::Type* {
+    if (ty->is_integer_scalar()) {
+      return builder_.create<sem::I32>();
+    } else if (auto* v = ty->As<sem::Vector>()) {
+      return builder_.create<sem::Vector>(builder_.create<sem::I32>(),
+                                          v->Width());
+    }
+    return {};
+  };
+
+  auto unsigned_type_of = [&](const sem::Type* ty) -> const sem::Type* {
+    if (ty->is_integer_scalar()) {
+      return builder_.create<sem::U32>();
+    } else if (auto* v = ty->As<sem::Vector>()) {
+      return builder_.create<sem::Vector>(builder_.create<sem::U32>(),
+                                          v->Width());
+    }
+    return {};
+  };
+
+  auto* lhs_type = TypeOf(expr->lhs())->UnwrapRef();
+  auto* rhs_type = TypeOf(expr->rhs())->UnwrapRef();
+
+  // Handle fmod
   if (expr->op() == ast::BinaryOp::kModulo &&
-      TypeOf(expr)->UnwrapRef()->is_float_scalar_or_vector()) {
+      lhs_type->is_float_scalar_or_vector()) {
     out << "fmod";
     ScopedParen sp(out);
     if (!EmitExpression(out, expr->lhs())) {
@@ -241,80 +363,75 @@ bool GeneratorImpl::EmitBinary(std::ostream& out, ast::BinaryExpression* expr) {
     return true;
   }
 
-  ScopedParen sp(out);
+  // Handle +/-/* of signed values
+  if ((expr->IsAdd() || expr->IsSubtract() || expr->IsMultiply()) &&
+      lhs_type->is_signed_scalar_or_vector() &&
+      rhs_type->is_signed_scalar_or_vector()) {
+    // If lhs or rhs is a vector, use that type (support implicit scalar to
+    // vector promotion)
+    auto* target_type =
+        lhs_type->Is<sem::Vector>()
+            ? lhs_type
+            : (rhs_type->Is<sem::Vector>() ? rhs_type : lhs_type);
 
+    // WGSL defines behaviour for signed overflow, MSL does not. For these
+    // cases, bitcast operands to unsigned, then cast result to signed.
+    ScopedBitCast outer_int_cast(this, out, target_type,
+                                 signed_type_of(target_type));
+    ScopedParen sp(out);
+    {
+      ScopedBitCast lhs_uint_cast(this, out, lhs_type,
+                                  unsigned_type_of(target_type));
+      if (!EmitExpression(out, expr->lhs())) {
+        return false;
+      }
+    }
+    if (!emit_op()) {
+      return false;
+    }
+    {
+      ScopedBitCast rhs_uint_cast(this, out, rhs_type,
+                                  unsigned_type_of(target_type));
+      if (!EmitExpression(out, expr->rhs())) {
+        return false;
+      }
+    }
+    return true;
+  }
+
+  // Handle left bit shifting a signed value
+  // TODO(crbug.com/tint/1077): This may not be necessary. The MSL spec
+  // seems to imply that left shifting a signed value is treated the same as
+  // left shifting an unsigned value, but we need to make sure.
+  if (expr->IsShiftLeft() && lhs_type->is_signed_scalar_or_vector()) {
+    // Shift left: discards top bits, so convert first operand to unsigned
+    // first, then convert result back to signed
+    ScopedBitCast outer_int_cast(this, out, lhs_type, signed_type_of(lhs_type));
+    ScopedParen sp(out);
+    {
+      ScopedBitCast lhs_uint_cast(this, out, lhs_type,
+                                  unsigned_type_of(lhs_type));
+      if (!EmitExpression(out, expr->lhs())) {
+        return false;
+      }
+    }
+    if (!emit_op()) {
+      return false;
+    }
+    if (!EmitExpression(out, expr->rhs())) {
+      return false;
+    }
+    return true;
+  }
+
+  // Emit as usual
+  ScopedParen sp(out);
   if (!EmitExpression(out, expr->lhs())) {
     return false;
   }
-  out << " ";
-
-  switch (expr->op()) {
-    case ast::BinaryOp::kAnd:
-      out << "&";
-      break;
-    case ast::BinaryOp::kOr:
-      out << "|";
-      break;
-    case ast::BinaryOp::kXor:
-      out << "^";
-      break;
-    case ast::BinaryOp::kLogicalAnd:
-      out << "&&";
-      break;
-    case ast::BinaryOp::kLogicalOr:
-      out << "||";
-      break;
-    case ast::BinaryOp::kEqual:
-      out << "==";
-      break;
-    case ast::BinaryOp::kNotEqual:
-      out << "!=";
-      break;
-    case ast::BinaryOp::kLessThan:
-      out << "<";
-      break;
-    case ast::BinaryOp::kGreaterThan:
-      out << ">";
-      break;
-    case ast::BinaryOp::kLessThanEqual:
-      out << "<=";
-      break;
-    case ast::BinaryOp::kGreaterThanEqual:
-      out << ">=";
-      break;
-    case ast::BinaryOp::kShiftLeft:
-      out << "<<";
-      break;
-    case ast::BinaryOp::kShiftRight:
-      // TODO(dsinclair): MSL is based on C++14, and >> in C++14 has
-      // implementation-defined behaviour for negative LHS.  We may have to
-      // generate extra code to implement WGSL-specified behaviour for negative
-      // LHS.
-      out << R"(>>)";
-      break;
-
-    case ast::BinaryOp::kAdd:
-      out << "+";
-      break;
-    case ast::BinaryOp::kSubtract:
-      out << "-";
-      break;
-    case ast::BinaryOp::kMultiply:
-      out << "*";
-      break;
-    case ast::BinaryOp::kDivide:
-      out << "/";
-      break;
-    case ast::BinaryOp::kModulo:
-      out << "%";
-      break;
-    case ast::BinaryOp::kNone:
-      diagnostics_.add_error(diag::System::Writer,
-                             "missing binary operation type");
-      return false;
+  if (!emit_op()) {
+    return false;
   }
-  out << " ";
-
   if (!EmitExpression(out, expr->rhs())) {
     return false;
   }
@@ -2338,6 +2455,53 @@ bool GeneratorImpl::EmitStructType(TextBuffer* b, const sem::Struct* str) {
 
 bool GeneratorImpl::EmitUnaryOp(std::ostream& out,
                                 ast::UnaryOpExpression* expr) {
+  // Handle `-e` when `e` is signed, so that we ensure that if `e` is the
+  // largest negative value, it returns `e`.
+  auto* expr_type = TypeOf(expr->expr())->UnwrapRef();
+  if (expr->op() == ast::UnaryOp::kNegation &&
+      expr_type->is_signed_scalar_or_vector()) {
+    auto fn =
+        utils::GetOrCreate(unary_minus_funcs_, expr_type, [&]() -> std::string {
+          // e.g.:
+          // int tint_unary_minus(const int v) {
+          //     return (v == -2147483648) ? v : -v;
+          // }
+          TextBuffer b;
+          TINT_DEFER(helpers_.Append(b));
+
+          auto fn_name = UniqueIdentifier("tint_unary_minus");
+          {
+            auto decl = line(&b);
+            if (!EmitTypeAndName(decl, expr_type, fn_name)) {
+              return "";
+            }
+            decl << "(const ";
+            if (!EmitType(decl, expr_type, "")) {
+              return "";
+            }
+            decl << " v) {";
+          }
+
+          {
+            ScopedIndent si(&b);
+            const auto largest_negative_value =
+                std::to_string(std::numeric_limits<int32_t>::min());
+            line(&b) << "return select(-v, v, v == " << largest_negative_value
+                     << ");";
+          }
+          line(&b) << "}";
+          line(&b);
+          return fn_name;
+        });
+
+    out << fn << "(";
+    if (!EmitExpression(out, expr->expr())) {
+      return false;
+    }
+    out << ")";
+    return true;
+  }
+
   switch (expr->op()) {
     case ast::UnaryOp::kAddressOf:
       out << "&";

src/writer/msl/generator_impl.h

@@ -356,6 +356,7 @@ class GeneratorImpl : public TextGenerator {
   bool has_invariant_ = false;
 
   std::unordered_map<const sem::Intrinsic*, std::string> intrinsics_;
+  std::unordered_map<const sem::Type*, std::string> unary_minus_funcs_;
 };
 
 }  // namespace msl

src/writer/msl/generator_impl_binary_test.cc

@@ -74,6 +74,85 @@ INSTANTIATE_TEST_SUITE_P(
         BinaryData{"(left / right)", ast::BinaryOp::kDivide},
         BinaryData{"(left % right)", ast::BinaryOp::kModulo}));
 
+using MslBinaryTest_SignedOverflowDefinedBehaviour =
+    TestParamHelper<BinaryData>;
+TEST_P(MslBinaryTest_SignedOverflowDefinedBehaviour, Emit) {
+  auto params = GetParam();
+
+  auto* a_type = ty.i32();
+  auto* b_type = (params.op == ast::BinaryOp::kShiftLeft ||
+                  params.op == ast::BinaryOp::kShiftRight)
+                     ? static_cast<ast::Type*>(ty.u32())
+                     : ty.i32();
+
+  auto* a = Var("a", a_type);
+  auto* b = Var("b", b_type);
+
+  auto* expr = create<ast::BinaryExpression>(params.op, Expr(a), Expr(b));
+  WrapInFunction(a, b, expr);
+
+  GeneratorImpl& gen = Build();
+
+  std::stringstream out;
+  ASSERT_TRUE(gen.EmitExpression(out, expr)) << gen.error();
+  EXPECT_EQ(out.str(), params.result);
+}
+using Op = ast::BinaryOp;
+constexpr BinaryData signed_overflow_defined_behaviour_cases[] = {
+    {"as_type<int>((as_type<uint>(a) << b))", Op::kShiftLeft},
+    {"(a >> b)", Op::kShiftRight},
+    {"as_type<int>((as_type<uint>(a) + as_type<uint>(b)))", Op::kAdd},
+    {"as_type<int>((as_type<uint>(a) - as_type<uint>(b)))", Op::kSubtract},
+    {"as_type<int>((as_type<uint>(a) * as_type<uint>(b)))", Op::kMultiply}};
+INSTANTIATE_TEST_SUITE_P(
+    MslGeneratorImplTest,
+    MslBinaryTest_SignedOverflowDefinedBehaviour,
+    testing::ValuesIn(signed_overflow_defined_behaviour_cases));
+
+using MslBinaryTest_SignedOverflowDefinedBehaviour_Chained =
+    TestParamHelper<BinaryData>;
+TEST_P(MslBinaryTest_SignedOverflowDefinedBehaviour_Chained, Emit) {
+  auto params = GetParam();
+
+  auto* a_type = ty.i32();
+  auto* b_type = (params.op == ast::BinaryOp::kShiftLeft ||
+                  params.op == ast::BinaryOp::kShiftRight)
+                     ? static_cast<ast::Type*>(ty.u32())
+                     : ty.i32();
+
+  auto* a = Var("a", a_type);
+  auto* b = Var("b", b_type);
+
+  auto* expr1 = create<ast::BinaryExpression>(params.op, Expr(a), Expr(b));
+  auto* expr2 = create<ast::BinaryExpression>(params.op, expr1, Expr(b));
+  WrapInFunction(a, b, expr2);
+
+  GeneratorImpl& gen = Build();
+
+  std::stringstream out;
+  ASSERT_TRUE(gen.EmitExpression(out, expr2)) << gen.error();
+  EXPECT_EQ(out.str(), params.result);
+}
+using Op = ast::BinaryOp;
+constexpr BinaryData signed_overflow_defined_behaviour_chained_cases[] = {
+    {"as_type<int>((as_type<uint>(as_type<int>((as_type<uint>(a) << b))) << "
+     "b))",
+     Op::kShiftLeft},
+    {"((a >> b) >> b)", Op::kShiftRight},
+    {"as_type<int>((as_type<uint>(as_type<int>((as_type<uint>(a) + "
+     "as_type<uint>(b)))) + as_type<uint>(b)))",
+     Op::kAdd},
+    {"as_type<int>((as_type<uint>(as_type<int>((as_type<uint>(a) - "
+     "as_type<uint>(b)))) - as_type<uint>(b)))",
+     Op::kSubtract},
+    {"as_type<int>((as_type<uint>(as_type<int>((as_type<uint>(a) * "
+     "as_type<uint>(b)))) * as_type<uint>(b)))",
+     Op::kMultiply}};
+INSTANTIATE_TEST_SUITE_P(
+    MslGeneratorImplTest,
+    MslBinaryTest_SignedOverflowDefinedBehaviour_Chained,
+    testing::ValuesIn(signed_overflow_defined_behaviour_chained_cases));
+
 TEST_F(MslBinaryTest, ModF32) {
   auto* left = Var("left", ty.f32());
   auto* right = Var("right", ty.f32());

src/writer/msl/generator_impl_intrinsic_test.cc

@@ -360,7 +360,7 @@ TEST_F(MslGeneratorImplTest, Ignore) {
 
 using namespace metal;
 int f(int a, int b, int c) {
-  return ((a + b) * c);
+  return as_type<int>((as_type<uint>(as_type<int>((as_type<uint>(a) + as_type<uint>(b)))) * as_type<uint>(c)));
 }
 
 kernel void func() {

src/writer/msl/generator_impl_loop_test.cc

@@ -259,7 +259,9 @@ TEST_F(MslGeneratorImplTest, Emit_ForLoopWithSimpleCont) {
   gen.increment_indent();
 
   ASSERT_TRUE(gen.EmitStatement(f)) << gen.error();
-  EXPECT_EQ(gen.result(), R"(  for(; ; i = (i + 1)) {
+  EXPECT_EQ(
+      gen.result(),
+      R"(  for(; ; i = as_type<int>((as_type<uint>(i) + as_type<uint>(1)))) {
     a_statement();
   }
 )");
@@ -310,7 +312,9 @@ TEST_F(MslGeneratorImplTest, Emit_ForLoopWithSimpleInitCondCont) {
   gen.increment_indent();
 
   ASSERT_TRUE(gen.EmitStatement(f)) << gen.error();
-  EXPECT_EQ(gen.result(), R"(  for(int i = 0; true; i = (i + 1)) {
+  EXPECT_EQ(
+      gen.result(),
+      R"(  for(int i = 0; true; i = as_type<int>((as_type<uint>(i) + as_type<uint>(1)))) {
     a_statement();
   }
 )");

src/writer/msl/generator_impl_unary_op_test.cc

@@ -85,7 +85,7 @@ TEST_F(MslUnaryOpTest, Negation) {
 
   std::stringstream out;
   ASSERT_TRUE(gen.EmitExpression(out, op)) << gen.error();
-  EXPECT_EQ(out.str(), "-(expr)");
+  EXPECT_EQ(out.str(), "tint_unary_minus(expr)");
 }
 
 TEST_F(MslUnaryOpTest, NegationOfIntMin) {
@@ -97,7 +97,7 @@ TEST_F(MslUnaryOpTest, NegationOfIntMin) {
 
   std::stringstream out;
   ASSERT_TRUE(gen.EmitExpression(out, op)) << gen.error();
-  EXPECT_EQ(out.str(), "-((-2147483647 - 1))");
+  EXPECT_EQ(out.str(), "tint_unary_minus((-2147483647 - 1))");
 }
 
 }  // namespace