[spirv-reader] Support ifbreak with other forward edge
Add a guard variable for flow control within that if-selection.
Also, the premerge blocks are always surrounded by an if-selection,
to ensure we cause reconvergence at the end of the original if-selection
construct, just like in the original SPIR-V.
Bug: tint:3
Change-Id: I614c6840e539bf9a338058beb5b6f70484e3320a
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/23182
Reviewed-by: dan sinclair <dsinclair@google.com>
diff --git a/src/reader/spirv/function.cc b/src/reader/spirv/function.cc
index 75276f2..99b44d6 100644
--- a/src/reader/spirv/function.cc
+++ b/src/reader/spirv/function.cc
@@ -15,6 +15,7 @@
#include "src/reader/spirv/function.h"
#include <algorithm>
+#include <sstream>
#include <unordered_map>
#include <unordered_set>
#include <utility>
@@ -29,6 +30,7 @@
#include "src/ast/as_expression.h"
#include "src/ast/assignment_statement.h"
#include "src/ast/binary_expression.h"
+#include "src/ast/bool_literal.h"
#include "src/ast/break_statement.h"
#include "src/ast/call_expression.h"
#include "src/ast/case_statement.h"
@@ -45,6 +47,7 @@
#include "src/ast/sint_literal.h"
#include "src/ast/storage_class.h"
#include "src/ast/switch_statement.h"
+#include "src/ast/type/bool_type.h"
#include "src/ast/type/u32_type.h"
#include "src/ast/type_constructor_expression.h"
#include "src/ast/uint_literal.h"
@@ -438,6 +441,36 @@
StatementBlock{construct, end_id, action, ast::StatementList{}, nullptr});
}
+void FunctionEmitter::PushGuard(const std::string& guard_name,
+ uint32_t end_id) {
+ assert(!statements_stack_.empty());
+ assert(!guard_name.empty());
+ // Guard control flow by the guard variable. Introduce a new
+ // if-selection with a then-clause ending at the same block
+ // as the statement block at the top of the stack.
+ const auto& top = statements_stack_.back();
+ auto* const guard_stmt =
+ AddStatement(std::make_unique<ast::IfStatement>())->AsIf();
+ guard_stmt->set_condition(
+ std::make_unique<ast::IdentifierExpression>(guard_name));
+ PushNewStatementBlock(top.construct_, end_id,
+ [guard_stmt](StatementBlock* s) {
+ guard_stmt->set_body(std::move(s->statements_));
+ });
+}
+
+void FunctionEmitter::PushTrueGuard(uint32_t end_id) {
+ assert(!statements_stack_.empty());
+ const auto& top = statements_stack_.back();
+ auto* const guard_stmt =
+ AddStatement(std::make_unique<ast::IfStatement>())->AsIf();
+ guard_stmt->set_condition(MakeTrue());
+ PushNewStatementBlock(top.construct_, end_id,
+ [guard_stmt](StatementBlock* s) {
+ guard_stmt->set_body(std::move(s->statements_));
+ });
+}
+
const ast::StatementList& FunctionEmitter::ast_body() {
assert(!statements_stack_.empty());
return statements_stack_[0].statements_;
@@ -1141,10 +1174,13 @@
// There should only be one backedge per backedge block.
uint32_t num_backedges = 0;
- // Track destinations for normal forward edges, either kForward,
- // kCaseFallThrough, or kIfBreak. These count toward the need
- // to have a merge instruction.
+ // Track destinations for normal forward edges, either kForward
+ // or kCaseFallThroughkIfBreak. These count toward the need
+ // to have a merge instruction. We also track kIfBreak edges
+ // because when used with normal forward edges, we'll need
+ // to generate a flow guard variable.
std::vector<uint32_t> normal_forward_edges;
+ std::vector<uint32_t> if_break_edges;
if (successors.empty() && src_construct.enclosing_continue) {
// Kill and return are not allowed in a continue construct.
@@ -1263,10 +1299,12 @@
// The edge-kind has been finalized.
if ((edge_kind == EdgeKind::kForward) ||
- (edge_kind == EdgeKind::kCaseFallThrough) ||
- (edge_kind == EdgeKind::kIfBreak)) {
+ (edge_kind == EdgeKind::kCaseFallThrough)) {
normal_forward_edges.push_back(dest);
}
+ if (edge_kind == EdgeKind::kIfBreak) {
+ if_break_edges.push_back(dest);
+ }
if ((edge_kind == EdgeKind::kForward) ||
(edge_kind == EdgeKind::kCaseFallThrough)) {
@@ -1340,6 +1378,21 @@
<< ") but it is not a structured header (it has no merge "
"instruction)";
}
+ if ((normal_forward_edges.size() + if_break_edges.size() > 1) &&
+ (src_info->merge_for_header == 0)) {
+ // There is a branch to the merge of an if-selection combined
+ // with an other normal forward branch. Control within the
+ // if-selection needs to be gated by a flow predicate.
+ for (auto if_break_dest : if_break_edges) {
+ auto* head_info =
+ GetBlockInfo(GetBlockInfo(if_break_dest)->header_for_merge);
+ // Generate a guard name, but only once.
+ if (head_info->flow_guard_name.empty()) {
+ const std::string guard = "guard" + std::to_string(head_info->id);
+ head_info->flow_guard_name = namer_.MakeDerivedName(guard);
+ }
+ }
+ }
}
return success();
@@ -1777,9 +1830,21 @@
assert(construct->kind == Construct::kIfSelection);
assert(construct->begin_id == block_info.id);
+ const uint32_t true_head = block_info.true_head;
const uint32_t false_head = block_info.false_head;
const uint32_t premerge_head = block_info.premerge_head;
+ const std::string guard_name = block_info.flow_guard_name;
+ if (!guard_name.empty()) {
+ // Declare the guard variable just before the "if", initialized to true.
+ auto guard_var = std::make_unique<ast::Variable>(
+ guard_name, ast::StorageClass::kFunction, parser_impl_.BoolType());
+ guard_var->set_constructor(MakeTrue());
+ auto guard_decl =
+ std::make_unique<ast::VariableDeclStatement>(std::move(guard_var));
+ AddStatement(std::move(guard_decl));
+ }
+
auto* const if_stmt =
AddStatement(std::make_unique<ast::IfStatement>())->AsIf();
const auto condition_id =
@@ -1857,7 +1922,30 @@
// Blocks for the then-clause appear before blocks for the else-clause.
// So push the else-clause handling onto the stack first. The else-clause
// might be empty, but this works anyway.
+
+ // Handle the premerge, if it exists.
+ if (premerge_head) {
+ // The top of the stack is the statement block that is the parent of the
+ // if-statement. Adding statements now will place them after that 'if'.
+ if (guard_name.empty()) {
+ // We won't have a flow guard for the premerge.
+ // Insert a trivial if(true) { ... } around the blocks from the
+ // premerge head until the end of the if-selection. This is needed
+ // to ensure uniform reconvergence occurs at the end of the if-selection
+ // just like in the original SPIR-V.
+ PushTrueGuard(construct->end_id);
+ } else {
+ // Add a flow guard around the blocks in the premrege area.
+ PushGuard(guard_name, construct->end_id);
+ }
+ }
+
push_else();
+ if (true_head && false_head && !guard_name.empty()) {
+ // There are non-trivial then and else clauses.
+ // We have to guard the start of the else.
+ PushGuard(guard_name, else_end);
+ }
push_then();
}
@@ -2083,11 +2171,20 @@
// Emit an 'if' statement to express the *other* branch as a conditional
// break or continue. Either or both of these could be nullptr.
// (A nullptr is generated for kIfBreak, kForward, or kBack.)
- auto true_branch = MakeBranch(block_info, *true_info);
- auto false_branch = MakeBranch(block_info, *false_info);
+ // Also if one of the branches is an if-break out of an if-selection
+ // requiring a flow guard, then get that flow guard name too. It will
+ // come from at most one of these two branches.
+ std::string flow_guard;
+ auto true_branch =
+ MakeBranchDetailed(block_info, *true_info, false, &flow_guard);
+ auto false_branch =
+ MakeBranchDetailed(block_info, *false_info, false, &flow_guard);
AddStatement(MakeSimpleIf(std::move(cond), std::move(true_branch),
std::move(false_branch)));
+ if (!flow_guard.empty()) {
+ PushGuard(flow_guard, statements_stack_.back().end_id_);
+ }
return true;
}
case SpvOpSwitch:
@@ -2100,10 +2197,11 @@
return success();
}
-std::unique_ptr<ast::Statement> FunctionEmitter::MakeBranchInternal(
+std::unique_ptr<ast::Statement> FunctionEmitter::MakeBranchDetailed(
const BlockInfo& src_info,
const BlockInfo& dest_info,
- bool forced) const {
+ bool forced,
+ std::string* flow_guard_name_ptr) const {
auto kind = src_info.succ_edge.find(dest_info.id)->second;
switch (kind) {
case EdgeKind::kBack:
@@ -2148,10 +2246,23 @@
}
// Otherwise, emit a regular continue statement.
return std::make_unique<ast::ContinueStatement>();
- case EdgeKind::kIfBreak:
+ case EdgeKind::kIfBreak: {
+ const auto& flow_guard =
+ GetBlockInfo(dest_info.header_for_merge)->flow_guard_name;
+ if (!flow_guard.empty()) {
+ if (flow_guard_name_ptr != nullptr) {
+ *flow_guard_name_ptr = flow_guard;
+ }
+ // Signal an exit from the branch.
+ return std::make_unique<ast::AssignmentStatement>(
+ std::make_unique<ast::IdentifierExpression>(flow_guard),
+ MakeFalse());
+ }
+
// For an unconditional branch, the break out to an if-selection
// merge block is implicit.
break;
+ }
case EdgeKind::kCaseFallThrough:
return std::make_unique<ast::FallthroughStatement>();
case EdgeKind::kForward:
@@ -2686,6 +2797,17 @@
return current_expr;
}
+std::unique_ptr<ast::Expression> FunctionEmitter::MakeTrue() const {
+ return std::make_unique<ast::ScalarConstructorExpression>(
+ std::make_unique<ast::BoolLiteral>(parser_impl_.BoolType(), true));
+}
+
+std::unique_ptr<ast::Expression> FunctionEmitter::MakeFalse() const {
+ ast::type::BoolType bool_type;
+ return std::make_unique<ast::ScalarConstructorExpression>(
+ std::make_unique<ast::BoolLiteral>(parser_impl_.BoolType(), false));
+}
+
} // namespace spirv
} // namespace reader
} // namespace tint
diff --git a/src/reader/spirv/function.h b/src/reader/spirv/function.h
index 686c924..c321f88 100644
--- a/src/reader/spirv/function.h
+++ b/src/reader/spirv/function.h
@@ -138,16 +138,23 @@
/// construct for an OpBranchConditional instruction.
/// If not 0, then this block is an if-selection header, and |true_head| is
- /// the target id of the true branch on the OpBranchConditional.
+ /// the target id of the true branch on the OpBranchConditional, and that
+ /// target is inside the if-selection.
uint32_t true_head = 0;
/// If not 0, then this block is an if-selection header, and |false_head|
- /// is the target id of the false branch on the OpBranchConditional.
+ /// is the target id of the false branch on the OpBranchConditional, and
+ /// that target is inside the if-selection.
uint32_t false_head = 0;
/// If not 0, then this block is an if-selection header, and when following
/// the flow via the true and false branches, control first reconverges at
/// the block with ID |premerge_head|, and |premerge_head| is still inside
/// the if-selection.
uint32_t premerge_head = 0;
+ /// If non-empty, then this block is an if-selection header, and control flow
+ /// in the body must be guarded by a boolean flow variable with this name.
+ /// This occurs when a block in this selection has both an if-break edge, and
+ /// also a different normal forward edge but without a merge instruction.
+ std::string flow_guard_name = "";
};
inline std::ostream& operator<<(std::ostream& o, const BlockInfo& bi) {
@@ -156,7 +163,6 @@
<< " merge_for_header: " << bi.merge_for_header
<< " continue_for_header: " << bi.continue_for_header
<< " header_for_merge: " << bi.header_for_merge
- << " header_for_merge: " << bi.header_for_merge
<< " single_block_loop: " << int(bi.is_single_block_loop) << "}";
return o;
}
@@ -338,7 +344,7 @@
/// @returns the new statement, or a null statement
std::unique_ptr<ast::Statement> MakeBranch(const BlockInfo& src_info,
const BlockInfo& dest_info) const {
- return MakeBranchInternal(src_info, dest_info, false);
+ return MakeBranchDetailed(src_info, dest_info, false, nullptr);
}
/// Returns a new statement to represent the given branch representing a
@@ -350,7 +356,7 @@
std::unique_ptr<ast::Statement> MakeForcedBranch(
const BlockInfo& src_info,
const BlockInfo& dest_info) const {
- return MakeBranchInternal(src_info, dest_info, true);
+ return MakeBranchDetailed(src_info, dest_info, true, nullptr);
}
/// Returns a new statement to represent the given branch representing a
@@ -359,13 +365,19 @@
/// is false, this method tries to avoid emitting a 'break' statement when
/// that would be redundant in WGSL due to implicit breaking out of a switch.
/// When |forced| is true, the method won't try to avoid emitting that break.
+ /// If the control flow edge is an if-break for an if-selection with a
+ /// control flow guard, then return that guard name via |flow_guard_name_ptr|
+ /// when that parameter is not null.
/// @param src_info the source block
/// @param dest_info the destination block
/// @param forced if true, always emit the branch (if it exists in WGSL)
+ /// @param flow_guard_name_ptr return parameter for control flow guard name
/// @returns the new statement, or a null statement
- std::unique_ptr<ast::Statement> MakeBranchInternal(const BlockInfo& src_info,
- const BlockInfo& dest_info,
- bool forced) const;
+ std::unique_ptr<ast::Statement> MakeBranchDetailed(
+ const BlockInfo& src_info,
+ const BlockInfo& dest_info,
+ bool forced,
+ std::string* flow_guard_name_ptr) const;
/// Returns a new if statement with the given statements as the then-clause
/// and the else-clause. Either or both clauses might be nullptr. If both
@@ -530,6 +542,25 @@
uint32_t end_id,
CompletionAction action);
+ /// Emits an if-statement whose condition is the given flow guard
+ /// variable, and pushes onto the statement stack the corresponding
+ /// statement block ending (and not including) the given block.
+ /// @param flow_guard name of the flow guard variable
+ /// @param end_id first block after the if construct.
+ void PushGuard(const std::string& flow_guard, uint32_t end_id);
+
+ /// Emits an if-statement with 'true' condition, and pushes onto the
+ /// statement stack the corresponding statement block ending (and not
+ /// including) the given block.
+ /// @param end_id first block after the if construct.
+ void PushTrueGuard(uint32_t end_id);
+
+ /// @returns a boolean true expression.
+ std::unique_ptr<ast::Expression> MakeTrue() const;
+
+ /// @returns a boolean false expression.
+ std::unique_ptr<ast::Expression> MakeFalse() const;
+
ParserImpl& parser_impl_;
ast::Module& ast_module_;
spvtools::opt::IRContext& ir_context_;
diff --git a/src/reader/spirv/function_cfg_test.cc b/src/reader/spirv/function_cfg_test.cc
index ab1bf0f..6d11200 100644
--- a/src/reader/spirv/function_cfg_test.cc
+++ b/src/reader/spirv/function_cfg_test.cc
@@ -6991,11 +6991,7 @@
}
TEST_F(SpvParserTest, ClassifyCFGEdges_IfBreak_FromThen_ForwardWithinThen) {
- // Arguably SPIR-V allows this configuration. We're debating whether to ban
- // it.
- // TODO(dneto): We can make this case work, if we injected
- // if (!cond2) { rest-of-then-body }
- // at block 30
+ // SPIR-V allows this unusual configuration.
auto assembly = CommonTypes() + R"(
%100 = OpFunction %void None %voidfn
@@ -7016,7 +7012,7 @@
auto* p = parser(test::Assemble(assembly));
ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << p->error();
FunctionEmitter fe(p, *spirv_function(100));
- EXPECT_FALSE(FlowClassifyCFGEdges(&fe));
+ EXPECT_TRUE(FlowClassifyCFGEdges(&fe));
EXPECT_THAT(fe.block_order(), ElementsAre(10, 20, 80, 99));
auto* bi20 = fe.GetBlockInfo(20);
@@ -7026,17 +7022,11 @@
EXPECT_EQ(bi20->succ_edge.count(99), 1u);
EXPECT_EQ(bi20->succ_edge[99], EdgeKind::kIfBreak);
- EXPECT_THAT(p->error(),
- Eq("Control flow diverges at block 20 (to 99, 80) but it is not "
- "a structured header (it has no merge instruction)"));
+ EXPECT_THAT(p->error(), Eq(""));
}
TEST_F(SpvParserTest, ClassifyCFGEdges_IfBreak_FromElse_ForwardWithinElse) {
- // Arguably SPIR-V allows this configuration. We're debating whether to ban
- // it.
- // TODO(dneto): We can make this case work, if we injected
- // if (!cond2) { rest-of-else-body }
- // at block 30
+ // SPIR-V allows this unusual configuration.
auto assembly = CommonTypes() + R"(
%100 = OpFunction %void None %voidfn
@@ -7060,7 +7050,7 @@
auto* p = parser(test::Assemble(assembly));
ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << p->error();
FunctionEmitter fe(p, *spirv_function(100));
- EXPECT_FALSE(FlowClassifyCFGEdges(&fe));
+ EXPECT_TRUE(FlowClassifyCFGEdges(&fe));
EXPECT_THAT(fe.block_order(), ElementsAre(10, 20, 30, 80, 99));
auto* bi30 = fe.GetBlockInfo(30);
@@ -7070,12 +7060,10 @@
EXPECT_EQ(bi30->succ_edge.count(99), 1u);
EXPECT_EQ(bi30->succ_edge[99], EdgeKind::kIfBreak);
- EXPECT_THAT(p->error(),
- Eq("Control flow diverges at block 30 (to 99, 80) but it is not "
- "a structured header (it has no merge instruction)"));
+ EXPECT_THAT(p->error(), Eq(""));
}
-TEST_F(SpvParserTest, ClassifyCFGEdges_IfBreak_WithForwardToPremerge_IsError) {
+TEST_F(SpvParserTest, ClassifyCFGEdges_IfBreak_WithForwardToPremerge) {
auto assembly = CommonTypes() + R"(
%100 = OpFunction %void None %voidfn
@@ -7084,7 +7072,7 @@
OpBranchConditional %cond %20 %30
%20 = OpLabel ; then
- OpBranchConditional %cond2 %99 %80 ; break with forward to premerge is error
+ OpBranchConditional %cond2 %99 %80 ; break with forward to premerge
%30 = OpLabel ; else
OpBranch %80
@@ -7099,7 +7087,7 @@
auto* p = parser(test::Assemble(assembly));
ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << p->error();
FunctionEmitter fe(p, *spirv_function(100));
- EXPECT_FALSE(FlowClassifyCFGEdges(&fe));
+ EXPECT_TRUE(FlowClassifyCFGEdges(&fe));
EXPECT_THAT(fe.block_order(), ElementsAre(10, 20, 30, 80, 99));
auto* bi20 = fe.GetBlockInfo(20);
@@ -7109,9 +7097,7 @@
EXPECT_EQ(bi20->succ_edge.count(99), 1u);
EXPECT_EQ(bi20->succ_edge[99], EdgeKind::kIfBreak);
- EXPECT_THAT(p->error(),
- Eq("Control flow diverges at block 20 (to 99, 80) but it is not "
- "a structured header (it has no merge instruction)"));
+ EXPECT_THAT(p->error(), Eq(""));
}
TEST_F(SpvParserTest, FindIfSelectionInternalHeaders_DomViolation_Merge_CantBeTrueHeader) {
@@ -7204,10 +7190,9 @@
EXPECT_THAT(p->error(), Eq("Block 70 is the merge block for 50 but has alternate paths reaching it, starting from blocks 20 and 50 which are the true and false branches for the if-selection header block 10 (violates dominance rule)"));
}
-TEST_F(SpvParserTest, DISABLED_Codegen_IfBreak_FromThen_ForwardWithinThen) {
- // TODO(dneto): We can make this case work, if we injected
- // if (!cond2) { rest-of-then-body }
- // at block 30
+TEST_F(SpvParserTest, EmitBody_IfBreak_FromThen_ForwardWithinThen) {
+ // Exercises the hard case where we a single OpBranchConditional has both
+ // IfBreak and Forward edges, within the true-branch clause.
auto assembly = CommonTypes() + R"(
%100 = OpFunction %void None %voidfn
@@ -7237,15 +7222,87 @@
ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << p->error();
FunctionEmitter fe(p, *spirv_function(100));
EXPECT_TRUE(fe.EmitBody()) << p->error();
- EXPECT_THAT(ToString(fe.ast_body()), Eq(R"(something
+ EXPECT_THAT(ToString(fe.ast_body()), Eq(R"(Assignment{
+ Identifier{var}
+ ScalarConstructor{1}
+}
+VariableDeclStatement{
+ Variable{
+ guard10
+ function
+ __bool
+ {
+ ScalarConstructor{true}
+ }
+ }
+}
+If{
+ (
+ ScalarConstructor{false}
+ )
+ {
+ Assignment{
+ Identifier{var}
+ ScalarConstructor{2}
+ }
+ If{
+ (
+ ScalarConstructor{true}
+ )
+ {
+ Assignment{
+ Identifier{guard10}
+ ScalarConstructor{false}
+ }
+ }
+ }
+ If{
+ (
+ Identifier{guard10}
+ )
+ {
+ Assignment{
+ Identifier{var}
+ ScalarConstructor{3}
+ }
+ Assignment{
+ Identifier{guard10}
+ ScalarConstructor{false}
+ }
+ }
+ }
+ }
+}
+Else{
+ {
+ If{
+ (
+ Identifier{guard10}
+ )
+ {
+ Assignment{
+ Identifier{var}
+ ScalarConstructor{4}
+ }
+ Assignment{
+ Identifier{guard10}
+ ScalarConstructor{false}
+ }
+ }
+ }
+ }
+}
+Assignment{
+ Identifier{var}
+ ScalarConstructor{5}
+}
Return{}
)")) << ToString(fe.ast_body());
}
-TEST_F(SpvParserTest, DISABLED_Codegen_IfBreak_FromElse_ForwardWithinElse) {
- // TODO(dneto): We can make this case work, if we injected
- // if (!cond2) { rest-of-else-body }
- // at block 80
+TEST_F(SpvParserTest, EmitBody_IfBreak_FromElse_ForwardWithinElse) {
+ // Exercises the hard case where we a single OpBranchConditional has both
+ // IfBreak and Forward edges, within the false-branch clause.
auto assembly = CommonTypes() + R"(
%100 = OpFunction %void None %voidfn
@@ -7275,16 +7332,90 @@
ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << p->error();
FunctionEmitter fe(p, *spirv_function(100));
EXPECT_TRUE(fe.EmitBody()) << p->error();
- EXPECT_THAT(ToString(fe.ast_body()), Eq(R"(something
+ EXPECT_THAT(ToString(fe.ast_body()), Eq(R"(Assignment{
+ Identifier{var}
+ ScalarConstructor{1}
+}
+VariableDeclStatement{
+ Variable{
+ guard10
+ function
+ __bool
+ {
+ ScalarConstructor{true}
+ }
+ }
+}
+If{
+ (
+ ScalarConstructor{false}
+ )
+ {
+ Assignment{
+ Identifier{var}
+ ScalarConstructor{2}
+ }
+ Assignment{
+ Identifier{guard10}
+ ScalarConstructor{false}
+ }
+ }
+}
+Else{
+ {
+ If{
+ (
+ Identifier{guard10}
+ )
+ {
+ Assignment{
+ Identifier{var}
+ ScalarConstructor{3}
+ }
+ If{
+ (
+ ScalarConstructor{true}
+ )
+ {
+ Assignment{
+ Identifier{guard10}
+ ScalarConstructor{false}
+ }
+ }
+ }
+ If{
+ (
+ Identifier{guard10}
+ )
+ {
+ Assignment{
+ Identifier{var}
+ ScalarConstructor{4}
+ }
+ Assignment{
+ Identifier{guard10}
+ ScalarConstructor{false}
+ }
+ }
+ }
+ }
+ }
+ }
+}
+Assignment{
+ Identifier{var}
+ ScalarConstructor{5}
+}
Return{}
)")) << ToString(fe.ast_body());
}
-TEST_F(
- SpvParserTest,
- DISABLED_Codegen_IfBreak_FromThenWithForward_FromElseWithForward_AlsoPremerge) {
- // This is a combination of the previous two, but also adding a premrge and
- //
+TEST_F(SpvParserTest,
+ EmitBody_IfBreak_FromThenWithForward_FromElseWithForward_AlsoPremerge) {
+ // This is a combination of the previous two, but also adding a premerge.
+ // We have IfBreak and Forward edges from the same OpBranchConditional, and
+ // this occurs in the true-branch clause, the false-branch clause, and within
+ // the premerge clause. Flow guards have to be sprinkled in lots of places.
auto assembly = CommonTypes() + R"(
%100 = OpFunction %void None %voidfn
@@ -7298,27 +7429,27 @@
OpBranchConditional %cond2 %21 %99 ; kForward and kIfBreak
%21 = OpLabel ; still in then clause
- OpStore %var %uint_2
+ OpStore %var %uint_3
OpBranch %80 ; to premerge
%50 = OpLabel ; else clause
- OpStore %var %uint_3
+ OpStore %var %uint_4
OpBranchConditional %cond2 %99 %51 ; kIfBreak with kForward
%51 = OpLabel ; still in else clause
- OpStore %var %uint_4
+ OpStore %var %uint_5
OpBranch %80 ; to premerge
%80 = OpLabel ; premerge
- OpStore %var %uint_5
+ OpStore %var %uint_6
OpBranchConditional %cond3 %81 %99
%81 = OpLabel ; premerge
- OpStore %var %uint_6
+ OpStore %var %uint_7
OpBranch %99
%99 = OpLabel
- OpStore %var %uint_7
+ OpStore %var %uint_8
OpReturn
OpFunctionEnd
)";
@@ -7326,7 +7457,139 @@
ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << p->error();
FunctionEmitter fe(p, *spirv_function(100));
EXPECT_TRUE(fe.EmitBody()) << p->error() << assembly;
- EXPECT_THAT(ToString(fe.ast_body()), Eq(R"(something
+ EXPECT_THAT(ToString(fe.ast_body()), Eq(R"(Assignment{
+ Identifier{var}
+ ScalarConstructor{1}
+}
+VariableDeclStatement{
+ Variable{
+ guard10
+ function
+ __bool
+ {
+ ScalarConstructor{true}
+ }
+ }
+}
+If{
+ (
+ ScalarConstructor{false}
+ )
+ {
+ Assignment{
+ Identifier{var}
+ ScalarConstructor{2}
+ }
+ If{
+ (
+ ScalarConstructor{true}
+ )
+ {
+ }
+ }
+ Else{
+ {
+ Assignment{
+ Identifier{guard10}
+ ScalarConstructor{false}
+ }
+ }
+ }
+ If{
+ (
+ Identifier{guard10}
+ )
+ {
+ Assignment{
+ Identifier{var}
+ ScalarConstructor{3}
+ }
+ }
+ }
+ }
+}
+Else{
+ {
+ If{
+ (
+ Identifier{guard10}
+ )
+ {
+ Assignment{
+ Identifier{var}
+ ScalarConstructor{4}
+ }
+ If{
+ (
+ ScalarConstructor{true}
+ )
+ {
+ Assignment{
+ Identifier{guard10}
+ ScalarConstructor{false}
+ }
+ }
+ }
+ If{
+ (
+ Identifier{guard10}
+ )
+ {
+ Assignment{
+ Identifier{var}
+ ScalarConstructor{5}
+ }
+ }
+ }
+ }
+ }
+ }
+}
+If{
+ (
+ Identifier{guard10}
+ )
+ {
+ Assignment{
+ Identifier{var}
+ ScalarConstructor{6}
+ }
+ If{
+ (
+ ScalarConstructor{false}
+ )
+ {
+ }
+ }
+ Else{
+ {
+ Assignment{
+ Identifier{guard10}
+ ScalarConstructor{false}
+ }
+ }
+ }
+ If{
+ (
+ Identifier{guard10}
+ )
+ {
+ Assignment{
+ Identifier{var}
+ ScalarConstructor{7}
+ }
+ Assignment{
+ Identifier{guard10}
+ ScalarConstructor{false}
+ }
+ }
+ }
+ }
+}
+Assignment{
+ Identifier{var}
+ ScalarConstructor{8}
+}
Return{}
)")) << ToString(fe.ast_body());
}
@@ -7605,16 +7868,23 @@
}
}
}
-Assignment{
- Identifier{var}
- ScalarConstructor{3}
+If{
+ (
+ ScalarConstructor{true}
+ )
+ {
+ Assignment{
+ Identifier{var}
+ ScalarConstructor{3}
+ }
+ }
}
Assignment{
Identifier{var}
ScalarConstructor{999}
}
Return{}
-)"));
+)")) << ToString(fe.ast_body());
}
TEST_F(SpvParserTest, EmitBody_If_Then_Premerge) {
@@ -7660,16 +7930,23 @@
}
}
}
-Assignment{
- Identifier{var}
- ScalarConstructor{3}
+If{
+ (
+ ScalarConstructor{true}
+ )
+ {
+ Assignment{
+ Identifier{var}
+ ScalarConstructor{3}
+ }
+ }
}
Assignment{
Identifier{var}
ScalarConstructor{999}
}
Return{}
-)"));
+)")) << ToString(fe.ast_body());
}
TEST_F(SpvParserTest, EmitBody_If_Else_Premerge) {
@@ -7719,16 +7996,23 @@
}
}
}
-Assignment{
- Identifier{var}
- ScalarConstructor{3}
+If{
+ (
+ ScalarConstructor{true}
+ )
+ {
+ Assignment{
+ Identifier{var}
+ ScalarConstructor{3}
+ }
+ }
}
Assignment{
Identifier{var}
ScalarConstructor{999}
}
Return{}
-)"));
+)")) << ToString(fe.ast_body());
}
TEST_F(SpvParserTest, EmitBody_If_Nest_If) {
diff --git a/src/reader/spirv/parser_impl.cc b/src/reader/spirv/parser_impl.cc
index f778c61..870bd79 100644
--- a/src/reader/spirv/parser_impl.cc
+++ b/src/reader/spirv/parser_impl.cc
@@ -192,6 +192,7 @@
: Reader(ctx),
spv_binary_(spv_binary),
fail_stream_(&success_, &errors_),
+ bool_type_(ctx->type_mgr().Get(std::make_unique<ast::type::BoolType>())),
namer_(fail_stream_),
enum_converter_(fail_stream_),
tools_context_(kTargetEnv),
@@ -282,7 +283,7 @@
case spvtools::opt::analysis::Type::kVoid:
return save(ctx_.type_mgr().Get(std::make_unique<ast::type::VoidType>()));
case spvtools::opt::analysis::Type::kBool:
- return save(ctx_.type_mgr().Get(std::make_unique<ast::type::BoolType>()));
+ return save(bool_type_);
case spvtools::opt::analysis::Type::kInteger:
return save(ConvertType(spirv_type->AsInteger()));
case spvtools::opt::analysis::Type::kFloat:
diff --git a/src/reader/spirv/parser_impl.h b/src/reader/spirv/parser_impl.h
index f7330a8..49003e7 100644
--- a/src/reader/spirv/parser_impl.h
+++ b/src/reader/spirv/parser_impl.h
@@ -280,6 +280,9 @@
ast::type::Type* ForcedResultType(SpvOp op,
ast::type::Type* first_operand_type);
+ /// @returns the registered boolean type.
+ ast::type::Type* BoolType() const { return bool_type_; }
+
private:
/// Converts a specific SPIR-V type to a Tint type. Integer case
ast::type::Type* ConvertType(const spvtools::opt::analysis::Integer* int_ty);
@@ -313,6 +316,9 @@
FailStream fail_stream_;
spvtools::MessageConsumer message_consumer_;
+ // The registered boolean type.
+ ast::type::Type* bool_type_;
+
// An object used to store and generate names for SPIR-V objects.
Namer namer_;
// An object used to convert SPIR-V enums to Tint enums
