From 26ebe5ec3644decb5d47d64580d50ab80a7c8c64 Mon Sep 17 00:00:00 2001 From: James Price Date: Fri, 29 Apr 2022 00:14:53 +0000 Subject: [PATCH] tint: Refactor if-else statement representation Instead of using an `if` node that has a list of `else` statements, make each `if` statement have a single optional `else` statement, which may itself be an `if` statement (or just a block statement). This better matches the WGSL grammar (now that we have removed `elseif`), and simplifies various pieces of code that handle these statements. Change-Id: Ie4272f1422224490ac598a03aa8b4dd00ba03010 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/87940 Kokoro: Kokoro Reviewed-by: Antonio Maiorano Commit-Queue: James Price --- docs/tint/compound_statements.md | 4 +- src/tint/BUILD.gn | 2 - src/tint/CMakeLists.txt | 4 - src/tint/ast/else_statement.cc | 45 ------- src/tint/ast/else_statement.h | 59 --------- src/tint/ast/else_statement_test.cc | 92 -------------- src/tint/ast/if_statement.cc | 17 ++- src/tint/ast/if_statement.h | 11 +- src/tint/ast/if_statement_test.cc | 26 ++-- src/tint/ast/statement.cc | 3 - src/tint/program_builder.h | 44 ++----- src/tint/reader/spirv/function.cc | 26 ++-- src/tint/reader/wgsl/parser_impl.cc | 116 ++++++++++-------- src/tint/reader/wgsl/parser_impl.h | 3 - .../wgsl/parser_impl_elseif_stmt_test.cc | 66 ---------- .../reader/wgsl/parser_impl_if_stmt_test.cc | 30 ++--- src/tint/resolver/compound_statement_test.cc | 18 +-- src/tint/resolver/dependency_graph.cc | 5 +- src/tint/resolver/dependency_graph_test.cc | 4 +- src/tint/resolver/resolver.cc | 49 +------- src/tint/resolver/resolver.h | 2 - src/tint/resolver/resolver_behavior_test.cc | 6 +- src/tint/resolver/resolver_test.cc | 5 +- src/tint/resolver/type_validation_test.cc | 4 +- src/tint/resolver/validation_test.cc | 72 +++++------ src/tint/resolver/validator.cc | 46 +++---- src/tint/resolver/validator.h | 6 - src/tint/resolver/var_let_validation_test.cc | 4 +- src/tint/sem/if_statement.cc | 12 -- src/tint/sem/if_statement.h | 34 ----- src/tint/sem/type_mappings.h | 3 - src/tint/transform/loop_to_for_loop.cc | 8 +- src/tint/transform/loop_to_for_loop_test.cc | 52 ++++++++ .../transform/multiplanar_external_texture.cc | 4 +- .../transform/promote_side_effects_to_decl.cc | 15 --- .../transform/unwind_discard_functions.cc | 31 ----- .../transform/utils/hoist_to_decl_before.cc | 116 +++++------------- .../utils/hoist_to_decl_before_test.cc | 13 +- src/tint/writer/glsl/generator_impl.cc | 32 ++--- .../writer/glsl/generator_impl_binary_test.cc | 7 +- .../writer/glsl/generator_impl_if_test.cc | 14 +-- src/tint/writer/hlsl/generator_impl.cc | 32 ++--- .../writer/hlsl/generator_impl_binary_test.cc | 7 +- .../writer/hlsl/generator_impl_if_test.cc | 14 +-- src/tint/writer/msl/generator_impl.cc | 32 ++--- src/tint/writer/msl/generator_impl_if_test.cc | 8 +- src/tint/writer/spirv/builder.cc | 32 +++-- src/tint/writer/spirv/builder.h | 6 +- src/tint/writer/spirv/builder_if_test.cc | 22 ++-- src/tint/writer/spirv/builder_loop_test.cc | 26 ++-- src/tint/writer/wgsl/generator_impl.cc | 27 ++-- .../writer/wgsl/generator_impl_if_test.cc | 14 +-- test/tint/BUILD.gn | 2 - 53 files changed, 392 insertions(+), 940 deletions(-) delete mode 100644 src/tint/ast/else_statement.cc delete mode 100644 src/tint/ast/else_statement.h delete mode 100644 src/tint/ast/else_statement_test.cc delete mode 100644 src/tint/reader/wgsl/parser_impl_elseif_stmt_test.cc diff --git a/docs/tint/compound_statements.md b/docs/tint/compound_statements.md index a113cceae1..1d75415e14 100644 --- a/docs/tint/compound_statements.md +++ b/docs/tint/compound_statements.md @@ -24,13 +24,11 @@ sem::IfStatement { sem::BlockStatement { statement_a } - sem::ElseStatement { + sem::IfStatement { condition_b sem::BlockStatement { statement_b } - } - sem::ElseStatement { sem::BlockStatement { statement_c } diff --git a/src/tint/BUILD.gn b/src/tint/BUILD.gn index f2a1ce3aa9..691b435abd 100644 --- a/src/tint/BUILD.gn +++ b/src/tint/BUILD.gn @@ -222,8 +222,6 @@ libtint_source_set("libtint_core_all_src") { "ast/disable_validation_attribute.h", "ast/discard_statement.cc", "ast/discard_statement.h", - "ast/else_statement.cc", - "ast/else_statement.h", "ast/enable.cc", "ast/enable.h", "ast/expression.cc", diff --git a/src/tint/CMakeLists.txt b/src/tint/CMakeLists.txt index 35b5f6926b..ac9723f585 100644 --- a/src/tint/CMakeLists.txt +++ b/src/tint/CMakeLists.txt @@ -110,8 +110,6 @@ set(TINT_LIB_SRCS ast/depth_texture.h ast/discard_statement.cc ast/discard_statement.h - ast/else_statement.cc - ast/else_statement.h ast/enable.cc ast/enable.h ast/expression.cc @@ -676,7 +674,6 @@ if(TINT_BUILD_TESTS) ast/depth_multisampled_texture_test.cc ast/depth_texture_test.cc ast/discard_statement_test.cc - ast/else_statement_test.cc ast/enable_test.cc ast/external_texture_test.cc ast/f32_test.cc @@ -897,7 +894,6 @@ if(TINT_BUILD_TESTS) reader/wgsl/parser_impl_depth_texture_test.cc reader/wgsl/parser_impl_enable_directive_test.cc reader/wgsl/parser_impl_external_texture_test.cc - reader/wgsl/parser_impl_elseif_stmt_test.cc reader/wgsl/parser_impl_equality_expression_test.cc reader/wgsl/parser_impl_error_msg_test.cc reader/wgsl/parser_impl_error_resync_test.cc diff --git a/src/tint/ast/else_statement.cc b/src/tint/ast/else_statement.cc deleted file mode 100644 index d047177a7f..0000000000 --- a/src/tint/ast/else_statement.cc +++ /dev/null @@ -1,45 +0,0 @@ -// Copyright 2020 The Tint Authors. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -#include "src/tint/ast/else_statement.h" - -#include "src/tint/program_builder.h" - -TINT_INSTANTIATE_TYPEINFO(tint::ast::ElseStatement); - -namespace tint::ast { - -ElseStatement::ElseStatement(ProgramID pid, - const Source& src, - const Expression* cond, - const BlockStatement* b) - : Base(pid, src), condition(cond), body(b) { - TINT_ASSERT(AST, body); - TINT_ASSERT_PROGRAM_IDS_EQUAL_IF_VALID(AST, body, program_id); - TINT_ASSERT_PROGRAM_IDS_EQUAL_IF_VALID(AST, condition, program_id); -} - -ElseStatement::ElseStatement(ElseStatement&&) = default; - -ElseStatement::~ElseStatement() = default; - -const ElseStatement* ElseStatement::Clone(CloneContext* ctx) const { - // Clone arguments outside of create() call to have deterministic ordering - auto src = ctx->Clone(source); - auto* cond = ctx->Clone(condition); - auto* b = ctx->Clone(body); - return ctx->dst->create(src, cond, b); -} - -} // namespace tint::ast diff --git a/src/tint/ast/else_statement.h b/src/tint/ast/else_statement.h deleted file mode 100644 index c8c6e51fda..0000000000 --- a/src/tint/ast/else_statement.h +++ /dev/null @@ -1,59 +0,0 @@ -// Copyright 2020 The Tint Authors. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -#ifndef SRC_TINT_AST_ELSE_STATEMENT_H_ -#define SRC_TINT_AST_ELSE_STATEMENT_H_ - -#include - -#include "src/tint/ast/block_statement.h" -#include "src/tint/ast/expression.h" - -namespace tint::ast { - -/// An else statement -class ElseStatement final : public Castable { - public: - /// Constructor - /// @param pid the identifier of the program that owns this node - /// @param src the source of this node - /// @param condition the else condition - /// @param body the else body - ElseStatement(ProgramID pid, - const Source& src, - const Expression* condition, - const BlockStatement* body); - /// Move constructor - ElseStatement(ElseStatement&&); - ~ElseStatement() override; - - /// Clones this node and all transitive child nodes using the `CloneContext` - /// `ctx`. - /// @param ctx the clone context - /// @return the newly cloned node - const ElseStatement* Clone(CloneContext* ctx) const override; - - /// The else condition or nullptr if none set - const Expression* const condition; - - /// The else body - const BlockStatement* const body; -}; - -/// A list of else statements -using ElseStatementList = std::vector; - -} // namespace tint::ast - -#endif // SRC_TINT_AST_ELSE_STATEMENT_H_ diff --git a/src/tint/ast/else_statement_test.cc b/src/tint/ast/else_statement_test.cc deleted file mode 100644 index a088438822..0000000000 --- a/src/tint/ast/else_statement_test.cc +++ /dev/null @@ -1,92 +0,0 @@ -// Copyright 2020 The Tint Authors. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -#include "gtest/gtest-spi.h" -#include "src/tint/ast/discard_statement.h" -#include "src/tint/ast/if_statement.h" -#include "src/tint/ast/test_helper.h" - -namespace tint::ast { -namespace { - -using ElseStatementTest = TestHelper; - -TEST_F(ElseStatementTest, Creation) { - auto* cond = Expr(true); - auto* body = create(StatementList{ - create(), - }); - auto* discard = body->statements[0]; - - auto* e = create(cond, body); - EXPECT_EQ(e->condition, cond); - ASSERT_EQ(e->body->statements.size(), 1u); - EXPECT_EQ(e->body->statements[0], discard); -} - -TEST_F(ElseStatementTest, Creation_WithSource) { - auto* e = create(Source{Source::Location{20, 2}}, Expr(true), - Block()); - auto src = e->source; - EXPECT_EQ(src.range.begin.line, 20u); - EXPECT_EQ(src.range.begin.column, 2u); -} - -TEST_F(ElseStatementTest, IsElse) { - auto* e = create(nullptr, Block()); - EXPECT_TRUE(e->Is()); -} - -TEST_F(ElseStatementTest, HasCondition) { - auto* cond = Expr(true); - auto* e = create(cond, Block()); - EXPECT_TRUE(e->condition); -} - -TEST_F(ElseStatementTest, HasContition_NullCondition) { - auto* e = create(nullptr, Block()); - EXPECT_FALSE(e->condition); -} - -TEST_F(ElseStatementTest, Assert_Null_Body) { - EXPECT_FATAL_FAILURE( - { - ProgramBuilder b; - b.create(b.Expr(true), nullptr); - }, - "internal compiler error"); -} - -TEST_F(ElseStatementTest, Assert_DifferentProgramID_Condition) { - EXPECT_FATAL_FAILURE( - { - ProgramBuilder b1; - ProgramBuilder b2; - b1.create(b2.Expr(true), b1.Block()); - }, - "internal compiler error"); -} - -TEST_F(ElseStatementTest, Assert_DifferentProgramID_Body) { - EXPECT_FATAL_FAILURE( - { - ProgramBuilder b1; - ProgramBuilder b2; - b1.create(b1.Expr(true), b2.Block()); - }, - "internal compiler error"); -} - -} // namespace -} // namespace tint::ast diff --git a/src/tint/ast/if_statement.cc b/src/tint/ast/if_statement.cc index 4b78da3252..0c693d4049 100644 --- a/src/tint/ast/if_statement.cc +++ b/src/tint/ast/if_statement.cc @@ -24,18 +24,17 @@ IfStatement::IfStatement(ProgramID pid, const Source& src, const Expression* cond, const BlockStatement* b, - ElseStatementList else_stmts) - : Base(pid, src), - condition(cond), - body(b), - else_statements(std::move(else_stmts)) { + const Statement* else_stmt) + : Base(pid, src), condition(cond), body(b), else_statement(else_stmt) { TINT_ASSERT(AST, condition); TINT_ASSERT_PROGRAM_IDS_EQUAL_IF_VALID(AST, condition, program_id); TINT_ASSERT(AST, body); TINT_ASSERT_PROGRAM_IDS_EQUAL_IF_VALID(AST, body, program_id); - for (auto* el : else_statements) { - TINT_ASSERT(AST, el); - TINT_ASSERT_PROGRAM_IDS_EQUAL_IF_VALID(AST, el, program_id); + if (else_statement) { + TINT_ASSERT_PROGRAM_IDS_EQUAL_IF_VALID(AST, else_statement, program_id); + TINT_ASSERT( + AST, + (else_statement->IsAnyOf())); } } @@ -48,7 +47,7 @@ const IfStatement* IfStatement::Clone(CloneContext* ctx) const { auto src = ctx->Clone(source); auto* cond = ctx->Clone(condition); auto* b = ctx->Clone(body); - auto el = ctx->Clone(else_statements); + auto el = ctx->Clone(else_statement); return ctx->dst->create(src, cond, b, el); } diff --git a/src/tint/ast/if_statement.h b/src/tint/ast/if_statement.h index df843f8727..4d98dba728 100644 --- a/src/tint/ast/if_statement.h +++ b/src/tint/ast/if_statement.h @@ -17,7 +17,8 @@ #include -#include "src/tint/ast/else_statement.h" +#include "src/tint/ast/block_statement.h" +#include "src/tint/ast/expression.h" namespace tint::ast { @@ -29,12 +30,12 @@ class IfStatement final : public Castable { /// @param src the source of this node /// @param condition the if condition /// @param body the if body - /// @param else_stmts the else statements + /// @param else_stmt the else statement, or nullptr IfStatement(ProgramID pid, const Source& src, const Expression* condition, const BlockStatement* body, - ElseStatementList else_stmts); + const Statement* else_stmt); /// Move constructor IfStatement(IfStatement&&); ~IfStatement() override; @@ -51,8 +52,8 @@ class IfStatement final : public Castable { /// The if body const BlockStatement* const body; - /// The else statements - const ElseStatementList else_statements; + /// The optional else statement, or nullptr + const Statement* else_statement; }; } // namespace tint::ast diff --git a/src/tint/ast/if_statement_test.cc b/src/tint/ast/if_statement_test.cc index 83f998ae15..abd11a1dfe 100644 --- a/src/tint/ast/if_statement_test.cc +++ b/src/tint/ast/if_statement_test.cc @@ -25,16 +25,15 @@ using IfStatementTest = TestHelper; TEST_F(IfStatementTest, Creation) { auto* cond = Expr("cond"); - auto* stmt = create(Source{Source::Location{20, 2}}, cond, - Block(create()), - ElseStatementList{}); + auto* stmt = If(Source{Source::Location{20, 2}}, cond, + Block(create())); auto src = stmt->source; EXPECT_EQ(src.range.begin.line, 20u); EXPECT_EQ(src.range.begin.column, 2u); } TEST_F(IfStatementTest, IsIf) { - auto* stmt = create(Expr(true), Block(), ElseStatementList{}); + auto* stmt = If(Expr(true), Block()); EXPECT_TRUE(stmt->Is()); } @@ -42,7 +41,7 @@ TEST_F(IfStatementTest, Assert_Null_Condition) { EXPECT_FATAL_FAILURE( { ProgramBuilder b; - b.create(nullptr, b.Block(), ElseStatementList{}); + b.If(nullptr, b.Block()); }, "internal compiler error"); } @@ -51,17 +50,16 @@ TEST_F(IfStatementTest, Assert_Null_Body) { EXPECT_FATAL_FAILURE( { ProgramBuilder b; - b.create(b.Expr(true), nullptr, ElseStatementList{}); + b.If(b.Expr(true), nullptr); }, "internal compiler error"); } -TEST_F(IfStatementTest, Assert_Null_ElseStatement) { +TEST_F(IfStatementTest, Assert_InvalidElse) { EXPECT_FATAL_FAILURE( { ProgramBuilder b; - auto* body = b.create(StatementList{}); - b.create(b.Expr(true), body, ElseStatementList{nullptr}); + b.If(b.Expr(true), b.Block(), b.CallStmt(b.Call("foo"))); }, "internal compiler error"); } @@ -71,7 +69,7 @@ TEST_F(IfStatementTest, Assert_DifferentProgramID_Cond) { { ProgramBuilder b1; ProgramBuilder b2; - b1.create(b2.Expr(true), b1.Block(), ElseStatementList{}); + b1.If(b2.Expr(true), b1.Block()); }, "internal compiler error"); } @@ -81,7 +79,7 @@ TEST_F(IfStatementTest, Assert_DifferentProgramID_Body) { { ProgramBuilder b1; ProgramBuilder b2; - b1.create(b1.Expr(true), b2.Block(), ElseStatementList{}); + b1.If(b1.Expr(true), b2.Block()); }, "internal compiler error"); } @@ -91,11 +89,7 @@ TEST_F(IfStatementTest, Assert_DifferentProgramID_ElseStatement) { { ProgramBuilder b1; ProgramBuilder b2; - b1.create( - b1.Expr(true), b1.Block(), - ElseStatementList{ - b2.create(b2.Expr("ident"), b2.Block()), - }); + b1.If(b1.Expr(true), b1.Block(), b2.If(b2.Expr("ident"), b2.Block())); }, "internal compiler error"); } diff --git a/src/tint/ast/statement.cc b/src/tint/ast/statement.cc index 0d8750dcef..f003d1d0ac 100644 --- a/src/tint/ast/statement.cc +++ b/src/tint/ast/statement.cc @@ -58,9 +58,6 @@ const char* Statement::Name() const { if (Is()) { return "discard statement"; } - if (Is()) { - return "else statement"; - } if (Is()) { return "fallthrough statement"; } diff --git a/src/tint/program_builder.h b/src/tint/program_builder.h index f64bade8f5..249a55cb3e 100644 --- a/src/tint/program_builder.h +++ b/src/tint/program_builder.h @@ -2190,56 +2190,34 @@ class ProgramBuilder { ast::StatementList{std::forward(statements)...}); } - /// Creates a ast::ElseStatement with input condition and body - /// @param condition the else condition expression - /// @param body the else body - /// @returns the else statement pointer - template - const ast::ElseStatement* Else(CONDITION&& condition, - const ast::BlockStatement* body) { - return create(Expr(std::forward(condition)), - body); - } - - /// Creates a ast::ElseStatement with no condition and body - /// @param body the else body - /// @returns the else statement pointer - const ast::ElseStatement* Else(const ast::BlockStatement* body) { - return create(nullptr, body); - } - /// Creates a ast::IfStatement with input condition, body, and optional - /// variadic else statements + /// else statement /// @param source the source information for the if statement /// @param condition the if statement condition expression /// @param body the if statement body - /// @param elseStatements optional variadic else statements + /// @param else_stmt optional else statement /// @returns the if statement pointer - template + template const ast::IfStatement* If(const Source& source, CONDITION&& condition, const ast::BlockStatement* body, - ELSE_STATEMENTS&&... elseStatements) { + const ast::Statement* else_stmt = nullptr) { return create( - source, Expr(std::forward(condition)), body, - ast::ElseStatementList{ - std::forward(elseStatements)...}); + source, Expr(std::forward(condition)), body, else_stmt); } /// Creates a ast::IfStatement with input condition, body, and optional - /// variadic else statements + /// else statement /// @param condition the if statement condition expression /// @param body the if statement body - /// @param elseStatements optional variadic else statements + /// @param else_stmt optional else statement /// @returns the if statement pointer - template + template const ast::IfStatement* If(CONDITION&& condition, const ast::BlockStatement* body, - ELSE_STATEMENTS&&... elseStatements) { - return create( - Expr(std::forward(condition)), body, - ast::ElseStatementList{ - std::forward(elseStatements)...}); + const ast::Statement* else_stmt = nullptr) { + return create(Expr(std::forward(condition)), + body, else_stmt); } /// Creates a ast::AssignmentStatement with input lhs and rhs expressions diff --git a/src/tint/reader/spirv/function.cc b/src/tint/reader/spirv/function.cc index e148050e4f..d3e4e5e92f 100644 --- a/src/tint/reader/spirv/function.cc +++ b/src/tint/reader/spirv/function.cc @@ -681,15 +681,15 @@ struct IfStatementBuilder final /// @param builder the program builder /// @returns the built ast::IfStatement const ast::IfStatement* Build(ProgramBuilder* builder) const override { - return builder->create(Source{}, cond, body, else_stmts); + return builder->create(Source{}, cond, body, else_stmt); } /// If-statement condition const ast::Expression* const cond; /// If-statement block body const ast::BlockStatement* body = nullptr; - /// Optional if-statement else statements - ast::ElseStatementList else_stmts; + /// Optional if-statement else statement + const ast::Statement* else_stmt = nullptr; }; /// A StatementBuilder for ast::LoopStatement @@ -2964,10 +2964,8 @@ bool FunctionEmitter::EmitIfStart(const BlockInfo& block_info) { // Only set the else-clause if there are statements to fill it. if (!stmts.empty()) { // The "else" consists of the statement list from the top of - // statements stack, without an elseif condition. - auto* else_body = create(Source{}, stmts); - builder->else_stmts.emplace_back( - create(Source{}, nullptr, else_body)); + // statements stack, without an "else if" condition. + builder->else_stmt = create(Source{}, stmts); } }); if (false_is_break) { @@ -3363,19 +3361,19 @@ const ast::Statement* FunctionEmitter::MakeSimpleIf( if ((then_stmt == nullptr) && (else_stmt == nullptr)) { return nullptr; } - ast::ElseStatementList else_stmts; - if (else_stmt != nullptr) { - ast::StatementList stmts{else_stmt}; - else_stmts.emplace_back(create( - Source{}, nullptr, create(Source{}, stmts))); - } ast::StatementList if_stmts; if (then_stmt != nullptr) { if_stmts.emplace_back(then_stmt); } auto* if_block = create(Source{}, if_stmts); + + const ast::Statement* else_block = nullptr; + if (else_stmt) { + else_block = create(ast::StatementList{else_stmt}); + } + auto* if_stmt = - create(Source{}, condition, if_block, else_stmts); + create(Source{}, condition, if_block, else_block); return if_stmt; } diff --git a/src/tint/reader/wgsl/parser_impl.cc b/src/tint/reader/wgsl/parser_impl.cc index d1a0dc2e43..2a93197354 100644 --- a/src/tint/reader/wgsl/parser_impl.cc +++ b/src/tint/reader/wgsl/parser_impl.cc @@ -1729,58 +1729,33 @@ Maybe ParserImpl::variable_stmt() { } // if_stmt -// : IF expression compound_stmt ( ELSE else_stmts ) ? -Maybe ParserImpl::if_stmt() { - Source source; - if (!match(Token::Type::kIf, &source)) - return Failure::kNoMatch; - - auto condition = logical_or_expression(); - if (condition.errored) - return Failure::kErrored; - if (!condition.matched) { - return add_error(peek(), "unable to parse condition expression"); - } - - auto body = expect_body_stmt(); - if (body.errored) - return Failure::kErrored; - - auto el = else_stmts(); - if (el.errored) { - return Failure::kErrored; - } - - return create(source, condition.value, body.value, - std::move(el.value)); -} - -// else_stmts +// : IF expression compound_stmt ( ELSE else_stmt ) ? +// else_stmt // : body_stmt // | if_stmt -Expect ParserImpl::else_stmts() { - ast::ElseStatementList stmts; - while (continue_parsing()) { - Source start; +Maybe ParserImpl::if_stmt() { + // Parse if-else chains iteratively instead of recursively, to avoid + // stack-overflow for long chains of if-else statements. - bool else_if = false; - if (match(Token::Type::kElse, &start)) { - else_if = match(Token::Type::kIf); - } else { - break; + struct IfInfo { + Source source; + const ast::Expression* condition; + const ast::BlockStatement* body; + }; + + // Parse an if statement, capturing the source, condition, and body statement. + auto parse_if = [&]() -> Maybe { + Source source; + if (!match(Token::Type::kIf, &source)) { + return Failure::kNoMatch; } - const ast::Expression* cond = nullptr; - if (else_if) { - auto condition = logical_or_expression(); - if (condition.errored) { - return Failure::kErrored; - } - if (!condition.matched) { - return add_error(peek(), "unable to parse condition expression"); - } - - cond = condition.value; + auto condition = logical_or_expression(); + if (condition.errored) { + return Failure::kErrored; + } + if (!condition.matched) { + return add_error(peek(), "unable to parse condition expression"); } auto body = expect_body_stmt(); @@ -1788,11 +1763,52 @@ Expect ParserImpl::else_stmts() { return Failure::kErrored; } - Source source = make_source_range_from(start); - stmts.emplace_back(create(source, cond, body.value)); + return IfInfo{source, condition.value, body.value}; + }; + + std::vector statements; + + // Parse the first if statement. + auto first_if = parse_if(); + if (first_if.errored) { + return Failure::kErrored; + } else if (!first_if.matched) { + return Failure::kNoMatch; + } + statements.push_back(first_if.value); + + // Parse the components of every "else {if}" in the chain. + const ast::Statement* last_stmt = nullptr; + while (continue_parsing()) { + if (!match(Token::Type::kElse)) { + break; + } + + // Try to parse an "else if". + auto else_if = parse_if(); + if (else_if.errored) { + return Failure::kErrored; + } else if (else_if.matched) { + statements.push_back(else_if.value); + continue; + } + + // If it wasn't an "else if", it must just be an "else". + auto else_body = expect_body_stmt(); + if (else_body.errored) { + return Failure::kErrored; + } + last_stmt = else_body.value; + break; } - return stmts; + // Now walk back through the statements to create their AST nodes. + for (auto itr = statements.rbegin(); itr != statements.rend(); itr++) { + last_stmt = create(itr->source, itr->condition, itr->body, + last_stmt); + } + + return last_stmt->As(); } // switch_stmt diff --git a/src/tint/reader/wgsl/parser_impl.h b/src/tint/reader/wgsl/parser_impl.h index 2f5b5e2338..929030048b 100644 --- a/src/tint/reader/wgsl/parser_impl.h +++ b/src/tint/reader/wgsl/parser_impl.h @@ -513,9 +513,6 @@ class ParserImpl { /// Parses a `if_stmt` grammar element /// @returns the parsed statement or nullptr Maybe if_stmt(); - /// Parses a list of `else_stmt` grammar elements - /// @returns the parsed statement or nullptr - Expect else_stmts(); /// Parses a `switch_stmt` grammar element /// @returns the parsed statement or nullptr Maybe switch_stmt(); diff --git a/src/tint/reader/wgsl/parser_impl_elseif_stmt_test.cc b/src/tint/reader/wgsl/parser_impl_elseif_stmt_test.cc deleted file mode 100644 index f0fb963a35..0000000000 --- a/src/tint/reader/wgsl/parser_impl_elseif_stmt_test.cc +++ /dev/null @@ -1,66 +0,0 @@ -// Copyright 2020 The Tint Authors. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -#include "src/tint/reader/wgsl/parser_impl_test_helper.h" - -namespace tint::reader::wgsl { -namespace { - -TEST_F(ParserImplTest, ElseStmts) { - auto p = parser("else if (a == 4) { a = b; c = d; }"); - auto e = p->else_stmts(); - EXPECT_FALSE(p->has_error()) << p->error(); - ASSERT_EQ(e.value.size(), 1u); - - ASSERT_TRUE(e.value[0]->Is()); - ASSERT_NE(e.value[0]->condition, nullptr); - ASSERT_TRUE(e.value[0]->condition->Is()); - EXPECT_EQ(e.value[0]->body->statements.size(), 2u); -} - -TEST_F(ParserImplTest, ElseStmts_Multiple) { - auto p = parser("else if (a == 4) { a = b; c = d; } else if(c) { d = 2; }"); - auto e = p->else_stmts(); - EXPECT_FALSE(p->has_error()) << p->error(); - ASSERT_EQ(e.value.size(), 2u); - - ASSERT_TRUE(e.value[0]->Is()); - ASSERT_NE(e.value[0]->condition, nullptr); - ASSERT_TRUE(e.value[0]->condition->Is()); - EXPECT_EQ(e.value[0]->body->statements.size(), 2u); - - ASSERT_TRUE(e.value[1]->Is()); - ASSERT_NE(e.value[1]->condition, nullptr); - ASSERT_TRUE(e.value[1]->condition->Is()); - EXPECT_EQ(e.value[1]->body->statements.size(), 1u); -} - -TEST_F(ParserImplTest, ElseStmts_InvalidBody) { - auto p = parser("else if (true) { fn main() {}}"); - auto e = p->else_stmts(); - EXPECT_TRUE(e.errored); - EXPECT_TRUE(p->has_error()); - EXPECT_EQ(p->error(), "1:18: expected '}'"); -} - -TEST_F(ParserImplTest, ElseStmts_MissingBody) { - auto p = parser("else if (true)"); - auto e = p->else_stmts(); - EXPECT_TRUE(e.errored); - EXPECT_TRUE(p->has_error()); - EXPECT_EQ(p->error(), "1:15: expected '{'"); -} - -} // namespace -} // namespace tint::reader::wgsl diff --git a/src/tint/reader/wgsl/parser_impl_if_stmt_test.cc b/src/tint/reader/wgsl/parser_impl_if_stmt_test.cc index 18c30e9453..1b81e0b293 100644 --- a/src/tint/reader/wgsl/parser_impl_if_stmt_test.cc +++ b/src/tint/reader/wgsl/parser_impl_if_stmt_test.cc @@ -29,7 +29,7 @@ TEST_F(ParserImplTest, IfStmt) { ASSERT_NE(e->condition, nullptr); ASSERT_TRUE(e->condition->Is()); EXPECT_EQ(e->body->statements.size(), 2u); - EXPECT_EQ(e->else_statements.size(), 0u); + EXPECT_EQ(e->else_statement, nullptr); } TEST_F(ParserImplTest, IfStmt_WithElse) { @@ -45,14 +45,14 @@ TEST_F(ParserImplTest, IfStmt_WithElse) { ASSERT_TRUE(e->condition->Is()); EXPECT_EQ(e->body->statements.size(), 2u); - ASSERT_EQ(e->else_statements.size(), 2u); - ASSERT_NE(e->else_statements[0]->condition, nullptr); - ASSERT_TRUE( - e->else_statements[0]->condition->Is()); - EXPECT_EQ(e->else_statements[0]->body->statements.size(), 1u); + auto* elseif = As(e->else_statement); + ASSERT_NE(elseif, nullptr); + ASSERT_TRUE(elseif->condition->Is()); + EXPECT_EQ(elseif->body->statements.size(), 1u); - ASSERT_EQ(e->else_statements[1]->condition, nullptr); - EXPECT_EQ(e->else_statements[1]->body->statements.size(), 0u); + auto* el = As(elseif->else_statement); + ASSERT_NE(el, nullptr); + EXPECT_EQ(el->statements.size(), 0u); } TEST_F(ParserImplTest, IfStmt_WithElse_WithParens) { @@ -68,14 +68,14 @@ TEST_F(ParserImplTest, IfStmt_WithElse_WithParens) { ASSERT_TRUE(e->condition->Is()); EXPECT_EQ(e->body->statements.size(), 2u); - ASSERT_EQ(e->else_statements.size(), 2u); - ASSERT_NE(e->else_statements[0]->condition, nullptr); - ASSERT_TRUE( - e->else_statements[0]->condition->Is()); - EXPECT_EQ(e->else_statements[0]->body->statements.size(), 1u); + auto* elseif = As(e->else_statement); + ASSERT_NE(elseif, nullptr); + ASSERT_TRUE(elseif->condition->Is()); + EXPECT_EQ(elseif->body->statements.size(), 1u); - ASSERT_EQ(e->else_statements[1]->condition, nullptr); - EXPECT_EQ(e->else_statements[1]->body->statements.size(), 0u); + auto* el = As(elseif->else_statement); + ASSERT_NE(el, nullptr); + EXPECT_EQ(el->statements.size(), 0u); } TEST_F(ParserImplTest, IfStmt_InvalidCondition) { diff --git a/src/tint/resolver/compound_statement_test.cc b/src/tint/resolver/compound_statement_test.cc index 29c0384b33..f15bf28210 100644 --- a/src/tint/resolver/compound_statement_test.cc +++ b/src/tint/resolver/compound_statement_test.cc @@ -232,8 +232,8 @@ TEST_F(ResolverCompoundStatementTest, If) { auto* cond_b = Expr(true); auto* stmt_b = Ignore(1); auto* stmt_c = Ignore(1); - auto* if_stmt = If(cond_a, Block(stmt_a), Else(cond_b, Block(stmt_b)), - Else(nullptr, Block(stmt_c))); + auto* if_stmt = + If(cond_a, Block(stmt_a), If(cond_b, Block(stmt_b), Block(stmt_c))); WrapInFunction(if_stmt); ASSERT_TRUE(r()->Resolve()) << r()->error(); @@ -268,8 +268,8 @@ TEST_F(ResolverCompoundStatementTest, If) { ASSERT_NE(e, nullptr); auto* s = e->Stmt(); ASSERT_NE(s, nullptr); - EXPECT_TRUE(s->Is()); - EXPECT_EQ(s->Parent(), s->FindFirstParent()); + EXPECT_TRUE(s->Is()); + EXPECT_EQ(s->Parent(), s->Parent()->FindFirstParent()); EXPECT_EQ(s->Parent()->Parent(), s->FindFirstParent()); EXPECT_EQ(s->Parent()->Parent(), s->Block()); @@ -279,9 +279,10 @@ TEST_F(ResolverCompoundStatementTest, If) { ASSERT_NE(s, nullptr); EXPECT_EQ(s->Parent(), s->FindFirstParent()); EXPECT_EQ(s->Parent(), s->Block()); - EXPECT_EQ(s->Parent()->Parent(), s->FindFirstParent()); + auto* elseif = s->FindFirstParent(); + EXPECT_EQ(s->Parent()->Parent(), elseif); EXPECT_EQ(s->Parent()->Parent()->Parent(), - s->FindFirstParent()); + elseif->Parent()->FindFirstParent()); EXPECT_EQ(s->Parent()->Parent()->Parent()->Parent(), s->FindFirstParent()); } @@ -290,9 +291,10 @@ TEST_F(ResolverCompoundStatementTest, If) { ASSERT_NE(s, nullptr); EXPECT_EQ(s->Parent(), s->FindFirstParent()); EXPECT_EQ(s->Parent(), s->Block()); - EXPECT_EQ(s->Parent()->Parent(), s->FindFirstParent()); + auto* elseif = s->FindFirstParent(); + EXPECT_EQ(s->Parent()->Parent(), elseif); EXPECT_EQ(s->Parent()->Parent()->Parent(), - s->FindFirstParent()); + elseif->Parent()->FindFirstParent()); EXPECT_EQ(s->Parent()->Parent()->Parent()->Parent(), s->FindFirstParent()); } diff --git a/src/tint/resolver/dependency_graph.cc b/src/tint/resolver/dependency_graph.cc index 37496fdd88..3d694727e8 100644 --- a/src/tint/resolver/dependency_graph.cc +++ b/src/tint/resolver/dependency_graph.cc @@ -248,9 +248,8 @@ class DependencyScanner { [&](const ast::IfStatement* i) { TraverseExpression(i->condition); TraverseStatement(i->body); - for (auto* e : i->else_statements) { - TraverseExpression(e->condition); - TraverseStatement(e->body); + if (i->else_statement) { + TraverseStatement(i->else_statement); } }, [&](const ast::ReturnStatement* r) { // diff --git a/src/tint/resolver/dependency_graph_test.cc b/src/tint/resolver/dependency_graph_test.cc index 64ae95d197..639991a8b0 100644 --- a/src/tint/resolver/dependency_graph_test.cc +++ b/src/tint/resolver/dependency_graph_test.cc @@ -1259,8 +1259,8 @@ TEST_F(ResolverDependencyGraphTraversalTest, SymbolsReached) { Assign(V, V)), // If(V, // Block(Assign(V, V)), // - Else(V, // - Block(Assign(V, V)))), // + If(V, // + Block(Assign(V, V)))), // Ignore(Bitcast(T, V)), // For(Decl(Var(Sym(), T, V)), // Equal(V, V), // diff --git a/src/tint/resolver/resolver.cc b/src/tint/resolver/resolver.cc index 483b9a9256..d01fc2cc4a 100644 --- a/src/tint/resolver/resolver.cc +++ b/src/tint/resolver/resolver.cc @@ -892,13 +892,6 @@ sem::Statement* Resolver::Statement(const ast::Statement* stmt) { stmt->source); return nullptr; }, - [&](const ast::ElseStatement*) { - TINT_ICE(Resolver, diagnostics_) - << "Resolver::Statement() encountered an Else statement. Else " - "statements are embedded in If statements, so should never be " - "encountered as top-level statements"; - return nullptr; - }, [&](Default) { AddError( "unknown statement type: " + std::string(stmt->TypeInfo().name), @@ -946,17 +939,14 @@ sem::IfStatement* Resolver::IfStatement(const ast::IfStatement* stmt) { } sem->Behaviors().Add(body->Behaviors()); - for (auto* else_stmt : stmt->else_statements) { - Mark(else_stmt); - auto* else_sem = ElseStatement(else_stmt); + if (stmt->else_statement) { + Mark(stmt->else_statement); + auto* else_sem = Statement(stmt->else_statement); if (!else_sem) { return false; } sem->Behaviors().Add(else_sem->Behaviors()); - } - - if (stmt->else_statements.empty() || - stmt->else_statements.back()->condition != nullptr) { + } else { // https://www.w3.org/TR/WGSL/#behaviors-rules // if statements without an else branch are treated as if they had an // empty else branch (which adds Next to their behavior) @@ -967,37 +957,6 @@ sem::IfStatement* Resolver::IfStatement(const ast::IfStatement* stmt) { }); } -sem::ElseStatement* Resolver::ElseStatement(const ast::ElseStatement* stmt) { - auto* sem = builder_->create( - stmt, current_compound_statement_->As(), - current_function_); - return StatementScope(stmt, sem, [&] { - if (auto* cond_expr = stmt->condition) { - auto* cond = Expression(cond_expr); - if (!cond) { - return false; - } - sem->SetCondition(cond); - // https://www.w3.org/TR/WGSL/#behaviors-rules - // if statements with else if branches are treated as if they were nested - // simple if/else statements - sem->Behaviors() = cond->Behaviors(); - } - sem->Behaviors().Remove(sem::Behavior::kNext); - - Mark(stmt->body); - auto* body = builder_->create( - stmt->body, current_compound_statement_, current_function_); - if (!StatementScope(stmt->body, body, - [&] { return Statements(stmt->body->statements); })) { - return false; - } - sem->Behaviors().Add(body->Behaviors()); - - return validator_.ElseStatement(sem); - }); -} - sem::BlockStatement* Resolver::BlockStatement(const ast::BlockStatement* stmt) { auto* sem = builder_->create( stmt->As(), current_compound_statement_, diff --git a/src/tint/resolver/resolver.h b/src/tint/resolver/resolver.h index 22e5d5efc8..740ea319c9 100644 --- a/src/tint/resolver/resolver.h +++ b/src/tint/resolver/resolver.h @@ -59,7 +59,6 @@ class Atomic; class BlockStatement; class Builtin; class CaseStatement; -class ElseStatement; class ForLoopStatement; class IfStatement; class LoopStatement; @@ -222,7 +221,6 @@ class Resolver { const ast::CompoundAssignmentStatement*); sem::Statement* ContinueStatement(const ast::ContinueStatement*); sem::Statement* DiscardStatement(const ast::DiscardStatement*); - sem::ElseStatement* ElseStatement(const ast::ElseStatement*); sem::Statement* FallthroughStatement(const ast::FallthroughStatement*); sem::ForLoopStatement* ForLoopStatement(const ast::ForLoopStatement*); sem::GlobalVariable* GlobalVariable(const ast::Variable*); diff --git a/src/tint/resolver/resolver_behavior_test.cc b/src/tint/resolver/resolver_behavior_test.cc index fe855bfa58..7fcf34864e 100644 --- a/src/tint/resolver/resolver_behavior_test.cc +++ b/src/tint/resolver/resolver_behavior_test.cc @@ -348,7 +348,7 @@ TEST_F(ResolverBehaviorTest, StmtIfTrue_ThenDiscard) { } TEST_F(ResolverBehaviorTest, StmtIfTrue_ThenEmptyBlock_ElseDiscard) { - auto* stmt = If(true, Block(), Else(Block(Discard()))); + auto* stmt = If(true, Block(), Block(Discard())); WrapInFunction(stmt); ASSERT_TRUE(r()->Resolve()) << r()->error(); @@ -359,7 +359,7 @@ TEST_F(ResolverBehaviorTest, StmtIfTrue_ThenEmptyBlock_ElseDiscard) { } TEST_F(ResolverBehaviorTest, StmtIfTrue_ThenDiscard_ElseDiscard) { - auto* stmt = If(true, Block(Discard()), Else(Block(Discard()))); + auto* stmt = If(true, Block(Discard()), Block(Discard())); WrapInFunction(stmt); ASSERT_TRUE(r()->Resolve()) << r()->error(); @@ -381,7 +381,7 @@ TEST_F(ResolverBehaviorTest, StmtIfCallFuncMayDiscard_ThenEmptyBlock) { TEST_F(ResolverBehaviorTest, StmtIfTrue_ThenEmptyBlock_ElseCallFuncMayDiscard) { auto* stmt = If(true, Block(), // - Else(Equal(Call("DiscardOrNext"), 1), Block())); + If(Equal(Call("DiscardOrNext"), 1), Block())); WrapInFunction(stmt); ASSERT_TRUE(r()->Resolve()) << r()->error(); diff --git a/src/tint/resolver/resolver_test.cc b/src/tint/resolver/resolver_test.cc index ef4d927098..c019e04aea 100644 --- a/src/tint/resolver/resolver_test.cc +++ b/src/tint/resolver/resolver_test.cc @@ -160,7 +160,7 @@ TEST_F(ResolverTest, Stmt_If) { auto* else_body = Block(Assign(else_lhs, else_rhs)); auto* else_cond = Expr(true); - auto* else_stmt = create(else_cond, else_body); + auto* else_stmt = If(else_cond, else_body); auto* lhs = Expr("v"); auto* rhs = Expr(2.3f); @@ -168,8 +168,7 @@ TEST_F(ResolverTest, Stmt_If) { auto* assign = Assign(lhs, rhs); auto* body = Block(assign); auto* cond = Expr(true); - auto* stmt = - create(cond, body, ast::ElseStatementList{else_stmt}); + auto* stmt = If(cond, body, else_stmt); WrapInFunction(v, stmt); EXPECT_TRUE(r()->Resolve()) << r()->error(); diff --git a/src/tint/resolver/type_validation_test.cc b/src/tint/resolver/type_validation_test.cc index 45b879355d..1d1ac38eb5 100644 --- a/src/tint/resolver/type_validation_test.cc +++ b/src/tint/resolver/type_validation_test.cc @@ -149,9 +149,7 @@ TEST_F(ResolverTypeValidationTest, RedeclaredIdentifierInnerScope_Pass) { auto* var_a_float = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(3.1f)); - auto* outer_body = - Block(create(cond, body, ast::ElseStatementList{}), - Decl(Source{{12, 34}}, var_a_float)); + auto* outer_body = Block(If(cond, body), Decl(Source{{12, 34}}, var_a_float)); WrapInFunction(outer_body); diff --git a/src/tint/resolver/validation_test.cc b/src/tint/resolver/validation_test.cc index 15f9418134..045adf5c84 100644 --- a/src/tint/resolver/validation_test.cc +++ b/src/tint/resolver/validation_test.cc @@ -124,16 +124,16 @@ TEST_F(ResolverValidationTest, Stmt_If_NonBool) { "12:34 error: if statement condition must be bool, got f32"); } -TEST_F(ResolverValidationTest, Stmt_Else_NonBool) { - // else (1.23f) {} +TEST_F(ResolverValidationTest, Stmt_ElseIf_NonBool) { + // else if (1.23f) {} WrapInFunction( - If(Expr(true), Block(), Else(Expr(Source{{12, 34}}, 1.23f), Block()))); + If(Expr(true), Block(), If(Expr(Source{{12, 34}}, 1.23f), Block()))); EXPECT_FALSE(r()->Resolve()); EXPECT_EQ(r()->error(), - "12:34 error: else statement condition must be bool, got f32"); + "12:34 error: if statement condition must be bool, got f32"); } TEST_F(ResolverValidationTest, Expr_ErrUnknownExprType) { @@ -242,9 +242,7 @@ TEST_F(ResolverValidationTest, UsingUndefinedVariableInnerScope_Fail) { auto* lhs = Expr(Source{{12, 34}}, "a"); auto* rhs = Expr(3.14f); - auto* outer_body = - Block(create(cond, body, ast::ElseStatementList{}), - Assign(lhs, rhs)); + auto* outer_body = Block(If(cond, body), Assign(lhs, rhs)); WrapInFunction(outer_body); @@ -265,9 +263,7 @@ TEST_F(ResolverValidationTest, UsingUndefinedVariableOuterScope_Pass) { auto* cond = Expr(true); auto* body = Block(Assign(lhs, rhs)); - auto* outer_body = - Block(Decl(var), - create(cond, body, ast::ElseStatementList{})); + auto* outer_body = Block(Decl(var), If(cond, body)); WrapInFunction(outer_body); @@ -1053,12 +1049,12 @@ TEST_F(ResolverValidationTest, Stmt_BreakInIfTrueInContinuing) { } TEST_F(ResolverValidationTest, Stmt_BreakInIfElseInContinuing) { - auto* cont = Block( // continuing { - If(true, Block(), // if(true) { - Else(Block( // } else { - Break(Source{{12, 34}}))))); // break; - // } - // } + auto* cont = Block( // continuing { + If(true, Block(), // if(true) { + Block( // } else { + Break(Source{{12, 34}})))); // break; + // } + // } WrapInFunction(Loop(Block(), cont)); EXPECT_TRUE(r()->Resolve()) << r()->error(); } @@ -1114,13 +1110,13 @@ TEST_F(ResolverValidationTest, Stmt_BreakInIfTrueMultipleStmtsInContinuing) { } TEST_F(ResolverValidationTest, Stmt_BreakInIfElseMultipleStmtsInContinuing) { - auto* cont = Block( // continuing { - If(true, Block(), // if(true) { - Else(Block(Source{{56, 78}}, // } else { - Assign(Phony(), 1), // _ = 1; - Break(Source{{12, 34}}))))); // break; - // } - // } + auto* cont = Block( // continuing { + If(true, Block(), // if(true) { + Block(Source{{56, 78}}, // } else { + Assign(Phony(), 1), // _ = 1; + Break(Source{{12, 34}})))); // break; + // } + // } WrapInFunction(Loop(Block(), cont)); EXPECT_FALSE(r()->Resolve()); EXPECT_EQ( @@ -1132,12 +1128,12 @@ TEST_F(ResolverValidationTest, Stmt_BreakInIfElseMultipleStmtsInContinuing) { } TEST_F(ResolverValidationTest, Stmt_BreakInIfElseIfInContinuing) { - auto* cont = Block( // continuing { - If(true, Block(), // if(true) { - Else(Expr(Source{{56, 78}}, true), // } else if (true) { - Block(Break(Source{{12, 34}}))))); // break; - // } - // } + auto* cont = Block( // continuing { + If(true, Block(), // if(true) { + If(Source{{56, 78}}, Expr(true), // } else if (true) { + Block(Break(Source{{12, 34}}))))); // break; + // } + // } WrapInFunction(Loop(Block(), cont)); EXPECT_FALSE(r()->Resolve()); EXPECT_EQ( @@ -1149,13 +1145,13 @@ TEST_F(ResolverValidationTest, Stmt_BreakInIfElseIfInContinuing) { } TEST_F(ResolverValidationTest, Stmt_BreakInIfNonEmptyElseInContinuing) { - auto* cont = Block( // continuing { - If(true, // if(true) { - Block(Break(Source{{12, 34}})), // break; - Else(Block(Source{{56, 78}}, // } else { - Assign(Phony(), 1))))); // _ = 1; - // } - // } + auto* cont = Block( // continuing { + If(true, // if(true) { + Block(Break(Source{{12, 34}})), // break; + Block(Source{{56, 78}}, // } else { + Assign(Phony(), 1)))); // _ = 1; + // } + // } WrapInFunction(Loop(Block(), cont)); EXPECT_FALSE(r()->Resolve()); EXPECT_EQ( @@ -1170,8 +1166,8 @@ TEST_F(ResolverValidationTest, Stmt_BreakInIfElseNonEmptyTrueInContinuing) { auto* cont = Block( // continuing { If(true, // if(true) { Block(Source{{56, 78}}, Assign(Phony(), 1)), // _ = 1; - Else(Block( // } else { - Break(Source{{12, 34}}))))); // break; + Block( // } else { + Break(Source{{12, 34}})))); // break; // } // } WrapInFunction(Loop(Block(), cont)); diff --git a/src/tint/resolver/validator.cc b/src/tint/resolver/validator.cc index 22ecb976e7..87a374f8f2 100644 --- a/src/tint/resolver/validator.cc +++ b/src/tint/resolver/validator.cc @@ -1383,10 +1383,6 @@ bool Validator::BreakStatement(const sem::Statement* stmt, if (auto* block = stmt->Parent()->As()) { auto* block_parent = block->Parent(); auto* if_stmt = block_parent->As(); - auto* el_stmt = block_parent->As(); - if (el_stmt) { - if_stmt = el_stmt->Parent(); - } if (!if_stmt) { return fail("break statement is not directly in if statement block", stmt->Declaration()->source); @@ -1395,22 +1391,25 @@ bool Validator::BreakStatement(const sem::Statement* stmt, return fail("if statement block contains multiple statements", block->Declaration()->source); } - for (auto* el : if_stmt->Declaration()->else_statements) { - if (el->condition) { - return fail("else has condition", el->condition->source); + + if (if_stmt->Parent()->Is()) { + return fail("else has condition", if_stmt->Declaration()->source); + } + + bool el_contains_break = + block->Declaration() == if_stmt->Declaration()->else_statement; + if (el_contains_break) { + if (auto* true_block = if_stmt->Declaration()->body; + !true_block->Empty()) { + return fail("non-empty true block", true_block->source); } - bool el_contains_break = el_stmt && el == el_stmt->Declaration(); - if (el_contains_break) { - if (auto* true_block = if_stmt->Declaration()->body; - !true_block->Empty()) { - return fail("non-empty true block", true_block->source); - } - } else { - if (!el->body->Empty()) { - return fail("non-empty false block", el->body->source); - } + } else { + auto* else_stmt = if_stmt->Declaration()->else_statement; + if (else_stmt) { + return fail("non-empty false block", else_stmt->source); } } + if (if_stmt->Parent()->Declaration() != continuing) { return fail( "if statement containing break statement is not directly in " @@ -1490,19 +1489,6 @@ bool Validator::FallthroughStatement(const sem::Statement* stmt) const { return false; } -bool Validator::ElseStatement(const sem::ElseStatement* stmt) const { - if (auto* cond = stmt->Condition()) { - auto* cond_ty = cond->Type()->UnwrapRef(); - if (!cond_ty->Is()) { - AddError("else statement condition must be bool, got " + - sem_.TypeNameOf(cond_ty), - stmt->Condition()->Declaration()->source); - return false; - } - } - return true; -} - bool Validator::LoopStatement(const sem::LoopStatement* stmt) const { if (stmt->Behaviors().Empty()) { AddError("loop does not exit", stmt->Declaration()->source.Begin()); diff --git a/src/tint/resolver/validator.h b/src/tint/resolver/validator.h index 146f7ba052..77e5172a1b 100644 --- a/src/tint/resolver/validator.h +++ b/src/tint/resolver/validator.h @@ -51,7 +51,6 @@ class Atomic; class BlockStatement; class Builtin; class CaseStatement; -class ElseStatement; class ForLoopStatement; class IfStatement; class LoopStatement; @@ -194,11 +193,6 @@ class Validator { bool DiscardStatement(const sem::Statement* stmt, sem::Statement* current_statement) const; - /// Validates an else statement - /// @param stmt the else statement to validate - /// @returns true on success, false otherwise - bool ElseStatement(const sem::ElseStatement* stmt) const; - /// Validates an entry point /// @param func the entry point function to validate /// @param stage the pipeline stage for the entry point diff --git a/src/tint/resolver/var_let_validation_test.cc b/src/tint/resolver/var_let_validation_test.cc index fca81cff2d..be41da8617 100644 --- a/src/tint/resolver/var_let_validation_test.cc +++ b/src/tint/resolver/var_let_validation_test.cc @@ -205,9 +205,7 @@ TEST_F(ResolverVarLetValidationTest, VarRedeclaredInIfBlock) { auto* cond = Expr(true); auto* body = Block(Decl(var)); - auto* outer_body = - Block(Decl(var_a_float), - create(cond, body, ast::ElseStatementList{})); + auto* outer_body = Block(Decl(var_a_float), If(cond, body)); WrapInFunction(outer_body); diff --git a/src/tint/sem/if_statement.cc b/src/tint/sem/if_statement.cc index 5102b898c2..a600d2147c 100644 --- a/src/tint/sem/if_statement.cc +++ b/src/tint/sem/if_statement.cc @@ -17,7 +17,6 @@ #include "src/tint/program_builder.h" TINT_INSTANTIATE_TYPEINFO(tint::sem::IfStatement); -TINT_INSTANTIATE_TYPEINFO(tint::sem::ElseStatement); namespace tint::sem { @@ -32,15 +31,4 @@ const ast::IfStatement* IfStatement::Declaration() const { return static_cast(Base::Declaration()); } -ElseStatement::ElseStatement(const ast::ElseStatement* declaration, - const IfStatement* parent, - const sem::Function* function) - : Base(declaration, parent, function) {} - -ElseStatement::~ElseStatement() = default; - -const ast::ElseStatement* ElseStatement::Declaration() const { - return static_cast(Base::Declaration()); -} - } // namespace tint::sem diff --git a/src/tint/sem/if_statement.h b/src/tint/sem/if_statement.h index 8e4fb27621..112bcd1329 100644 --- a/src/tint/sem/if_statement.h +++ b/src/tint/sem/if_statement.h @@ -20,7 +20,6 @@ // Forward declarations namespace tint::ast { class IfStatement; -class ElseStatement; } // namespace tint::ast namespace tint::sem { class Expression; @@ -56,39 +55,6 @@ class IfStatement final : public Castable { const Expression* condition_ = nullptr; }; -/// Holds semantic information about an else statement -class ElseStatement final : public Castable { - public: - /// Constructor - /// @param declaration the AST node for this else statement - /// @param parent the owning statement - /// @param function the owning function - ElseStatement(const ast::ElseStatement* declaration, - const IfStatement* parent, - const sem::Function* function); - - /// Destructor - ~ElseStatement() override; - - /// @returns the AST node - const ast::ElseStatement* Declaration() const; - - /// @returns the else-statement condition expression - const Expression* Condition() const { return condition_; } - - /// @return the statement that encloses this statement - const IfStatement* Parent() const { - return static_cast(Statement::Parent()); - } - - /// Sets the else-statement condition expression - /// @param condition the else condition expression - void SetCondition(const Expression* condition) { condition_ = condition; } - - private: - const Expression* condition_ = nullptr; -}; - } // namespace tint::sem #endif // SRC_TINT_SEM_IF_STATEMENT_H_ diff --git a/src/tint/sem/type_mappings.h b/src/tint/sem/type_mappings.h index 1bbc46f5f1..4aea326499 100644 --- a/src/tint/sem/type_mappings.h +++ b/src/tint/sem/type_mappings.h @@ -22,7 +22,6 @@ namespace tint::ast { class Array; class CallExpression; class Expression; -class ElseStatement; class ForLoopStatement; class Function; class IfStatement; @@ -39,7 +38,6 @@ namespace tint::sem { class Array; class Call; class Expression; -class ElseStatement; class ForLoopStatement; class Function; class IfStatement; @@ -63,7 +61,6 @@ struct TypeMappings { Array* operator()(ast::Array*); Call* operator()(ast::CallExpression*); Expression* operator()(ast::Expression*); - ElseStatement* operator()(ast::ElseStatement*); ForLoopStatement* operator()(ast::ForLoopStatement*); Function* operator()(ast::Function*); IfStatement* operator()(ast::IfStatement*); diff --git a/src/tint/transform/loop_to_for_loop.cc b/src/tint/transform/loop_to_for_loop.cc index d01ac48e26..f5bc05d58b 100644 --- a/src/tint/transform/loop_to_for_loop.cc +++ b/src/tint/transform/loop_to_for_loop.cc @@ -83,14 +83,14 @@ void LoopToForLoop::Run(CloneContext& ctx, const DataMap&, DataMap&) const { if (!if_stmt) { return nullptr; } + auto* else_stmt = tint::As(if_stmt->else_statement); bool negate_condition = false; if (IsBlockWithSingleBreak(if_stmt->body) && - if_stmt->else_statements.empty()) { + if_stmt->else_statement == nullptr) { negate_condition = true; - } else if (if_stmt->body->Empty() && if_stmt->else_statements.size() == 1 && - if_stmt->else_statements[0]->condition == nullptr && - IsBlockWithSingleBreak(if_stmt->else_statements[0]->body)) { + } else if (if_stmt->body->Empty() && else_stmt && + IsBlockWithSingleBreak(else_stmt)) { negate_condition = false; } else { return nullptr; diff --git a/src/tint/transform/loop_to_for_loop_test.cc b/src/tint/transform/loop_to_for_loop_test.cc index c34b0e84b0..d4a7693e64 100644 --- a/src/tint/transform/loop_to_for_loop_test.cc +++ b/src/tint/transform/loop_to_for_loop_test.cc @@ -302,5 +302,57 @@ fn f() { EXPECT_EQ(expect, str(got)); } +TEST_F(LoopToForLoopTest, NoTransform_IfBreakWithElse) { + auto* src = R"( +fn f() { + var i : i32; + i = 0; + loop { + if ((i > 15)) { + break; + } else { + } + _ = 123; + + continuing { + i = (i + 1); + } + } +} +)"; + + auto* expect = src; + + auto got = Run(src); + + EXPECT_EQ(expect, str(got)); +} + +TEST_F(LoopToForLoopTest, NoTransform_IfBreakWithElseIf) { + auto* src = R"( +fn f() { + var i : i32; + i = 0; + loop { + if ((i > 15)) { + break; + } else if (true) { + } + _ = 123; + + continuing { + i = (i + 1); + } + } +} +)"; + + auto* expect = src; + + auto got = Run(src); + + EXPECT_EQ(expect, str(got)); +} + } // namespace } // namespace tint::transform diff --git a/src/tint/transform/multiplanar_external_texture.cc b/src/tint/transform/multiplanar_external_texture.cc index 37d1fa004f..a5a2134bc3 100644 --- a/src/tint/transform/multiplanar_external_texture.cc +++ b/src/tint/transform/multiplanar_external_texture.cc @@ -288,7 +288,7 @@ struct MultiplanarExternalTexture::State { b.Block( // color = textureLoad(plane0, coord, 0).rgb; b.Assign("color", b.MemberAccessor(single_plane_call, "rgb"))), - b.Else(b.Block( + b.Block( // color = vec4(plane_0_call.r, plane_1_call.rg, 1.0) * // params.yuvToRgbConversionMatrix; b.Assign("color", @@ -296,7 +296,7 @@ struct MultiplanarExternalTexture::State { b.MemberAccessor(plane_0_call, "r"), b.MemberAccessor(plane_1_call, "rg"), 1.0f), b.MemberAccessor( - "params", "yuvToRgbConversionMatrix")))))), + "params", "yuvToRgbConversionMatrix"))))), // return vec4(color, 1.0f); b.Return(b.vec4("color", 1.0f))}; } diff --git a/src/tint/transform/promote_side_effects_to_decl.cc b/src/tint/transform/promote_side_effects_to_decl.cc index 8eb62eb59a..146d9a7b25 100644 --- a/src/tint/transform/promote_side_effects_to_decl.cc +++ b/src/tint/transform/promote_side_effects_to_decl.cc @@ -393,9 +393,6 @@ class DecomposeSideEffects::CollectHoistsState : public StateBase { [&](const ast::CallStatement* s) { // ProcessStatement(s->expr); }, - [&](const ast::ElseStatement* s) { // - ProcessStatement(s->condition); - }, [&](const ast::ForLoopStatement* s) { ProcessStatement(s->condition); }, @@ -594,18 +591,6 @@ class DecomposeSideEffects::DecomposeState : public StateBase { InsertBefore(stmts, s); return ctx.CloneWithoutTransform(s); }, - [&](const ast::ElseStatement* s) -> const ast::Statement* { - if (!s->condition || !sem.Get(s->condition)->HasSideEffects()) { - return nullptr; - } - // NOTE: We shouldn't reach here as else-if with side-effect - // conditions are simplified to else { if } by - // SimplifySideEffectStatements. - ast::StatementList stmts; - ctx.Replace(s->condition, Decompose(s->condition, &stmts)); - InsertBefore(stmts, s); - return ctx.CloneWithoutTransform(s); - }, [&](const ast::ForLoopStatement* s) -> const ast::Statement* { if (!s->condition || !sem.Get(s->condition)->HasSideEffects()) { return nullptr; diff --git a/src/tint/transform/unwind_discard_functions.cc b/src/tint/transform/unwind_discard_functions.cc index b15f3a093c..81b1d6d4d9 100644 --- a/src/tint/transform/unwind_discard_functions.cc +++ b/src/tint/transform/unwind_discard_functions.cc @@ -43,15 +43,6 @@ class State { Symbol module_discard_var_name; // Use ModuleDiscardVarName() to read Symbol module_discard_func_name; // Use ModuleDiscardFuncName() to read - // If `block`'s parent is of type TO, returns pointer to it. - template - const TO* ParentAs(const ast::BlockStatement* block) { - if (auto* sem_block = sem.Get(block)) { - return As(sem_block->Parent()); - } - return nullptr; - } - // Returns true if `sem_expr` contains a call expression that may // (transitively) execute a discard statement. bool MayDiscard(const sem::Expression* sem_expr) { @@ -265,14 +256,6 @@ class State { } return TryInsertAfter(s, sem_expr); }, - [&](const ast::ElseStatement* s) -> const ast::Statement* { - if (MayDiscard(sem.Get(s->condition))) { - TINT_ICE(Transform, b.Diagnostics()) - << "Unexpected ElseIf condition that may discard. Make sure " - "transform::PromoteSideEffectsToDecl was run first."; - } - return nullptr; - }, [&](const ast::ForLoopStatement* s) -> const ast::Statement* { if (MayDiscard(sem.Get(s->condition))) { TINT_ICE(Transform, b.Diagnostics()) @@ -326,20 +309,6 @@ class State { void Run() { ctx.ReplaceAll( [&](const ast::BlockStatement* block) -> const ast::Statement* { - // If this block is for an else-if statement, process the else-if now - // before processing its block statements. - // NOTE: we can't replace else statements at this point - this would - // need to be done when replacing the parent if-statement. However, in - // this transform, we don't ever expect to need to do this as else-ifs - // are converted to else { if } by PromoteSideEffectsToDecl, so this - // is only for validation. - if (auto* sem_else = ParentAs(block)) { - if (auto* new_stmt = Statement(sem_else->Declaration())) { - TINT_ASSERT(Transform, new_stmt == nullptr); - return nullptr; - } - } - // Iterate block statements and replace them as needed. for (auto* stmt : block->statements) { if (auto* new_stmt = Statement(stmt)) { diff --git a/src/tint/transform/utils/hoist_to_decl_before.cc b/src/tint/transform/utils/hoist_to_decl_before.cc index 15d09267df..78ba992acd 100644 --- a/src/tint/transform/utils/hoist_to_decl_before.cc +++ b/src/tint/transform/utils/hoist_to_decl_before.cc @@ -39,25 +39,17 @@ class HoistToDeclBefore::State { ast::StatementList cont_decls; }; - /// Holds information about 'if's with 'else-if' statements that need to be - /// decomposed into 'if {else}' so that declaration statements can be - /// inserted before the condition expression. - struct IfInfo { - /// Info for each else-if that needs decomposing - struct ElseIfInfo { - /// Decls to insert before condition - ast::StatementList cond_decls; - }; - - /// 'else if's that need to be decomposed to 'else { if }' - std::unordered_map else_ifs; + /// Info for each else-if that needs decomposing + struct ElseIfInfo { + /// Decls to insert before condition + ast::StatementList cond_decls; }; /// For-loops that need to be decomposed to loops. std::unordered_map loops; - /// If statements with 'else if's that need to be decomposed to 'else {if}' - std::unordered_map ifs; + /// 'else if' statements that need to be decomposed to 'else {if}' + std::unordered_map else_ifs; // Converts any for-loops marked for conversion to loops, inserting // registered declaration statements before the condition or continuing @@ -118,78 +110,27 @@ class HoistToDeclBefore::State { } void ElseIfsToElseWithNestedIfs() { - if (ifs.empty()) { - return; - } - - ctx.ReplaceAll([&](const ast::IfStatement* if_stmt) // - -> const ast::IfStatement* { - auto& sem = ctx.src->Sem(); - auto* sem_if = sem.Get(if_stmt); - if (!sem_if) { - return nullptr; - } - - auto it = ifs.find(sem_if); - if (it == ifs.end()) { - return nullptr; - } - auto& if_info = it->second; - - // This if statement has "else if"s that need to be converted to "else - // { if }"s - - ast::ElseStatementList next_else_stmts; - next_else_stmts.reserve(if_stmt->else_statements.size()); - - for (auto* else_stmt : utils::Reverse(if_stmt->else_statements)) { - if (else_stmt->condition == nullptr) { - // The last 'else', keep as is - next_else_stmts.insert(next_else_stmts.begin(), ctx.Clone(else_stmt)); - - } else { - auto* sem_else_if = sem.Get(else_stmt); - - auto it2 = if_info.else_ifs.find(sem_else_if); - if (it2 == if_info.else_ifs.end()) { - // 'else if' we don't need to modify (no decls to insert), so - // keep as is - next_else_stmts.insert(next_else_stmts.begin(), - ctx.Clone(else_stmt)); - - } else { - // 'else if' we need to replace with 'else { if }' - auto& else_if_info = it2->second; - - // Build the else body's statements, starting with let decls for - // the conditional expression - auto& body_stmts = else_if_info.cond_decls; - - // Build nested if - auto* cond = ctx.Clone(else_stmt->condition); - auto* body = ctx.Clone(else_stmt->body); - body_stmts.emplace_back(b.If(cond, body, next_else_stmts)); - - // Build else - auto* else_with_nested_if = b.Else(b.Block(body_stmts)); - - // This will be used in parent if (either another nested if, or - // top-level if) - next_else_stmts = {else_with_nested_if}; + // Decompose 'else-if' statements into 'else { if }' blocks. + ctx.ReplaceAll( + [&](const ast::IfStatement* else_if) -> const ast::Statement* { + if (!else_ifs.count(else_if)) { + return nullptr; } - } - } + auto& else_if_info = else_ifs[else_if]; - // Build a new top-level if with new else statements - if (next_else_stmts.empty()) { - TINT_ICE(Transform, b.Diagnostics()) - << "Expected else statements to insert into new if"; - } - auto* cond = ctx.Clone(if_stmt->condition); - auto* body = ctx.Clone(if_stmt->body); - auto* new_if = b.If(cond, body, next_else_stmts); - return new_if; - }); + // Build the else block's body statements, starting with let decls for + // the conditional expression. + auto& body_stmts = else_if_info.cond_decls; + + // Move the 'else-if' into the new `else` block as a plain 'if'. + auto* cond = ctx.Clone(else_if->condition); + auto* body = ctx.Clone(else_if->body); + auto* new_if = b.If(cond, body, ctx.Clone(else_if->else_statement)); + body_stmts.emplace_back(new_if); + + // Replace the 'else-if' with the new 'else' block. + return b.Block(body_stmts); + }); } public: @@ -235,13 +176,14 @@ class HoistToDeclBefore::State { const ast::Statement* stmt) { auto* ip = before_stmt->Declaration(); - if (auto* else_if = before_stmt->As()) { + auto* else_if = before_stmt->As(); + if (else_if && else_if->Parent()->Is()) { // Insertion point is an 'else if' condition. // Need to convert 'else if' to 'else { if }'. - auto& if_info = ifs[else_if->Parent()->As()]; + auto& else_if_info = else_ifs[else_if->Declaration()]; // Index the map to convert this else if, even if `stmt` is nullptr. - auto& decls = if_info.else_ifs[else_if].cond_decls; + auto& decls = else_if_info.cond_decls; if (stmt) { decls.emplace_back(stmt); } diff --git a/src/tint/transform/utils/hoist_to_decl_before_test.cc b/src/tint/transform/utils/hoist_to_decl_before_test.cc index 589eb9a5e6..e313d918ba 100644 --- a/src/tint/transform/utils/hoist_to_decl_before_test.cc +++ b/src/tint/transform/utils/hoist_to_decl_before_test.cc @@ -187,8 +187,8 @@ TEST_F(HoistToDeclBeforeTest, ElseIf) { auto* var = b.Decl(b.Var("a", b.ty.bool_())); auto* expr = b.Expr("a"); auto* s = b.If(b.Expr(true), b.Block(), // - b.Else(expr, b.Block()), // - b.Else(b.Block())); + b.If(expr, b.Block(), // + b.Block())); b.Func("f", {}, b.ty.void_(), {var, s}); Program original(std::move(b)); @@ -383,8 +383,8 @@ TEST_F(HoistToDeclBeforeTest, Prepare_ElseIf) { auto* var = b.Decl(b.Var("a", b.ty.bool_())); auto* expr = b.Expr("a"); auto* s = b.If(b.Expr(true), b.Block(), // - b.Else(expr, b.Block()), // - b.Else(b.Block())); + b.If(expr, b.Block(), // + b.Block())); b.Func("f", {}, b.ty.void_(), {var, s}); Program original(std::move(b)); @@ -556,10 +556,9 @@ TEST_F(HoistToDeclBeforeTest, InsertBefore_ElseIf) { ProgramBuilder b; b.Func("foo", {}, b.ty.void_(), {}); auto* var = b.Decl(b.Var("a", b.ty.bool_())); - auto* elseif = b.Else(b.Expr("a"), b.Block()); + auto* elseif = b.If(b.Expr("a"), b.Block(), b.Block()); auto* s = b.If(b.Expr(true), b.Block(), // - elseif, // - b.Else(b.Block())); + elseif); b.Func("f", {}, b.ty.void_(), {var, s}); Program original(std::move(b)); diff --git a/src/tint/writer/glsl/generator_impl.cc b/src/tint/writer/glsl/generator_impl.cc index f21f96da96..b60b6ff2ee 100644 --- a/src/tint/writer/glsl/generator_impl.cc +++ b/src/tint/writer/glsl/generator_impl.cc @@ -1842,36 +1842,20 @@ bool GeneratorImpl::EmitIf(const ast::IfStatement* stmt) { return false; } - for (auto* e : stmt->else_statements) { - if (e->condition) { - line() << "} else {"; - increment_indent(); - - { - auto out = line(); - out << "if ("; - if (!EmitExpression(out, e->condition)) { - return false; - } - out << ") {"; + if (stmt->else_statement) { + line() << "} else {"; + if (auto* block = stmt->else_statement->As()) { + if (!EmitStatementsWithIndent(block->statements)) { + return false; } } else { - line() << "} else {"; - } - - if (!EmitStatementsWithIndent(e->body->statements)) { - return false; + if (!EmitStatementsWithIndent({stmt->else_statement})) { + return false; + } } } - line() << "}"; - for (auto* e : stmt->else_statements) { - if (e->condition) { - decrement_indent(); - line() << "}"; - } - } return true; } diff --git a/src/tint/writer/glsl/generator_impl_binary_test.cc b/src/tint/writer/glsl/generator_impl_binary_test.cc index 744eef48af..c6b4ca78e0 100644 --- a/src/tint/writer/glsl/generator_impl_binary_test.cc +++ b/src/tint/writer/glsl/generator_impl_binary_test.cc @@ -360,10 +360,9 @@ TEST_F(GlslGeneratorImplTest_Binary, If_WithLogical) { auto* expr = If(create(ast::BinaryOp::kLogicalAnd, Expr("a"), Expr("b")), Block(Return(1)), - Else(create(ast::BinaryOp::kLogicalOr, - Expr("b"), Expr("c")), - Block(Return(2))), - Else(Block(Return(3)))); + If(create(ast::BinaryOp::kLogicalOr, + Expr("b"), Expr("c")), + Block(Return(2)), Block(Return(3)))); Func("func", {}, ty.i32(), {WrapInStatement(expr)}); GeneratorImpl& gen = Build(); diff --git a/src/tint/writer/glsl/generator_impl_if_test.cc b/src/tint/writer/glsl/generator_impl_if_test.cc index 69759433b8..9efa861d81 100644 --- a/src/tint/writer/glsl/generator_impl_if_test.cc +++ b/src/tint/writer/glsl/generator_impl_if_test.cc @@ -46,9 +46,7 @@ TEST_F(GlslGeneratorImplTest_If, Emit_IfWithElseIf) { auto* cond = Expr("cond"); auto* body = Block(Return()); - auto* i = If( - cond, body, - ast::ElseStatementList{create(else_cond, else_body)}); + auto* i = If(cond, body, If(else_cond, else_body)); WrapInFunction(i); GeneratorImpl& gen = Build(); @@ -73,9 +71,7 @@ TEST_F(GlslGeneratorImplTest_If, Emit_IfWithElse) { auto* cond = Expr("cond"); auto* body = Block(Return()); - auto* i = If( - cond, body, - ast::ElseStatementList{create(nullptr, else_body)}); + auto* i = If(cond, body, else_body); WrapInFunction(i); GeneratorImpl& gen = Build(); @@ -103,11 +99,7 @@ TEST_F(GlslGeneratorImplTest_If, Emit_IfWithMultiple) { auto* cond = Expr("cond"); auto* body = Block(Return()); - auto* i = If(cond, body, - ast::ElseStatementList{ - create(else_cond, else_body), - create(nullptr, else_body_2), - }); + auto* i = If(cond, body, If(else_cond, else_body, else_body_2)); WrapInFunction(i); GeneratorImpl& gen = Build(); diff --git a/src/tint/writer/hlsl/generator_impl.cc b/src/tint/writer/hlsl/generator_impl.cc index c34a1c6670..c257989575 100644 --- a/src/tint/writer/hlsl/generator_impl.cc +++ b/src/tint/writer/hlsl/generator_impl.cc @@ -2691,36 +2691,20 @@ bool GeneratorImpl::EmitIf(const ast::IfStatement* stmt) { return false; } - for (auto* e : stmt->else_statements) { - if (e->condition) { - line() << "} else {"; - increment_indent(); - - { - auto out = line(); - out << "if ("; - if (!EmitExpression(out, e->condition)) { - return false; - } - out << ") {"; + if (stmt->else_statement) { + line() << "} else {"; + if (auto* block = stmt->else_statement->As()) { + if (!EmitStatementsWithIndent(block->statements)) { + return false; } } else { - line() << "} else {"; - } - - if (!EmitStatementsWithIndent(e->body->statements)) { - return false; + if (!EmitStatementsWithIndent({stmt->else_statement})) { + return false; + } } } - line() << "}"; - for (auto* e : stmt->else_statements) { - if (e->condition) { - decrement_indent(); - line() << "}"; - } - } return true; } diff --git a/src/tint/writer/hlsl/generator_impl_binary_test.cc b/src/tint/writer/hlsl/generator_impl_binary_test.cc index 8fc89eb694..d2f075b200 100644 --- a/src/tint/writer/hlsl/generator_impl_binary_test.cc +++ b/src/tint/writer/hlsl/generator_impl_binary_test.cc @@ -348,10 +348,9 @@ TEST_F(HlslGeneratorImplTest_Binary, If_WithLogical) { auto* expr = If(create(ast::BinaryOp::kLogicalAnd, Expr("a"), Expr("b")), Block(Return(1)), - Else(create(ast::BinaryOp::kLogicalOr, - Expr("b"), Expr("c")), - Block(Return(2))), - Else(Block(Return(3)))); + If(create(ast::BinaryOp::kLogicalOr, + Expr("b"), Expr("c")), + Block(Return(2)), Block(Return(3)))); Func("func", {}, ty.i32(), {WrapInStatement(expr)}); GeneratorImpl& gen = Build(); diff --git a/src/tint/writer/hlsl/generator_impl_if_test.cc b/src/tint/writer/hlsl/generator_impl_if_test.cc index f2e092cf9c..0812282e47 100644 --- a/src/tint/writer/hlsl/generator_impl_if_test.cc +++ b/src/tint/writer/hlsl/generator_impl_if_test.cc @@ -46,9 +46,7 @@ TEST_F(HlslGeneratorImplTest_If, Emit_IfWithElseIf) { auto* cond = Expr("cond"); auto* body = Block(Return()); - auto* i = If( - cond, body, - ast::ElseStatementList{create(else_cond, else_body)}); + auto* i = If(cond, body, If(else_cond, else_body)); WrapInFunction(i); GeneratorImpl& gen = Build(); @@ -73,9 +71,7 @@ TEST_F(HlslGeneratorImplTest_If, Emit_IfWithElse) { auto* cond = Expr("cond"); auto* body = Block(Return()); - auto* i = If( - cond, body, - ast::ElseStatementList{create(nullptr, else_body)}); + auto* i = If(cond, body, else_body); WrapInFunction(i); GeneratorImpl& gen = Build(); @@ -103,11 +99,7 @@ TEST_F(HlslGeneratorImplTest_If, Emit_IfWithMultiple) { auto* cond = Expr("cond"); auto* body = Block(Return()); - auto* i = If(cond, body, - ast::ElseStatementList{ - create(else_cond, else_body), - create(nullptr, else_body_2), - }); + auto* i = If(cond, body, If(else_cond, else_body, else_body_2)); WrapInFunction(i); GeneratorImpl& gen = Build(); diff --git a/src/tint/writer/msl/generator_impl.cc b/src/tint/writer/msl/generator_impl.cc index dbe1114c81..de3810294d 100644 --- a/src/tint/writer/msl/generator_impl.cc +++ b/src/tint/writer/msl/generator_impl.cc @@ -2051,36 +2051,20 @@ bool GeneratorImpl::EmitIf(const ast::IfStatement* stmt) { return false; } - for (auto* e : stmt->else_statements) { - if (e->condition) { - line() << "} else {"; - increment_indent(); - - { - auto out = line(); - out << "if ("; - if (!EmitExpression(out, e->condition)) { - return false; - } - out << ") {"; + if (stmt->else_statement) { + line() << "} else {"; + if (auto* block = stmt->else_statement->As()) { + if (!EmitStatementsWithIndent(block->statements)) { + return false; } } else { - line() << "} else {"; - } - - if (!EmitStatementsWithIndent(e->body->statements)) { - return false; + if (!EmitStatementsWithIndent({stmt->else_statement})) { + return false; + } } } - line() << "}"; - for (auto* e : stmt->else_statements) { - if (e->condition) { - decrement_indent(); - line() << "}"; - } - } return true; } diff --git a/src/tint/writer/msl/generator_impl_if_test.cc b/src/tint/writer/msl/generator_impl_if_test.cc index ff0cabcb9b..49798389e9 100644 --- a/src/tint/writer/msl/generator_impl_if_test.cc +++ b/src/tint/writer/msl/generator_impl_if_test.cc @@ -38,7 +38,7 @@ TEST_F(MslGeneratorImplTest, Emit_If) { TEST_F(MslGeneratorImplTest, Emit_IfWithElseIf) { auto* cond = Var("cond", ty.bool_()); auto* else_cond = Var("else_cond", ty.bool_()); - auto* i = If(cond, Block(Return()), Else(else_cond, Block(Return()))); + auto* i = If(cond, Block(Return()), If(else_cond, Block(Return()))); WrapInFunction(cond, else_cond, i); GeneratorImpl& gen = Build(); @@ -58,7 +58,7 @@ TEST_F(MslGeneratorImplTest, Emit_IfWithElseIf) { TEST_F(MslGeneratorImplTest, Emit_IfWithElse) { auto* cond = Var("cond", ty.bool_()); - auto* i = If(cond, Block(Return()), Else(nullptr, Block(Return()))); + auto* i = If(cond, Block(Return()), Block(Return())); WrapInFunction(cond, i); GeneratorImpl& gen = Build(); @@ -77,8 +77,8 @@ TEST_F(MslGeneratorImplTest, Emit_IfWithElse) { TEST_F(MslGeneratorImplTest, Emit_IfWithMultiple) { auto* cond = Var("cond", ty.bool_()); auto* else_cond = Var("else_cond", ty.bool_()); - auto* i = If(cond, Block(Return()), Else(else_cond, Block(Return())), - Else(nullptr, Block(Return()))); + auto* i = If(cond, Block(Return()), + If(else_cond, Block(Return()), Block(Return()))); WrapInFunction(cond, else_cond, i); GeneratorImpl& gen = Build(); diff --git a/src/tint/writer/spirv/builder.cc b/src/tint/writer/spirv/builder.cc index aab35fdc67..672d10e525 100644 --- a/src/tint/writer/spirv/builder.cc +++ b/src/tint/writer/spirv/builder.cc @@ -3365,11 +3365,9 @@ uint32_t Builder::GenerateBitcastExpression( return result_id; } -bool Builder::GenerateConditionalBlock( - const ast::Expression* cond, - const ast::BlockStatement* true_body, - size_t cur_else_idx, - const ast::ElseStatementList& else_stmts) { +bool Builder::GenerateConditionalBlock(const ast::Expression* cond, + const ast::BlockStatement* true_body, + const ast::Statement* else_stmt) { auto cond_id = GenerateExpressionWithLoadIfNeeded(cond); if (cond_id == 0) { return false; @@ -3389,8 +3387,7 @@ bool Builder::GenerateConditionalBlock( // if there are no more else statements we branch on false to the merge // block otherwise we branch to the false block - auto false_block_id = - cur_else_idx < else_stmts.size() ? next_id() : merge_block_id; + auto false_block_id = else_stmt ? next_id() : merge_block_id; if (!push_function_inst(spv::Op::OpBranchConditional, {Operand(cond_id), Operand(true_block_id), @@ -3418,15 +3415,15 @@ bool Builder::GenerateConditionalBlock( return false; } - auto* else_stmt = else_stmts[cur_else_idx]; // Handle the else case by just outputting the statements. - if (!else_stmt->condition) { - if (!GenerateBlockStatement(else_stmt->body)) { + if (auto* block = else_stmt->As()) { + if (!GenerateBlockStatement(block)) { return false; } } else { - if (!GenerateConditionalBlock(else_stmt->condition, else_stmt->body, - cur_else_idx + 1, else_stmts)) { + auto* elseif = else_stmt->As(); + if (!GenerateConditionalBlock(elseif->condition, elseif->body, + elseif->else_statement)) { return false; } } @@ -3460,7 +3457,7 @@ bool Builder::GenerateIfStatement(const ast::IfStatement* stmt) { return block && (block->statements.size() == 1) && block->Last()->Is(); }; - if (is_just_a_break(stmt->body) && stmt->else_statements.empty()) { + if (is_just_a_break(stmt->body) && stmt->else_statement == nullptr) { // It's a break-if. TINT_ASSERT(Writer, !backedge_stack_.empty()); const auto cond_id = GenerateExpressionWithLoadIfNeeded(stmt->condition); @@ -3473,9 +3470,8 @@ bool Builder::GenerateIfStatement(const ast::IfStatement* stmt) { Operand(ci.loop_header_id)}); return true; } else if (stmt->body->Empty()) { - const auto& es = stmt->else_statements; - if (es.size() == 1 && !es.back()->condition && - is_just_a_break(es.back()->body)) { + auto* es_block = As(stmt->else_statement); + if (es_block && is_just_a_break(es_block)) { // It's a break-unless. TINT_ASSERT(Writer, !backedge_stack_.empty()); const auto cond_id = @@ -3492,8 +3488,8 @@ bool Builder::GenerateIfStatement(const ast::IfStatement* stmt) { } } - if (!GenerateConditionalBlock(stmt->condition, stmt->body, 0, - stmt->else_statements)) { + if (!GenerateConditionalBlock(stmt->condition, stmt->body, + stmt->else_statement)) { return false; } return true; diff --git a/src/tint/writer/spirv/builder.h b/src/tint/writer/spirv/builder.h index 7ef316fddf..8bdbcb9452 100644 --- a/src/tint/writer/spirv/builder.h +++ b/src/tint/writer/spirv/builder.h @@ -438,13 +438,11 @@ class Builder { /// Generates a conditional section merge block /// @param cond the condition /// @param true_body the statements making up the true block - /// @param cur_else_idx the index of the current else statement to process - /// @param else_stmts the list of all else statements + /// @param else_stmt the statement for the else block /// @returns true on success, false on failure bool GenerateConditionalBlock(const ast::Expression* cond, const ast::BlockStatement* true_body, - size_t cur_else_idx, - const ast::ElseStatementList& else_stmts); + const ast::Statement* else_stmt); /// Generates a statement /// @param stmt the statement to generate /// @returns true if the statement was generated diff --git a/src/tint/writer/spirv/builder_if_test.cc b/src/tint/writer/spirv/builder_if_test.cc index 661bbd8658..43453c817f 100644 --- a/src/tint/writer/spirv/builder_if_test.cc +++ b/src/tint/writer/spirv/builder_if_test.cc @@ -106,7 +106,7 @@ TEST_F(BuilderTest, If_WithElse) { auto* body = Block(Assign("v", 2)); auto* else_body = Block(Assign("v", 3)); - auto* expr = If(true, body, Else(else_body)); + auto* expr = If(true, body, else_body); WrapInFunction(expr); spirv::Builder& b = Build(); @@ -148,7 +148,7 @@ TEST_F(BuilderTest, If_WithElseIf) { auto* body = Block(Assign("v", 2)); auto* else_body = Block(Assign("v", 3)); - auto* expr = If(true, body, Else(true, else_body)); + auto* expr = If(true, body, If(true, else_body)); WrapInFunction(expr); spirv::Builder& b = Build(); @@ -202,9 +202,9 @@ TEST_F(BuilderTest, If_WithMultiple) { auto* else_body = Block(Assign("v", 5)); auto* expr = If(true, body, // - Else(true, elseif_1_body), // - Else(false, elseif_2_body), // - Else(else_body)); + If(true, elseif_1_body, // + If(false, elseif_2_body, // + else_body))); WrapInFunction(expr); spirv::Builder& b = Build(); @@ -305,7 +305,7 @@ TEST_F(BuilderTest, If_WithElseBreak) { // } auto* else_body = Block(Break()); - auto* if_stmt = If(true, Block(), Else(else_body)); + auto* if_stmt = If(true, Block(), else_body); auto* loop_body = Block(if_stmt); @@ -349,7 +349,7 @@ TEST_F(BuilderTest, If_WithContinueAndBreak) { // } // } - auto* if_stmt = If(true, Block(Continue()), Else(Block(Break()))); + auto* if_stmt = If(true, Block(Continue()), Block(Break())); auto* expr = Loop(Block(if_stmt), Block()); WrapInFunction(expr); @@ -392,7 +392,7 @@ TEST_F(BuilderTest, If_WithElseContinue) { // } auto* else_body = Block(create()); - auto* if_stmt = If(true, Block(), Else(else_body)); + auto* if_stmt = If(true, Block(), else_body); auto* loop_body = Block(if_stmt, Break()); @@ -493,7 +493,7 @@ TEST_F(BuilderTest, IfElse_BothReturn) { { If(true, // Block(Return(true)), // - Else(Block(Return(true)))), + Block(Return(true))), }); spirv::Builder& b = Build(); @@ -589,7 +589,7 @@ TEST_F(BuilderTest, If_ElseIf_WithReturn) { // return; // } - auto* if_stmt = If(false, Block(), Else(true, Block(Return()))); + auto* if_stmt = If(false, Block(), If(true, Block(Return()))); auto* fn = Func("f", {}, ty.void_(), {if_stmt}); spirv::Builder& b = Build(); @@ -627,7 +627,7 @@ TEST_F(BuilderTest, Loop_If_ElseIf_WithBreak) { // } // } - auto* if_stmt = If(false, Block(), Else(true, Block(Break()))); + auto* if_stmt = If(false, Block(), If(true, Block(Break()))); auto* fn = Func("f", {}, ty.void_(), {Loop(Block(if_stmt))}); spirv::Builder& b = Build(); diff --git a/src/tint/writer/spirv/builder_loop_test.cc b/src/tint/writer/spirv/builder_loop_test.cc index 555c167185..34b0182fbc 100644 --- a/src/tint/writer/spirv/builder_loop_test.cc +++ b/src/tint/writer/spirv/builder_loop_test.cc @@ -236,8 +236,7 @@ TEST_F(BuilderTest, Loop_WithContinuing_BreakIf) { // } // } - auto* if_stmt = create(Expr(true), Block(Break()), - ast::ElseStatementList{}); + auto* if_stmt = If(Expr(true), Block(Break())); auto* continuing = Block(if_stmt); auto* loop = Loop(Block(), continuing); WrapInFunction(loop); @@ -269,9 +268,7 @@ TEST_F(BuilderTest, Loop_WithContinuing_BreakUnless) { // if (true) {} else { break; } // } // } - auto* if_stmt = create( - Expr(true), Block(), - ast::ElseStatementList{Else(nullptr, Block(Break()))}); + auto* if_stmt = If(Expr(true), Block(), Block(Break())); auto* continuing = Block(if_stmt); auto* loop = Loop(Block(), continuing); WrapInFunction(loop); @@ -306,7 +303,7 @@ TEST_F(BuilderTest, Loop_WithContinuing_BreakIf_ConditionIsVar) { // } auto* cond_var = Decl(Var("cond", nullptr, Expr(true))); - auto* if_stmt = If(Expr("cond"), Block(Break()), ast::ElseStatementList{}); + auto* if_stmt = If(Expr("cond"), Block(Break())); auto* continuing = Block(cond_var, if_stmt); auto* loop = Loop(Block(), continuing); WrapInFunction(loop); @@ -344,8 +341,7 @@ TEST_F(BuilderTest, Loop_WithContinuing_BreakUnless_ConditionIsVar) { // } // } auto* cond_var = Decl(Var("cond", nullptr, Expr(true))); - auto* if_stmt = If(Expr("cond"), Block(), - ast::ElseStatementList{Else(nullptr, Block(Break()))}); + auto* if_stmt = If(Expr("cond"), Block(), Block(Break())); auto* continuing = Block(cond_var, if_stmt); auto* loop = Loop(Block(), continuing); WrapInFunction(loop); @@ -388,13 +384,11 @@ TEST_F(BuilderTest, Loop_WithContinuing_BreakIf_Nested) { // } // } - auto* inner_if_stmt = create(Expr(true), Block(Break()), - ast::ElseStatementList{}); + auto* inner_if_stmt = If(Expr(true), Block(Break())); auto* inner_continuing = Block(inner_if_stmt); auto* inner_loop = Loop(Block(), inner_continuing); - auto* outer_if_stmt = create(Expr(true), Block(Break()), - ast::ElseStatementList{}); + auto* outer_if_stmt = If(Expr(true), Block(Break())); auto* outer_continuing = Block(inner_loop, outer_if_stmt); auto* outer_loop = Loop(Block(), outer_continuing); @@ -443,15 +437,11 @@ TEST_F(BuilderTest, Loop_WithContinuing_BreakUnless_Nested) { // } // } - auto* inner_if_stmt = create( - Expr(true), Block(), - ast::ElseStatementList{Else(nullptr, Block(Break()))}); + auto* inner_if_stmt = If(Expr(true), Block(), Block(Break())); auto* inner_continuing = Block(inner_if_stmt); auto* inner_loop = Loop(Block(), inner_continuing); - auto* outer_if_stmt = create( - Expr(true), Block(), - ast::ElseStatementList{Else(nullptr, Block(Break()))}); + auto* outer_if_stmt = If(Expr(true), Block(), Block(Break())); auto* outer_continuing = Block(inner_loop, outer_if_stmt); auto* outer_loop = Loop(Block(), outer_continuing); diff --git a/src/tint/writer/wgsl/generator_impl.cc b/src/tint/writer/wgsl/generator_impl.cc index a733765416..b836800059 100644 --- a/src/tint/writer/wgsl/generator_impl.cc +++ b/src/tint/writer/wgsl/generator_impl.cc @@ -1075,20 +1075,27 @@ bool GeneratorImpl::EmitIf(const ast::IfStatement* stmt) { return false; } - for (auto* e : stmt->else_statements) { - if (e->condition) { - auto out = line(); - out << "} else if ("; - if (!EmitExpression(out, e->condition)) { + const ast::Statement* e = stmt->else_statement; + while (e) { + if (auto* elseif = e->As()) { + { + auto out = line(); + out << "} else if ("; + if (!EmitExpression(out, elseif->condition)) { + return false; + } + out << ") {"; + } + if (!EmitStatementsWithIndent(elseif->body->statements)) { return false; } - out << ") {"; + e = elseif->else_statement; } else { line() << "} else {"; - } - - if (!EmitStatementsWithIndent(e->body->statements)) { - return false; + if (!EmitStatementsWithIndent(e->As()->statements)) { + return false; + } + break; } } diff --git a/src/tint/writer/wgsl/generator_impl_if_test.cc b/src/tint/writer/wgsl/generator_impl_if_test.cc index 48180190bc..950da1ad3f 100644 --- a/src/tint/writer/wgsl/generator_impl_if_test.cc +++ b/src/tint/writer/wgsl/generator_impl_if_test.cc @@ -47,9 +47,7 @@ TEST_F(WgslGeneratorImplTest, Emit_IfWithElseIf) { auto* cond = Expr("cond"); auto* body = Block(Return()); - auto* i = If( - cond, body, - ast::ElseStatementList{create(else_cond, else_body)}); + auto* i = If(cond, body, If(else_cond, else_body)); WrapInFunction(i); GeneratorImpl& gen = Build(); @@ -72,9 +70,7 @@ TEST_F(WgslGeneratorImplTest, Emit_IfWithElse) { auto* cond = Expr("cond"); auto* body = Block(Return()); - auto* i = If( - cond, body, - ast::ElseStatementList{create(nullptr, else_body)}); + auto* i = If(cond, body, else_body); WrapInFunction(i); GeneratorImpl& gen = Build(); @@ -102,11 +98,7 @@ TEST_F(WgslGeneratorImplTest, Emit_IfWithMultiple) { auto* cond = Expr("cond"); auto* body = Block(Return()); - auto* i = If(cond, body, - ast::ElseStatementList{ - create(else_cond, else_body), - create(nullptr, else_body_2), - }); + auto* i = If(cond, body, If(else_cond, else_body, else_body_2)); WrapInFunction(i); GeneratorImpl& gen = Build(); diff --git a/test/tint/BUILD.gn b/test/tint/BUILD.gn index 8ad60fe73f..9c64420f49 100644 --- a/test/tint/BUILD.gn +++ b/test/tint/BUILD.gn @@ -163,7 +163,6 @@ tint_unittests_source_set("tint_unittests_ast_src") { "../../src/tint/ast/depth_multisampled_texture_test.cc", "../../src/tint/ast/depth_texture_test.cc", "../../src/tint/ast/discard_statement_test.cc", - "../../src/tint/ast/else_statement_test.cc", "../../src/tint/ast/enable_test.cc", "../../src/tint/ast/external_texture_test.cc", "../../src/tint/ast/f32_test.cc", @@ -486,7 +485,6 @@ tint_unittests_source_set("tint_unittests_wgsl_reader_src") { "../../src/tint/reader/wgsl/parser_impl_continue_stmt_test.cc", "../../src/tint/reader/wgsl/parser_impl_continuing_stmt_test.cc", "../../src/tint/reader/wgsl/parser_impl_depth_texture_test.cc", - "../../src/tint/reader/wgsl/parser_impl_elseif_stmt_test.cc", "../../src/tint/reader/wgsl/parser_impl_enable_directive_test.cc", "../../src/tint/reader/wgsl/parser_impl_equality_expression_test.cc", "../../src/tint/reader/wgsl/parser_impl_error_msg_test.cc",