[hlsl-writer] Emit LogicalAnd and LogicalOr operations

This Cl emits the required code to handle LogicalAnd and LogicalOr short
circuting in HLSL.

Bug: tint:192
Change-Id: I30a88c207f7864e0a3c856d2dae1420c7ff35eb4
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/27782
Reviewed-by: David Neto <dneto@google.com>
Commit-Queue: David Neto <dneto@google.com>

src/writer/hlsl/generator_impl.cc

@@ -161,7 +161,7 @@ void GeneratorImpl::register_global(ast::Variable* global) {
 std::string GeneratorImpl::generate_name(const std::string& prefix) {
   std::string name = prefix;
   uint32_t i = 0;
-  while (namer_.IsMapped(name)) {
+  while (namer_.IsMapped(name) || namer_.IsRemapped(name)) {
     name = prefix + "_" + std::to_string(i);
     ++i;
   }
@@ -302,8 +302,97 @@ bool GeneratorImpl::EmitAssign(std::ostream& out,
 bool GeneratorImpl::EmitBinary(std::ostream& pre,
                                std::ostream& out,
                                ast::BinaryExpression* expr) {
-  out << "(";
+  if (expr->op() == ast::BinaryOp::kLogicalAnd) {
+    std::ostringstream lhs_out;
+    if (!EmitExpression(pre, lhs_out, expr->lhs())) {
+      return false;
+    }
 
+    auto name = generate_name(kTempNamePrefix);
+    make_indent(pre);
+    pre << "bool " << name << " = false;" << std::endl;
+    make_indent(pre);
+    pre << "if (" << lhs_out.str() << ") {" << std::endl;
+    {
+      increment_indent();
+
+      std::ostringstream rhs_out;
+      if (!EmitExpression(pre, rhs_out, expr->rhs())) {
+        return false;
+      }
+      make_indent(pre);
+      pre << "if (" << rhs_out.str() << ") {" << std::endl;
+      {
+        increment_indent();
+
+        make_indent(pre);
+        pre << name << " = true;" << std::endl;
+
+        decrement_indent();
+      }
+      make_indent(pre);
+      pre << "}" << std::endl;
+
+      decrement_indent();
+    }
+    make_indent(pre);
+    pre << "}" << std::endl;
+
+    out << "(" << name << ")";
+    return true;
+  }
+  if (expr->op() == ast::BinaryOp::kLogicalOr) {
+    std::ostringstream lhs_out;
+    if (!EmitExpression(pre, lhs_out, expr->lhs())) {
+      return false;
+    }
+
+    auto name = generate_name(kTempNamePrefix);
+    make_indent(pre);
+    pre << "bool " << name << " = false;" << std::endl;
+    make_indent(pre);
+    pre << "if (" << lhs_out.str() << ") {" << std::endl;
+    {
+      increment_indent();
+
+      make_indent(pre);
+      pre << name << " = true;" << std::endl;
+
+      decrement_indent();
+    }
+
+    make_indent(pre);
+    pre << "} else {" << std::endl;
+    {
+      increment_indent();
+
+      std::ostringstream rhs_out;
+      if (!EmitExpression(pre, rhs_out, expr->rhs())) {
+        return false;
+      }
+      make_indent(pre);
+      pre << "if (" << rhs_out.str() << ") {" << std::endl;
+      {
+        increment_indent();
+
+        make_indent(pre);
+        pre << name << " = true;" << std::endl;
+
+        decrement_indent();
+      }
+      make_indent(pre);
+      pre << "}" << std::endl;
+
+      decrement_indent();
+    }
+    make_indent(pre);
+    pre << "}" << std::endl;
+
+    out << "(" << name << ")";
+    return true;
+  }
+
+  out << "(";
   if (!EmitExpression(pre, out, expr->lhs())) {
     return false;
   }
@@ -320,13 +409,11 @@ bool GeneratorImpl::EmitBinary(std::ostream& pre,
       out << "^";
       break;
     case ast::BinaryOp::kLogicalAnd:
-      // TODO(dsinclair): Implement support ...
-      error_ = "&& not supported yet";
-      return false;
-    case ast::BinaryOp::kLogicalOr:
-      // TODO(dsinclair): Implement support ...
-      error_ = "|| not supported yet";
+    case ast::BinaryOp::kLogicalOr: {
+      // These are both handled above.
+      assert(false);
       return false;
+    }
     case ast::BinaryOp::kEqual:
       out << "==";
       break;

src/writer/hlsl/generator_impl_binary_test.cc

@@ -24,6 +24,8 @@ namespace writer {
 namespace hlsl {
 namespace {
 
+using HlslGeneratorImplTest_Binary = TestHelper;
+
 struct BinaryData {
   const char* result;
   ast::BinaryOp op;
@@ -52,8 +54,6 @@ INSTANTIATE_TEST_SUITE_P(
         BinaryData{"(left & right)", ast::BinaryOp::kAnd},
         BinaryData{"(left | right)", ast::BinaryOp::kOr},
         BinaryData{"(left ^ right)", ast::BinaryOp::kXor},
-        // BinaryData{"(left && right)", ast::BinaryOp::kLogicalAnd},
-        // BinaryData{"(left || right)", ast::BinaryOp::kLogicalOr},
         BinaryData{"(left == right)", ast::BinaryOp::kEqual},
         BinaryData{"(left != right)", ast::BinaryOp::kNotEqual},
         BinaryData{"(left < right)", ast::BinaryOp::kLessThan},
@@ -68,6 +68,85 @@ INSTANTIATE_TEST_SUITE_P(
         BinaryData{"(left / right)", ast::BinaryOp::kDivide},
         BinaryData{"(left % right)", ast::BinaryOp::kModulo}));
 
+TEST_F(HlslGeneratorImplTest_Binary, Logical_And) {
+  auto left = std::make_unique<ast::IdentifierExpression>("left");
+  auto right = std::make_unique<ast::IdentifierExpression>("right");
+
+  ast::BinaryExpression expr(ast::BinaryOp::kLogicalAnd, std::move(left),
+                             std::move(right));
+
+  ASSERT_TRUE(gen().EmitExpression(pre(), out(), &expr)) << gen().error();
+  EXPECT_EQ(result(), "(_tint_tmp)");
+  EXPECT_EQ(pre_result(), R"(bool _tint_tmp = false;
+if (left) {
+  if (right) {
+    _tint_tmp = true;
+  }
+}
+)");
+}
+
+TEST_F(HlslGeneratorImplTest_Binary, Logical_Multi) {
+  // (a && b) || (c || d)
+  auto a = std::make_unique<ast::IdentifierExpression>("a");
+  auto b = std::make_unique<ast::IdentifierExpression>("b");
+  auto c = std::make_unique<ast::IdentifierExpression>("c");
+  auto d = std::make_unique<ast::IdentifierExpression>("d");
+
+  ast::BinaryExpression expr(
+      ast::BinaryOp::kLogicalOr,
+      std::make_unique<ast::BinaryExpression>(ast::BinaryOp::kLogicalAnd,
+                                              std::move(a), std::move(b)),
+      std::make_unique<ast::BinaryExpression>(ast::BinaryOp::kLogicalOr,
+                                              std::move(c), std::move(d)));
+
+  ASSERT_TRUE(gen().EmitExpression(pre(), out(), &expr)) << gen().error();
+  EXPECT_EQ(result(), "(_tint_tmp_0)");
+  EXPECT_EQ(pre_result(), R"(bool _tint_tmp = false;
+if (a) {
+  if (b) {
+    _tint_tmp = true;
+  }
+}
+bool _tint_tmp_0 = false;
+if ((_tint_tmp)) {
+  _tint_tmp_0 = true;
+} else {
+  bool _tint_tmp_1 = false;
+  if (c) {
+    _tint_tmp_1 = true;
+  } else {
+    if (d) {
+      _tint_tmp_1 = true;
+    }
+  }
+  if ((_tint_tmp_1)) {
+    _tint_tmp_0 = true;
+  }
+}
+)");
+}
+
+TEST_F(HlslGeneratorImplTest_Binary, Logical_Or) {
+  auto left = std::make_unique<ast::IdentifierExpression>("left");
+  auto right = std::make_unique<ast::IdentifierExpression>("right");
+
+  ast::BinaryExpression expr(ast::BinaryOp::kLogicalOr, std::move(left),
+                             std::move(right));
+
+  ASSERT_TRUE(gen().EmitExpression(pre(), out(), &expr)) << gen().error();
+  EXPECT_EQ(result(), "(_tint_tmp)");
+  EXPECT_EQ(pre_result(), R"(bool _tint_tmp = false;
+if (left) {
+  _tint_tmp = true;
+} else {
+  if (right) {
+    _tint_tmp = true;
+  }
+}
+)");
+}
+
 }  // namespace
 }  // namespace hlsl
 }  // namespace writer

src/writer/hlsl/namer.cc

@@ -664,8 +664,7 @@ std::string Namer::NameFor(const std::string& name) {
     uint32_t i = 0;
     // Make sure the ident name wasn't assigned by a remapping.
     while (true) {
-      auto remap_it = remapped_names_.find(ret_name);
-      if (remap_it == remapped_names_.end()) {
+      if (!IsRemapped(ret_name)) {
         break;
       }
       ret_name = name + "_" + std::to_string(i);
@@ -683,6 +682,11 @@ bool Namer::IsMapped(const std::string& name) {
   return it != name_map_.end();
 }
 
+bool Namer::IsRemapped(const std::string& name) {
+  auto it = remapped_names_.find(name);
+  return it != remapped_names_.end();
+}
+
 void Namer::RegisterRemappedName(const std::string& name) {
   remapped_names_.insert(name);
 }

src/writer/hlsl/namer.h

@@ -39,11 +39,16 @@ class Namer {
   /// @param name the name to register
   void RegisterRemappedName(const std::string& name);
 
-  /// Returns if the given name has been mapped alread
+  /// Returns if the given name has been mapped already
   /// @param name the name to check
   /// @returns true if the name has been mapped
   bool IsMapped(const std::string& name);
 
+  /// Returns if the given name has been remapped already
+  /// @param name the name to check
+  /// @returns true if the name has been remapped
+  bool IsRemapped(const std::string& name);
+
  private:
   /// Map of original name to new name. The two names may be the same.
   std::unordered_map<std::string, std::string> name_map_;