diff --git a/src/reader/spirv/function.cc b/src/reader/spirv/function.cc index d5769351ee..7ce03edce5 100644 --- a/src/reader/spirv/function.cc +++ b/src/reader/spirv/function.cc @@ -1358,7 +1358,7 @@ bool FunctionEmitter::FindSwitchCaseHeaders() { if (construct->begin_pos >= default_block->pos) { // An OpSwitch must dominate its cases. Also, it can't be a self-loop // as that would be a backedge, and backedges can only target a loop, - // and loops use an OpLoopMerge instruction, which can't preceded an + // and loops use an OpLoopMerge instruction, which can't precede an // OpSwitch. return Fail() << "Switch branch from block " << construct->begin_id << " to default target block " << default_id @@ -1553,7 +1553,7 @@ bool FunctionEmitter::ClassifyCFGEdges() { uint32_t num_backedges = 0; // Track destinations for normal forward edges, either kForward - // or kCaseFallThroughkIfBreak. These count toward the need + // or kCaseFallThrough. 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. @@ -1797,6 +1797,15 @@ bool FunctionEmitter::FindIfSelectionInternalHeaders() { const bool contains_true = construct->ContainsPos(true_head_pos); const bool contains_false = construct->ContainsPos(false_head_pos); + // The cases for each edge are: + // - kBack: invalid because it's an invalid exit from the selection + // - kSwitchBreak + // - kLoopBreak + // - kLoopContinue + // - kIfBreak; normal case, may require a guard variable. + // - kFallThrough; invalid exit from the selection + // - kForward; normal case + if (contains_true) { if_header_info->true_head = true_head; } @@ -1804,11 +1813,12 @@ bool FunctionEmitter::FindIfSelectionInternalHeaders() { if_header_info->false_head = false_head; } - if ((true_head_info->header_for_merge != 0) && + if (contains_true && (true_head_info->header_for_merge != 0) && (true_head_info->header_for_merge != construct->begin_id)) { // The OpBranchConditional instruction for the true head block is an - // alternate path to the merge block, and hence the merge block is not - // dominated by its own (different) header. + // alternate path to the merge block of a construct nested inside the + // selection, and hence the merge block is not dominated by its own + // (different) header. return Fail() << "Block " << true_head << " is the true branch for if-selection header " << construct->begin_id @@ -1816,11 +1826,12 @@ bool FunctionEmitter::FindIfSelectionInternalHeaders() { << true_head_info->header_for_merge << " (violates dominance rule)"; } - if ((false_head_info->header_for_merge != 0) && + if (contains_false && (false_head_info->header_for_merge != 0) && (false_head_info->header_for_merge != construct->begin_id)) { // The OpBranchConditional instruction for the false head block is an - // alternate path to the merge block, and hence the merge block is not - // dominated by its own (different) header. + // alternate path to the merge block of a construct nested inside the + // selection, and hence the merge block is not dominated by its own + // (different) header. return Fail() << "Block " << false_head << " is the false branch for if-selection header " << construct->begin_id diff --git a/src/reader/spirv/function_cfg_test.cc b/src/reader/spirv/function_cfg_test.cc index 62ef82129b..05a1fff4a4 100644 --- a/src/reader/spirv/function_cfg_test.cc +++ b/src/reader/spirv/function_cfg_test.cc @@ -7220,8 +7220,9 @@ TEST_F(SpvParserTest, ClassifyCFGEdges_IfBreak_WithForwardToPremerge) { EXPECT_THAT(p->error(), Eq("")); } -TEST_F(SpvParserTest, - FindIfSelectionInternalHeaders_DomViolation_Merge_CantBeTrueHeader) { +TEST_F( + SpvParserTest, + FindIfSelectionInternalHeaders_DomViolation_InteriorMerge_CantBeTrueHeader) { auto assembly = CommonTypes() + R"( %100 = OpFunction %void None %voidfn @@ -7252,8 +7253,9 @@ TEST_F(SpvParserTest, "merge block for header block 20 (violates dominance rule)")); } -TEST_F(SpvParserTest, - FindIfSelectionInternalHeaders_DomViolation_Merge_CantBeFalseHeader) { +TEST_F( + SpvParserTest, + FindIfSelectionInternalHeaders_DomViolation_InteriorMerge_CantBeFalseHeader) { auto assembly = CommonTypes() + R"( %100 = OpFunction %void None %voidfn @@ -7284,8 +7286,9 @@ TEST_F(SpvParserTest, "merge block for header block 20 (violates dominance rule)")); } -TEST_F(SpvParserTest, - FindIfSelectionInternalHeaders_DomViolation_Merge_CantBePremerge) { +TEST_F( + SpvParserTest, + FindIfSelectionInternalHeaders_DomViolation_InteriorMerge_CantBePremerge) { auto assembly = CommonTypes() + R"( %100 = OpFunction %void None %voidfn @@ -7323,6 +7326,208 @@ TEST_F(SpvParserTest, "(violates dominance rule)")); } +TEST_F(SpvParserTest, FindIfSelectionInternalHeaders_TrueBranch_LoopBreak_Ok) { + // crbug.com/tint/243 + auto assembly = CommonTypes() + R"( + %100 = OpFunction %void None %voidfn + + %5 = OpLabel + OpBranch %10 + + %10 = OpLabel + OpLoopMerge %99 %90 None + OpBranch %20 + + %20 = OpLabel + OpSelectionMerge %40 None + OpBranchConditional %cond %99 %30 ; true branch breaking is ok + + %30 = OpLabel + OpBranch %40 + + %40 = OpLabel ; selection merge + OpBranch %90 + + %90 = OpLabel ; continue target + OpBranch %10 ; backedge + + %99 = OpLabel ; loop merge + OpReturn +)"; + auto p = parser(test::Assemble(assembly)); + ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << p->error(); + FunctionEmitter fe(p.get(), *spirv_function(p.get(), 100)); + EXPECT_TRUE(FlowFindIfSelectionInternalHeaders(&fe)); + EXPECT_THAT(p->error(), Eq("")); +} + +TEST_F(SpvParserTest, + FindIfSelectionInternalHeaders_TrueBranch_LoopContinue_Ok) { + // crbug.com/tint/243 + auto assembly = CommonTypes() + R"( + %100 = OpFunction %void None %voidfn + + %5 = OpLabel + OpBranch %10 + + %10 = OpLabel + OpLoopMerge %99 %90 None + OpBranch %20 + + %20 = OpLabel + OpSelectionMerge %40 None + OpBranchConditional %cond %90 %30 ; true branch continue is ok + + %30 = OpLabel + OpBranch %40 + + %40 = OpLabel ; selection merge + OpBranch %90 + + %90 = OpLabel ; continue target + OpBranch %10 ; backedge + + %99 = OpLabel ; loop merge + OpReturn +)"; + auto p = parser(test::Assemble(assembly)); + ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << p->error(); + FunctionEmitter fe(p.get(), *spirv_function(p.get(), 100)); + EXPECT_TRUE(FlowFindIfSelectionInternalHeaders(&fe)); + EXPECT_THAT(p->error(), Eq("")); +} + +TEST_F(SpvParserTest, + FindIfSelectionInternalHeaders_TrueBranch_SwitchBreak_Ok) { + // crbug.com/tint/243 + auto assembly = CommonTypes() + R"( + %100 = OpFunction %void None %voidfn + + %10 = OpLabel + OpSelectionMerge %99 None + OpSwitch %uint_20 %99 20 %20 + + %20 = OpLabel + OpSelectionMerge %40 None + OpBranchConditional %cond %99 %30 ; true branch switch break is ok + + %30 = OpLabel + OpBranch %40 + + %40 = OpLabel ; if-selection merge + OpBranch %99 + + %99 = OpLabel ; switch merge + OpReturn +)"; + auto p = parser(test::Assemble(assembly)); + ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << p->error(); + FunctionEmitter fe(p.get(), *spirv_function(p.get(), 100)); + EXPECT_TRUE(FlowFindIfSelectionInternalHeaders(&fe)); + EXPECT_THAT(p->error(), Eq("")); +} + +TEST_F(SpvParserTest, FindIfSelectionInternalHeaders_FalseBranch_LoopBreak_Ok) { + // crbug.com/tint/243 + auto assembly = CommonTypes() + R"( + %100 = OpFunction %void None %voidfn + + %5 = OpLabel + OpBranch %10 + + %10 = OpLabel + OpLoopMerge %99 %90 None + OpBranch %20 + + %20 = OpLabel + OpSelectionMerge %40 None + OpBranchConditional %cond %30 %99 ; false branch breaking is ok + + %30 = OpLabel + OpBranch %40 + + %40 = OpLabel ; selection merge + OpBranch %90 + + %90 = OpLabel ; continue target + OpBranch %10 ; backedge + + %99 = OpLabel ; loop merge + OpReturn +)"; + auto p = parser(test::Assemble(assembly)); + ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << p->error(); + FunctionEmitter fe(p.get(), *spirv_function(p.get(), 100)); + EXPECT_TRUE(FlowFindIfSelectionInternalHeaders(&fe)); + EXPECT_THAT(p->error(), Eq("")); +} + +TEST_F(SpvParserTest, + FindIfSelectionInternalHeaders_FalseBranch_LoopContinue_Ok) { + // crbug.com/tint/243 + auto assembly = CommonTypes() + R"( + %100 = OpFunction %void None %voidfn + + %5 = OpLabel + OpBranch %10 + + %10 = OpLabel + OpLoopMerge %99 %90 None + OpBranch %20 + + %20 = OpLabel + OpSelectionMerge %40 None + OpBranchConditional %cond %30 %90 ; false branch continue is ok + + %30 = OpLabel + OpBranch %40 + + %40 = OpLabel ; selection merge + OpBranch %90 + + %90 = OpLabel ; continue target + OpBranch %10 ; backedge + + %99 = OpLabel ; loop merge + OpReturn +)"; + auto p = parser(test::Assemble(assembly)); + ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << p->error(); + FunctionEmitter fe(p.get(), *spirv_function(p.get(), 100)); + EXPECT_TRUE(FlowFindIfSelectionInternalHeaders(&fe)); + EXPECT_THAT(p->error(), Eq("")); +} + +TEST_F(SpvParserTest, + FindIfSelectionInternalHeaders_FalseBranch_SwitchBreak_Ok) { + // crbug.com/tint/243 + auto assembly = CommonTypes() + R"( + %100 = OpFunction %void None %voidfn + + %10 = OpLabel + OpSelectionMerge %99 None + OpSwitch %uint_20 %99 20 %20 + + %20 = OpLabel + OpSelectionMerge %40 None + OpBranchConditional %cond %30 %99 ; false branch switch break is ok + + %30 = OpLabel + OpBranch %40 + + %40 = OpLabel ; if-selection merge + OpBranch %99 + + %99 = OpLabel ; switch merge + OpReturn +)"; + auto p = parser(test::Assemble(assembly)); + ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << p->error(); + FunctionEmitter fe(p.get(), *spirv_function(p.get(), 100)); + EXPECT_TRUE(FlowFindIfSelectionInternalHeaders(&fe)); + EXPECT_THAT(p->error(), Eq("")); +} + 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.