From 764110d6751ea1d603e70d439ebd93466f875abc Mon Sep 17 00:00:00 2001 From: dan sinclair Date: Mon, 31 Aug 2020 14:15:51 +0000 Subject: [PATCH] [msl-writer][hlsl-writer] Pull loop variables out with continuing. If there is a continuing block we pull the variables declared in the loop up into the scope outside the loop. This allows those variables to be used in the continuing block. We pull out all variables instead of detecting ones which are only used in continuing as that's easier and still correct. Bug: tint:187, tint:186 Change-Id: I1de0e36111a236ff04a323cf9777bc79e67afa77 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/27620 Commit-Queue: David Neto Reviewed-by: Sarah Mashayekhi Reviewed-by: David Neto --- src/writer/hlsl/generator_impl.cc | 41 +++++++++++++++++++-- src/writer/hlsl/generator_impl.h | 5 ++- src/writer/hlsl/generator_impl_loop_test.cc | 30 ++++++++++++--- src/writer/msl/generator_impl.cc | 39 ++++++++++++++++++-- src/writer/msl/generator_impl.h | 3 +- src/writer/msl/generator_impl_loop_test.cc | 34 +++++++++++++---- 6 files changed, 132 insertions(+), 20 deletions(-) diff --git a/src/writer/hlsl/generator_impl.cc b/src/writer/hlsl/generator_impl.cc index ca3a445454..a6f454dd3b 100644 --- a/src/writer/hlsl/generator_impl.cc +++ b/src/writer/hlsl/generator_impl.cc @@ -1470,6 +1470,19 @@ bool GeneratorImpl::EmitLoop(std::ostream& out, ast::LoopStatement* stmt) { make_indent(out); out << "bool " << guard << " = true;" << std::endl; + + // A continuing block may use variables declared in the method body. As a + // first pass, if we have a continuing, we pull all declarations outside + // the for loop into the continuing scope. Then, the variable declarations + // will be turned into assignments. + for (const auto& s : *(stmt->body())) { + if (!s->IsVariableDecl()) { + continue; + } + if (!EmitVariable(out, s->AsVariableDecl()->variable(), true)) { + return false; + } + } } make_indent(out); @@ -1490,6 +1503,26 @@ bool GeneratorImpl::EmitLoop(std::ostream& out, ast::LoopStatement* stmt) { } for (const auto& s : *(stmt->body())) { + // If we have a continuing block we've already emitted the variable + // declaration before the loop, so treat it as an assignment. + if (s->IsVariableDecl() && stmt->has_continuing()) { + make_indent(out); + + auto* var = s->AsVariableDecl()->variable(); + out << var->name() << " = "; + if (var->constructor() != nullptr) { + if (!EmitExpression(out, var->constructor())) { + return false; + } + } else { + if (!EmitZeroValue(out, var->type())) { + return false; + } + } + out << ";" << std::endl; + continue; + } + if (!EmitStatement(out, s.get())) { return false; } @@ -1831,7 +1864,7 @@ bool GeneratorImpl::EmitStatement(std::ostream& out, ast::Statement* stmt) { return EmitSwitch(out, stmt->AsSwitch()); } if (stmt->IsVariableDecl()) { - return EmitVariable(out, stmt->AsVariableDecl()->variable()); + return EmitVariable(out, stmt->AsVariableDecl()->variable(), false); } error_ = "unknown statement type: " + stmt->str(); @@ -1982,7 +2015,9 @@ bool GeneratorImpl::EmitUnaryOp(std::ostream& out, return true; } -bool GeneratorImpl::EmitVariable(std::ostream& out, ast::Variable* var) { +bool GeneratorImpl::EmitVariable(std::ostream& out, + ast::Variable* var, + bool skip_constructor) { make_indent(out); // TODO(dsinclair): Handle variable decorations @@ -2001,7 +2036,7 @@ bool GeneratorImpl::EmitVariable(std::ostream& out, ast::Variable* var) { out << " " << var->name(); } - if (var->constructor() != nullptr) { + if (!skip_constructor && var->constructor() != nullptr) { out << " = "; if (!EmitExpression(out, var->constructor())) { return false; diff --git a/src/writer/hlsl/generator_impl.h b/src/writer/hlsl/generator_impl.h index e087c77dcc..ba3f60b595 100644 --- a/src/writer/hlsl/generator_impl.h +++ b/src/writer/hlsl/generator_impl.h @@ -255,8 +255,11 @@ class GeneratorImpl { /// Handles generating a variable /// @param out the output stream /// @param var the variable to generate + /// @param skip_constructor set true if the constructor should be skipped /// @returns true if the variable was emitted - bool EmitVariable(std::ostream& out, ast::Variable* var); + bool EmitVariable(std::ostream& out, + ast::Variable* var, + bool skip_constructor); /// Handles generating a program scope constant variable /// @param out the output stream /// @param var the variable to emit diff --git a/src/writer/hlsl/generator_impl_loop_test.cc b/src/writer/hlsl/generator_impl_loop_test.cc index 54d600ef9a..576b7b263e 100644 --- a/src/writer/hlsl/generator_impl_loop_test.cc +++ b/src/writer/hlsl/generator_impl_loop_test.cc @@ -122,9 +122,28 @@ TEST_F(HlslGeneratorImplTest_Loop, Emit_LoopNestedWithContinuing) { )"); } -// TODO(dsinclair): Handle pulling declared variables up and out of the for() if -// there is a continuing block. -TEST_F(HlslGeneratorImplTest_Loop, DISABLED_Emit_LoopWithVarUsedInContinuing) { +TEST_F(HlslGeneratorImplTest_Loop, Emit_LoopWithVarUsedInContinuing) { + // loop { + // var lhs : f32 = 2.4; + // var other : f32; + // continuing { + // lhs = rhs + // } + // } + // + // -> + // { + // float lhs; + // float other; + // for (;;) { + // if (continuing) { + // lhs = rhs; + // } + // lhs = 2.4f; + // other = 0.0f; + // } + // } + ast::type::F32Type f32; auto var = std::make_unique( @@ -150,8 +169,9 @@ TEST_F(HlslGeneratorImplTest_Loop, DISABLED_Emit_LoopWithVarUsedInContinuing) { ASSERT_TRUE(gen().EmitStatement(out(), &outer)) << gen().error(); EXPECT_EQ(result(), R"( { - float lhs; bool tint_hlsl_is_first_1 = true; + float lhs; + float other; for(;;) { if (!tint_hlsl_is_first_1) { lhs = rhs; @@ -159,7 +179,7 @@ TEST_F(HlslGeneratorImplTest_Loop, DISABLED_Emit_LoopWithVarUsedInContinuing) { tint_hlsl_is_first_1 = false; lhs = 2.40000010f; - float other; + other = 0.0f; } } )"); diff --git a/src/writer/msl/generator_impl.cc b/src/writer/msl/generator_impl.cc index ce0d14e227..6bc21fd079 100644 --- a/src/writer/msl/generator_impl.cc +++ b/src/writer/msl/generator_impl.cc @@ -1462,6 +1462,19 @@ bool GeneratorImpl::EmitLoop(ast::LoopStatement* stmt) { make_indent(); out_ << "bool " << guard << " = true;" << std::endl; + + // A continuing block may use variables declared in the method body. As a + // first pass, if we have a continuing, we pull all declarations outside + // the for loop into the continuing scope. Then, the variable declarations + // will be turned into assignments. + for (const auto& s : *(stmt->body())) { + if (!s->IsVariableDecl()) { + continue; + } + if (!EmitVariable(s->AsVariableDecl()->variable(), true)) { + return false; + } + } } make_indent(); @@ -1482,6 +1495,26 @@ bool GeneratorImpl::EmitLoop(ast::LoopStatement* stmt) { } for (const auto& s : *(stmt->body())) { + // If we have a continuing block we've already emitted the variable + // declaration before the loop, so treat it as an assignment. + if (s->IsVariableDecl() && stmt->has_continuing()) { + make_indent(); + + auto* var = s->AsVariableDecl()->variable(); + out_ << var->name() << " = "; + if (var->constructor() != nullptr) { + if (!EmitExpression(var->constructor())) { + return false; + } + } else { + if (!EmitZeroValue(var->type())) { + return false; + } + } + out_ << ";" << std::endl; + continue; + } + if (!EmitStatement(s.get())) { return false; } @@ -1652,7 +1685,7 @@ bool GeneratorImpl::EmitStatement(ast::Statement* stmt) { return EmitSwitch(stmt->AsSwitch()); } if (stmt->IsVariableDecl()) { - return EmitVariable(stmt->AsVariableDecl()->variable()); + return EmitVariable(stmt->AsVariableDecl()->variable(), false); } error_ = "unknown statement type: " + stmt->str(); @@ -1815,7 +1848,7 @@ bool GeneratorImpl::EmitUnaryOp(ast::UnaryOpExpression* expr) { return true; } -bool GeneratorImpl::EmitVariable(ast::Variable* var) { +bool GeneratorImpl::EmitVariable(ast::Variable* var, bool skip_constructor) { make_indent(); // TODO(dsinclair): Handle variable decorations @@ -1834,7 +1867,7 @@ bool GeneratorImpl::EmitVariable(ast::Variable* var) { out_ << " " << var->name(); } - if (var->constructor() != nullptr) { + if (!skip_constructor && var->constructor() != nullptr) { out_ << " = "; if (!EmitExpression(var->constructor())) { return false; diff --git a/src/writer/msl/generator_impl.h b/src/writer/msl/generator_impl.h index 1a9e0b93de..b574b43b82 100644 --- a/src/writer/msl/generator_impl.h +++ b/src/writer/msl/generator_impl.h @@ -200,8 +200,9 @@ class GeneratorImpl : public TextGenerator { bool EmitUnaryOp(ast::UnaryOpExpression* expr); /// Handles generating a variable /// @param var the variable to generate + /// @param skip_constructor set true if the constructor should be skipped /// @returns true if the variable was emitted - bool EmitVariable(ast::Variable* var); + bool EmitVariable(ast::Variable* var, bool skip_constructor); /// Handles generating a program scope constant variable /// @param var the variable to emit /// @returns true if the variable was emitted diff --git a/src/writer/msl/generator_impl_loop_test.cc b/src/writer/msl/generator_impl_loop_test.cc index 1cf6609239..c11efdcad6 100644 --- a/src/writer/msl/generator_impl_loop_test.cc +++ b/src/writer/msl/generator_impl_loop_test.cc @@ -132,9 +132,28 @@ TEST_F(MslGeneratorImplTest, Emit_LoopNestedWithContinuing) { )"); } -// TODO(dsinclair): Handle pulling declared variables up and out of the for() if -// there is a continuing block. -TEST_F(MslGeneratorImplTest, DISABLED_Emit_LoopWithVarUsedInContinuing) { +TEST_F(MslGeneratorImplTest, Emit_LoopWithVarUsedInContinuing) { + // loop { + // var lhs : f32 = 2.4; + // var other : f32; + // continuing { + // lhs = rhs + // } + // } + // + // -> + // { + // float lhs; + // float other; + // for (;;) { + // if (continuing) { + // lhs = rhs; + // } + // lhs = 2.4f; + // other = 0.0f; + // } + // } + ast::type::F32Type f32; auto var = std::make_unique( @@ -155,16 +174,17 @@ TEST_F(MslGeneratorImplTest, DISABLED_Emit_LoopWithVarUsedInContinuing) { continuing->append(std::make_unique( std::move(lhs), std::move(rhs))); - ast::LoopStatement outer(std::move(body), std::move(continuing)); - ast::Module m; GeneratorImpl g(&m); g.increment_indent(); + ast::LoopStatement outer(std::move(body), std::move(continuing)); + ASSERT_TRUE(g.EmitStatement(&outer)) << g.error(); EXPECT_EQ(g.result(), R"( { - float lhs; bool tint_msl_is_first_1 = true; + float lhs; + float other; for(;;) { if (!tint_msl_is_first_1) { lhs = rhs; @@ -172,7 +192,7 @@ TEST_F(MslGeneratorImplTest, DISABLED_Emit_LoopWithVarUsedInContinuing) { tint_msl_is_first_1 = false; lhs = 2.40000010f; - float other; + other = 0.0f; } } )");