From 228392558fc719a168f7d3a9c00403a6cc4fb3d5 Mon Sep 17 00:00:00 2001 From: dan sinclair Date: Mon, 27 Apr 2020 20:32:56 +0000 Subject: [PATCH] [spirv-writer] Add start of break and continue. This CL adds the beginning of break and continue support. The conditional versions are not supported, just the non-conditional. Bug: tint:5 Change-Id: I84418cffd3e29dc011c4313bf9aa3da4833c009f Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/20500 Reviewed-by: David Neto --- .gitignore | 1 + src/reader/spirv/function.cc | 82 +++++++++---------- src/writer/spirv/builder.cc | 49 ++++++++++- src/writer/spirv/builder.h | 10 +++ .../spirv/builder_accessor_expression_test.cc | 4 +- src/writer/spirv/builder_loop_test.cc | 70 +++++++++++++++- 6 files changed, 170 insertions(+), 46 deletions(-) diff --git a/.gitignore b/.gitignore index 6d3242f4e9..139e452a0d 100644 --- a/.gitignore +++ b/.gitignore @@ -6,6 +6,7 @@ build buildtools out +testing third_party/cpplint third_party/binutils third_party/googletest diff --git a/src/reader/spirv/function.cc b/src/reader/spirv/function.cc index 86fc03a816..1f24a10f68 100644 --- a/src/reader/spirv/function.cc +++ b/src/reader/spirv/function.cc @@ -495,7 +495,8 @@ bool FunctionEmitter::RegisterMerges() { const auto block_id = block.id(); auto* block_info = GetBlockInfo(block_id); if (!block_info) { - return Fail() << "internal error: block " << block_id << " missing; blocks should already " + return Fail() << "internal error: block " << block_id + << " missing; blocks should already " "have been registered"; } @@ -617,48 +618,47 @@ bool FunctionEmitter::VerifyHeaderContinueMergeOrder() { if (merge == 0) { continue; } - // This is a header. - const auto header = block_id; - const auto* header_info = block_info; - const auto header_pos = header_info->pos; - const auto merge_pos = GetBlockInfo(merge)->pos; + // This is a header. + const auto header = block_id; + const auto* header_info = block_info; + const auto header_pos = header_info->pos; + const auto merge_pos = GetBlockInfo(merge)->pos; - // Pos(H) < Pos(M(H)) - // Note: When recording merges we made sure H != M(H) - if (merge_pos <= header_pos) { - return Fail() << "Header " << header - << " does not strictly dominate its merge block " - << merge; - // TODO(dneto): Report a path from the entry block to the merge block - // without going through the header block. - } + // Pos(H) < Pos(M(H)) + // Note: When recording merges we made sure H != M(H) + if (merge_pos <= header_pos) { + return Fail() << "Header " << header + << " does not strictly dominate its merge block " << merge; + // TODO(dneto): Report a path from the entry block to the merge block + // without going through the header block. + } - const auto ct = block_info->continue_for_header; - if (ct == 0) { - continue; - } - // Furthermore, this is a loop header. - const auto* ct_info = GetBlockInfo(ct); - const auto ct_pos = ct_info->pos; - // Pos(H) <= Pos(CT(H)), with equality only for single-block loops. - if (header_info->is_single_block_loop && ct_pos != header_pos) { - Fail() << "Internal error: Single block loop. CT pos is not the " - "header pos. Should have already checked this"; - } - if (!header_info->is_single_block_loop && (ct_pos <= header_pos)) { - Fail() << "Loop header " << header - << " does not dominate its continue target " << ct; - } - // Pos(CT(H)) < Pos(M(H)) - // Note: When recording merges we made sure CT(H) != M(H) - if (merge_pos <= ct_pos) { - return Fail() << "Merge block " << merge - << " for loop headed at block " << header - << " appears at or before the loop's continue " - "construct headed by " - "block " - << ct; - } + const auto ct = block_info->continue_for_header; + if (ct == 0) { + continue; + } + // Furthermore, this is a loop header. + const auto* ct_info = GetBlockInfo(ct); + const auto ct_pos = ct_info->pos; + // Pos(H) <= Pos(CT(H)), with equality only for single-block loops. + if (header_info->is_single_block_loop && ct_pos != header_pos) { + Fail() << "Internal error: Single block loop. CT pos is not the " + "header pos. Should have already checked this"; + } + if (!header_info->is_single_block_loop && (ct_pos <= header_pos)) { + Fail() << "Loop header " << header + << " does not dominate its continue target " << ct; + } + // Pos(CT(H)) < Pos(M(H)) + // Note: When recording merges we made sure CT(H) != M(H) + if (merge_pos <= ct_pos) { + return Fail() << "Merge block " << merge << " for loop headed at block " + << header + << " appears at or before the loop's continue " + "construct headed by " + "block " + << ct; + } } return success(); } diff --git a/src/writer/spirv/builder.cc b/src/writer/spirv/builder.cc index a3a7e894b4..2a32c8e89d 100644 --- a/src/writer/spirv/builder.cc +++ b/src/writer/spirv/builder.cc @@ -84,6 +84,19 @@ uint32_t pipeline_stage_to_execution_model(ast::PipelineStage stage) { return model; } +// A terminator is anything which will case a SPIR-V terminator to be emitted. +// This means things like breaks, fallthroughs and continues which all emit an +// OpBranch or return for the OpReturn emission. +bool LastIsTerminator(const ast::StatementList& stmts) { + if (stmts.empty()) { + return false; + } + + auto* last = stmts.back().get(); + return last->IsBreak() || last->IsContinue() || last->IsReturn() || + last->IsKill() || last->IsFallthrough(); +} + } // namespace Builder::Builder(ast::Module* mod) : mod_(mod), scope_stack_({}) {} @@ -178,6 +191,24 @@ bool Builder::GenerateAssignStatement(ast::AssignmentStatement* assign) { return true; } +bool Builder::GenerateBreakStatement(ast::BreakStatement*) { + if (merge_stack_.empty()) { + error_ = "Attempted to break with a merge block"; + return false; + } + push_function_inst(spv::Op::OpBranch, {Operand::Int(merge_stack_.back())}); + return true; +} + +bool Builder::GenerateContinueStatement(ast::ContinueStatement*) { + if (continue_stack_.empty()) { + error_ = "Attempted to continue with a continue block"; + return false; + } + push_function_inst(spv::Op::OpBranch, {Operand::Int(continue_stack_.back())}); + return true; +} + bool Builder::GenerateEntryPoint(ast::EntryPoint* ep) { auto name = ep->name(); if (name.empty()) { @@ -988,12 +1019,19 @@ bool Builder::GenerateLoopStatement(ast::LoopStatement* stmt) { {Operand::Int(merge_block_id), Operand::Int(continue_block_id), Operand::Int(SpvLoopControlMaskNone)}); + continue_stack_.push_back(continue_block_id); + merge_stack_.push_back(merge_block_id); + push_function_inst(spv::Op::OpBranch, {Operand::Int(body_block_id)}); push_function_inst(spv::Op::OpLabel, {body_block}); if (!GenerateStatementList(stmt->body())) { return false; } - push_function_inst(spv::Op::OpBranch, {Operand::Int(continue_block_id)}); + + // We only branch if the last element of the body didn't already branch. + if (!LastIsTerminator(stmt->body())) { + push_function_inst(spv::Op::OpBranch, {Operand::Int(continue_block_id)}); + } push_function_inst(spv::Op::OpLabel, {continue_block}); if (!GenerateStatementList(stmt->continuing())) { @@ -1001,6 +1039,9 @@ bool Builder::GenerateLoopStatement(ast::LoopStatement* stmt) { } push_function_inst(spv::Op::OpBranch, {Operand::Int(loop_header_id)}); + merge_stack_.pop_back(); + continue_stack_.pop_back(); + push_function_inst(spv::Op::OpLabel, {merge_block}); return true; @@ -1019,6 +1060,12 @@ bool Builder::GenerateStatement(ast::Statement* stmt) { if (stmt->IsAssign()) { return GenerateAssignStatement(stmt->AsAssign()); } + if (stmt->IsBreak()) { + return GenerateBreakStatement(stmt->AsBreak()); + } + if (stmt->IsContinue()) { + return GenerateContinueStatement(stmt->AsContinue()); + } if (stmt->IsIf()) { return GenerateIfStatement(stmt->AsIf()); } diff --git a/src/writer/spirv/builder.h b/src/writer/spirv/builder.h index a425cb1915..32af780309 100644 --- a/src/writer/spirv/builder.h +++ b/src/writer/spirv/builder.h @@ -149,6 +149,14 @@ class Builder { /// @param assign the statement to generate /// @returns true if the statement was successfully generated bool GenerateAssignStatement(ast::AssignmentStatement* assign); + /// Generates a break statement + /// @param stmt the statement to generate + /// @returns true if the statement was successfully generated + bool GenerateBreakStatement(ast::BreakStatement* stmt); + /// Generates a continue statement + /// @param stmt the statement to generate + /// @returns true if the statement was successfully generated + bool GenerateContinueStatement(ast::ContinueStatement* stmt); /// Generates an entry point instruction /// @param ep the entry point /// @returns true if the instruction was generated, false otherwise @@ -308,6 +316,8 @@ class Builder { std::unordered_map const_to_id_; ScopeStack scope_stack_; std::unordered_map spirv_id_to_variable_; + std::vector merge_stack_; + std::vector continue_stack_; }; } // namespace spirv diff --git a/src/writer/spirv/builder_accessor_expression_test.cc b/src/writer/spirv/builder_accessor_expression_test.cc index 99f1f5c4f4..f09f0cff21 100644 --- a/src/writer/spirv/builder_accessor_expression_test.cc +++ b/src/writer/spirv/builder_accessor_expression_test.cc @@ -278,8 +278,8 @@ TEST_F(BuilderTest, MemberAccessor_Nested_WithAlias) { ast::type::AliasType alias("Inner", &inner_struct); ast::StructMemberList outer_members; - outer_members.push_back(std::make_unique( - "inner", &alias, std::move(decos))); + outer_members.push_back( + std::make_unique("inner", &alias, std::move(decos))); ast::type::StructType s_type(std::make_unique( ast::StructDecoration::kNone, std::move(outer_members))); diff --git a/src/writer/spirv/builder_loop_test.cc b/src/writer/spirv/builder_loop_test.cc index ec258d9e1b..6333fad522 100644 --- a/src/writer/spirv/builder_loop_test.cc +++ b/src/writer/spirv/builder_loop_test.cc @@ -16,6 +16,8 @@ #include "gtest/gtest.h" #include "src/ast/assignment_statement.h" +#include "src/ast/break_statement.h" +#include "src/ast/continue_statement.h" #include "src/ast/identifier_expression.h" #include "src/ast/int_literal.h" #include "src/ast/loop_statement.h" @@ -166,9 +168,73 @@ OpBranch %4 )"); } -TEST_F(BuilderTest, DISABLED_Loop_WithContinuing) {} +TEST_F(BuilderTest, Loop_WithContinue) { + // loop { + // continue; + // } + ast::StatementList body; + body.push_back(std::make_unique()); -TEST_F(BuilderTest, DISABLED_Loop_WithBreak) {} + ast::StatementList continuing; + ast::LoopStatement expr(std::move(body), std::move(continuing)); + + Context ctx; + ast::Module mod; + TypeDeterminer td(&ctx, &mod); + ASSERT_TRUE(td.DetermineResultType(&expr)) << td.error(); + + Builder b(&mod); + b.push_function(Function{}); + + EXPECT_TRUE(b.GenerateLoopStatement(&expr)) << b.error(); + EXPECT_EQ(DumpInstructions(b.functions()[0].instructions()), + R"(OpBranch %1 +%1 = OpLabel +OpLoopMerge %2 %3 None +OpBranch %4 +%4 = OpLabel +OpBranch %3 +%3 = OpLabel +OpBranch %1 +%2 = OpLabel +)"); +} + +TEST_F(BuilderTest, DISABLED_Loop_WithConditionalContinue) {} + +TEST_F(BuilderTest, Loop_WithBreak) { + // loop { + // break; + // } + ast::StatementList body; + body.push_back(std::make_unique()); + + ast::StatementList continuing; + ast::LoopStatement expr(std::move(body), std::move(continuing)); + + Context ctx; + ast::Module mod; + TypeDeterminer td(&ctx, &mod); + ASSERT_TRUE(td.DetermineResultType(&expr)) << td.error(); + + Builder b(&mod); + b.push_function(Function{}); + + EXPECT_TRUE(b.GenerateLoopStatement(&expr)) << b.error(); + EXPECT_EQ(DumpInstructions(b.functions()[0].instructions()), + R"(OpBranch %1 +%1 = OpLabel +OpLoopMerge %2 %3 None +OpBranch %4 +%4 = OpLabel +OpBranch %2 +%3 = OpLabel +OpBranch %1 +%2 = OpLabel +)"); +} + +TEST_F(BuilderTest, DISABLED_Loop_WithConditionalBreak) {} } // namespace } // namespace spirv