[spirv-reader] Emit break-if as needed.
This CL updates the SPIRV-Reader to emit `break-if` nodes instead of
`if-break` statements.
Bug: tint:1724
Change-Id: I8cd568f5e90a950acc5a42a470345273a5f1e6bc
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/111103
Commit-Queue: Dan Sinclair <dsinclair@chromium.org>
Reviewed-by: David Neto <dneto@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
diff --git a/src/tint/reader/spirv/function.cc b/src/tint/reader/spirv/function.cc
index b344246..f97d169 100644
--- a/src/tint/reader/spirv/function.cc
+++ b/src/tint/reader/spirv/function.cc
@@ -3210,8 +3210,8 @@
return false;
}
AddStatement(create<ast::ReturnStatement>(Source{}, value.expr));
- }
return true;
+ }
case spv::Op::OpKill:
// For now, assume SPIR-V OpKill has same semantics as WGSL discard.
// TODO(dneto): https://github.com/gpuweb/gpuweb/issues/676
@@ -3266,21 +3266,58 @@
return Fail() << "Fallthrough not supported in WGSL";
}
+ // In the case of a continuing block a `break-if` needs to be emitted for either an
+ // if-break or an if-else-break statement. This only happens inside the continue block.
+ // It's possible for a continue block to also be the loop block, so checks are needed
+ // that this is a continue construct and the header construct will cause a continuing
+ // construct to be emitted. (i.e. the header is not `continue is entire loop`.
+ bool needs_break_if = false;
+ if ((true_kind == EdgeKind::kLoopBreak || false_kind == EdgeKind::kLoopBreak) &&
+ block_info.construct && block_info.construct->kind == Construct::Kind::kContinue) {
+ auto* header = GetBlockInfo(block_info.construct->begin_id);
+
+ TINT_ASSERT(Reader, header->construct &&
+ header->construct->kind == Construct::Kind::kContinue);
+ if (!header->is_continue_entire_loop) {
+ needs_break_if = true;
+ }
+ }
+
// At this point, at most one edge is kForward or kIfBreak.
- // 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.)
- // 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, &flow_guard);
- auto* false_branch = MakeBranchDetailed(block_info, *false_info, &flow_guard);
+ // If this is a continuing block and a `break` is to be emitted, then this needs to be
+ // converted to a `break-if`. This may involve inverting the condition if this was a
+ // `break-unless`.
+ if (needs_break_if) {
+ if (true_kind == EdgeKind::kLoopBreak && false_kind == EdgeKind::kLoopBreak) {
+ // Both branches break ... ?
+ return Fail() << "Both branches of if inside continuing break.";
+ }
- AddStatement(MakeSimpleIf(cond, true_branch, false_branch));
- if (!flow_guard.empty()) {
- PushGuard(flow_guard, statements_stack_.Back().GetEndId());
+ if (true_kind == EdgeKind::kLoopBreak) {
+ AddStatement(create<ast::BreakIfStatement>(Source{}, cond));
+ } else {
+ AddStatement(create<ast::BreakIfStatement>(
+ Source{},
+ create<ast::UnaryOpExpression>(Source{}, ast::UnaryOp::kNot, cond)));
+ }
+ return true;
+
+ } else {
+ // 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.)
+ // 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, &flow_guard);
+ auto* false_branch = MakeBranchDetailed(block_info, *false_info, &flow_guard);
+
+ AddStatement(MakeSimpleIf(cond, true_branch, false_branch));
+ if (!flow_guard.empty()) {
+ PushGuard(flow_guard, statements_stack_.Back().GetEndId());
+ }
}
return true;
}
diff --git a/src/tint/reader/spirv/function_cfg_test.cc b/src/tint/reader/spirv/function_cfg_test.cc
index 88dc78e..c5800fb 100644
--- a/src/tint/reader/spirv/function_cfg_test.cc
+++ b/src/tint/reader/spirv/function_cfg_test.cc
@@ -638,9 +638,8 @@
EXPECT_FALSE(bi99->is_continue_entire_loop);
}
-TEST_F(
- SpvParserCFGTest,
- RegisterMerges_GoodLoopMerge_MultiBlockLoop_ContinueIsNotHeader_BranchConditional) { // NOLINT
+TEST_F(SpvParserCFGTest,
+ RegisterMerges_GoodLoopMerge_MultiBlockLoop_ContinueIsNotHeader_BranchConditional) {
auto p = parser(test::Assemble(CommonTypes() + R"(
%100 = OpFunction %void None %voidfn
@@ -2605,7 +2604,7 @@
}
TEST_F(SpvParserCFGTest,
- VerifyHeaderContinueMergeOrder_HeaderDoesNotStrictlyDominateContinueTarget) { // NOLINT
+ VerifyHeaderContinueMergeOrder_HeaderDoesNotStrictlyDominateContinueTarget) {
auto assembly = CommonTypes() + R"(
%100 = OpFunction %void None %voidfn
@@ -4451,9 +4450,8 @@
EXPECT_EQ(bi40->succ_edge[20], EdgeKind::kBack);
}
-TEST_F(
- SpvParserCFGTest,
- ClassifyCFGEdges_BackEdge_MultiBlockLoop_MultiBlockContinueConstruct_ContinueIsNotHeader) { // NOLINT
+TEST_F(SpvParserCFGTest,
+ ClassifyCFGEdges_BackEdge_MultiBlockLoop_MultiBlockContinueConstruct_ContinueIsNotHeader) {
auto assembly = CommonTypes() + R"(
%100 = OpFunction %void None %voidfn
@@ -4489,9 +4487,8 @@
EXPECT_EQ(bi50->succ_edge[20], EdgeKind::kBack);
}
-TEST_F(
- SpvParserCFGTest,
- ClassifyCFGEdges_BackEdge_MultiBlockLoop_MultiBlockContinueConstruct_ContinueIsHeader) { // NOLINT
+TEST_F(SpvParserCFGTest,
+ ClassifyCFGEdges_BackEdge_MultiBlockLoop_MultiBlockContinueConstruct_ContinueIsHeader) {
auto assembly = CommonTypes() + R"(
%100 = OpFunction %void None %voidfn
@@ -6051,7 +6048,7 @@
}
TEST_F(SpvParserCFGTest,
- FindSwitchCaseHeaders_DomViolation_SwitchCase_CantBeMergeForOtherConstruct) { // NOLINT
+ FindSwitchCaseHeaders_DomViolation_SwitchCase_CantBeMergeForOtherConstruct) {
auto assembly = CommonTypes() + R"(
%100 = OpFunction %void None %voidfn
@@ -6762,8 +6759,7 @@
}
TEST_F(SpvParserCFGTest,
- FindIfSelectionInternalHeaders_DomViolation_InteriorMerge_CantBeTrueHeader) { // NOLINT -
- // line length
+ FindIfSelectionInternalHeaders_DomViolation_InteriorMerge_CantBeTrueHeader) {
auto assembly = CommonTypes() + R"(
%100 = OpFunction %void None %voidfn
@@ -6795,10 +6791,8 @@
"merge block for header block 20 (violates dominance rule)"));
}
-TEST_F(
- SpvParserCFGTest,
- FindIfSelectionInternalHeaders_DomViolation_InteriorMerge_CantBeFalseHeader) { // NOLINT - line
- // length
+TEST_F(SpvParserCFGTest,
+ FindIfSelectionInternalHeaders_DomViolation_InteriorMerge_CantBeFalseHeader) {
auto assembly = CommonTypes() + R"(
%100 = OpFunction %void None %voidfn
@@ -9474,9 +9468,7 @@
}
TEST_F(SpvParserCFGTest,
- EmitBody_Branch_LoopBreak_MultiBlockLoop_FromContinueConstructEnd_Unconditional) { // NOLINT
- // - line
- // length
+ EmitBody_Branch_LoopBreak_MultiBlockLoop_FromContinueConstructEnd_Unconditional) {
auto p = parser(test::Assemble(CommonTypes() + R"(
%100 = OpFunction %void None %voidfn
@@ -9519,10 +9511,46 @@
ASSERT_EQ(expect, got);
}
-TEST_F(
- SpvParserCFGTest,
- EmitBody_Branch_LoopBreak_MultiBlockLoop_FromContinueConstructEnd_Conditional) { // NOLINT -
- // line length
+TEST_F(SpvParserCFGTest,
+ EmitBody_Branch_LoopBreak_MultiBlockLoop_FromContinueConstructEnd_Conditional_BreakIf) {
+ auto p = parser(test::Assemble(CommonTypes() + R"(
+ %100 = OpFunction %void None %voidfn
+
+ %10 = OpLabel
+ OpBranch %20
+
+ %20 = OpLabel
+ OpLoopMerge %99 %80 None
+ OpBranch %80
+
+ %80 = OpLabel ; continue target
+ OpStore %var %uint_1
+ OpBranchConditional %cond %99 %20 ; exit, and backedge
+
+ %99 = OpLabel
+ OpReturn
+
+ OpFunctionEnd
+ )"));
+ ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << p->error();
+ auto fe = p->function_emitter(100);
+ EXPECT_TRUE(fe.EmitBody()) << p->error();
+ auto ast_body = fe.ast_body();
+ auto got = test::ToString(p->program(), ast_body);
+ auto* expect = R"(loop {
+
+ continuing {
+ var_1 = 1u;
+ break if false;
+ }
+}
+return;
+)";
+ ASSERT_EQ(expect, got);
+}
+
+TEST_F(SpvParserCFGTest,
+ EmitBody_Branch_LoopBreak_MultiBlockLoop_FromContinueConstructEnd_Conditional) {
auto p = parser(test::Assemble(CommonTypes() + R"(
%100 = OpFunction %void None %voidfn
@@ -9551,10 +9579,48 @@
continuing {
var_1 = 1u;
- if (false) {
- } else {
- break;
- }
+ break if !(false);
+ }
+}
+return;
+)";
+ ASSERT_EQ(expect, got);
+}
+
+TEST_F(SpvParserCFGTest, EmitBody_Branch_LoopBreak_FromContinueConstructTail) {
+ auto p = parser(test::Assemble(CommonTypes() + R"(
+ %100 = OpFunction %void None %voidfn
+
+ %10 = OpLabel
+ OpBranch %20
+
+ %20 = OpLabel
+ OpLoopMerge %99 %50 None
+ OpBranchConditional %cond %30 %99
+ %30 = OpLabel
+ OpBranch %50
+ %50 = OpLabel
+ OpBranch %60
+ %60 = OpLabel
+ OpBranchConditional %cond %20 %99
+ %99 = OpLabel
+ OpReturn
+
+ OpFunctionEnd
+ )"));
+ ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << p->error();
+ auto fe = p->function_emitter(100);
+ EXPECT_TRUE(fe.EmitBody()) << p->error();
+ auto ast_body = fe.ast_body();
+ auto got = test::ToString(p->program(), ast_body);
+ auto* expect = R"(loop {
+ if (false) {
+ } else {
+ break;
+ }
+
+ continuing {
+ break if !(false);
}
}
return;
@@ -10009,9 +10075,7 @@
var_1 = 1u;
continuing {
- if (false) {
- break;
- }
+ break if false;
}
}
var_1 = 5u;
@@ -10052,10 +10116,7 @@
var_1 = 1u;
continuing {
- if (false) {
- } else {
- break;
- }
+ break if !(false);
}
}
var_1 = 5u;
@@ -10878,9 +10939,8 @@
ASSERT_EQ(expect, got);
}
-TEST_F(
- SpvParserCFGTest,
- EmitBody_BranchConditional_Continue_Continue_AfterHeader_Conditional_EmptyContinuing) { // NOLINT
+TEST_F(SpvParserCFGTest,
+ EmitBody_BranchConditional_Continue_Continue_AfterHeader_Conditional_EmptyContinuing) {
// Like the previous tests, but with an empty continuing clause.
auto p = parser(test::Assemble(CommonTypes() + R"(
%100 = OpFunction %void None %voidfn
diff --git a/src/tint/reader/spirv/function_var_test.cc b/src/tint/reader/spirv/function_var_test.cc
index 5b83105..1119667 100644
--- a/src/tint/reader/spirv/function_var_test.cc
+++ b/src/tint/reader/spirv/function_var_test.cc
@@ -689,9 +689,7 @@
continuing {
x_1 = 4u;
- if (false) {
- break;
- }
+ break if false;
}
}
x_1 = 5u;
@@ -1610,9 +1608,7 @@
x_999 = false;
continuing {
- if (true) {
- break;
- }
+ break if true;
}
}
}
@@ -1694,9 +1690,7 @@
x_999 = false;
continuing {
- if (true) {
- break;
- }
+ break if true;
}
}
diff --git a/test/tint/vk-gl-cts/graphicsfuzz/write-red-after-search/0-opt.wgsl b/test/tint/vk-gl-cts/graphicsfuzz/write-red-after-search/0-opt.wgsl
index e235b6e..0528f31 100644
--- a/test/tint/vk-gl-cts/graphicsfuzz/write-red-after-search/0-opt.wgsl
+++ b/test/tint/vk-gl-cts/graphicsfuzz/write-red-after-search/0-opt.wgsl
@@ -103,10 +103,7 @@
continuing {
let x_382 : f32 = x_27.injectionSwitch.x;
let x_384 : f32 = x_27.injectionSwitch.y;
- if ((x_382 > x_384)) {
- } else {
- break;
- }
+ break if !(x_382 > x_384);
}
}
continue;
