From b39c77e37071bf9c1596f2896c7bdcc99957b278 Mon Sep 17 00:00:00 2001 From: dan sinclair Date: Wed, 2 Sep 2020 15:02:45 +0000 Subject: [PATCH] [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 Commit-Queue: David Neto --- src/writer/hlsl/generator_impl.cc | 103 ++++++++++++++++-- src/writer/hlsl/generator_impl_binary_test.cc | 83 +++++++++++++- src/writer/hlsl/namer.cc | 8 +- src/writer/hlsl/namer.h | 7 +- 4 files changed, 188 insertions(+), 13 deletions(-) diff --git a/src/writer/hlsl/generator_impl.cc b/src/writer/hlsl/generator_impl.cc index a2b15ead7b..bba6c0fc34 100644 --- a/src/writer/hlsl/generator_impl.cc +++ b/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; diff --git a/src/writer/hlsl/generator_impl_binary_test.cc b/src/writer/hlsl/generator_impl_binary_test.cc index 024aa42f16..c1b04bb951 100644 --- a/src/writer/hlsl/generator_impl_binary_test.cc +++ b/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("left"); + auto right = std::make_unique("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("a"); + auto b = std::make_unique("b"); + auto c = std::make_unique("c"); + auto d = std::make_unique("d"); + + ast::BinaryExpression expr( + ast::BinaryOp::kLogicalOr, + std::make_unique(ast::BinaryOp::kLogicalAnd, + std::move(a), std::move(b)), + std::make_unique(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("left"); + auto right = std::make_unique("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 diff --git a/src/writer/hlsl/namer.cc b/src/writer/hlsl/namer.cc index 8c04e9152e..a577d639e2 100644 --- a/src/writer/hlsl/namer.cc +++ b/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); } diff --git a/src/writer/hlsl/namer.h b/src/writer/hlsl/namer.h index be3b521572..b5da343908 100644 --- a/src/writer/hlsl/namer.h +++ b/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 name_map_;