From a8bc2962598a16b369840059425754da3c530522 Mon Sep 17 00:00:00 2001 From: dan sinclair Date: Wed, 23 Nov 2022 01:51:24 +0000 Subject: [PATCH] [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 Reviewed-by: David Neto Kokoro: Kokoro --- src/tint/reader/spirv/function.cc | 63 ++++++-- src/tint/reader/spirv/function_cfg_test.cc | 136 +++++++++++++----- src/tint/reader/spirv/function_var_test.cc | 12 +- .../write-red-after-search/0-opt.wgsl | 5 +- 4 files changed, 152 insertions(+), 64 deletions(-) diff --git a/src/tint/reader/spirv/function.cc b/src/tint/reader/spirv/function.cc index b3442467fd..f97d169c35 100644 --- a/src/tint/reader/spirv/function.cc +++ b/src/tint/reader/spirv/function.cc @@ -3210,8 +3210,8 @@ bool FunctionEmitter::EmitNormalTerminator(const BlockInfo& block_info) { return false; } AddStatement(create(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 @@ bool FunctionEmitter::EmitNormalTerminator(const BlockInfo& block_info) { 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(Source{}, cond)); + } else { + AddStatement(create( + Source{}, + create(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 88dc78eba1..c5800fb539 100644 --- a/src/tint/reader/spirv/function_cfg_test.cc +++ b/src/tint/reader/spirv/function_cfg_test.cc @@ -638,9 +638,8 @@ TEST_F(SpvParserCFGTest, RegisterMerges_GoodLoopMerge_MultiBlockLoop_ContinueIsN 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_HeaderDoesNotStrictlyDom } TEST_F(SpvParserCFGTest, - VerifyHeaderContinueMergeOrder_HeaderDoesNotStrictlyDominateContinueTarget) { // NOLINT + VerifyHeaderContinueMergeOrder_HeaderDoesNotStrictlyDominateContinueTarget) { auto assembly = CommonTypes() + R"( %100 = OpFunction %void None %voidfn @@ -4451,9 +4450,8 @@ TEST_F(SpvParserCFGTest, ClassifyCFGEdges_BackEdge_MultiBlockLoop_SingleBlockCon 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 @@ TEST_F( 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, ClassifyCFGEdges_DomViolation_AfterContinueToContinueIn } 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, ClassifyCFGEdges_IfBreak_WithForwardToPremerge) { } 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 @@ TEST_F(SpvParserCFGTest, "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, } 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 @@ return; 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 @@ TEST_F( 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 @@ loop { var_1 = 1u; continuing { - if (false) { - break; - } + break if false; } } var_1 = 5u; @@ -10052,10 +10116,7 @@ loop { var_1 = 1u; continuing { - if (false) { - } else { - break; - } + break if !(false); } } var_1 = 5u; @@ -10878,9 +10939,8 @@ return; 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 5b8310591a..1119667404 100644 --- a/src/tint/reader/spirv/function_var_test.cc +++ b/src/tint/reader/spirv/function_var_test.cc @@ -689,9 +689,7 @@ loop { continuing { x_1 = 4u; - if (false) { - break; - } + break if false; } } x_1 = 5u; @@ -1610,9 +1608,7 @@ OpFunctionEnd x_999 = false; continuing { - if (true) { - break; - } + break if true; } } } @@ -1694,9 +1690,7 @@ loop { 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 e235b6e2c7..0528f31542 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 @@ fn insert_i1_i1_(treeIndex : ptr, data_1 : ptr) { 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;