From b275fd2f0e6bcbc7ef54bdb4d67cba2827b87e71 Mon Sep 17 00:00:00 2001 From: David Neto Date: Tue, 19 May 2020 14:43:19 +0000 Subject: [PATCH] [spirv-reader] Systematic bad construct exit tests Do so systmatically. Before we had tested some as a side effect of other objectives. Fix the error message for when we have a bad exit from a loop construct that bypasses not only the continue construct but the loop merge block itself. Bug: tint:3 Change-Id: Iaf8fc9bcd3162002aa906efa90a244ef5f439911 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/21580 Reviewed-by: dan sinclair --- src/reader/spirv/function.cc | 23 ++++- src/reader/spirv/function_cfg_test.cc | 141 ++++++++++++++++++++++++-- 2 files changed, 152 insertions(+), 12 deletions(-) diff --git a/src/reader/spirv/function.cc b/src/reader/spirv/function.cc index b3f0c81fa8..94aa686dc0 100644 --- a/src/reader/spirv/function.cc +++ b/src/reader/spirv/function.cc @@ -1139,11 +1139,30 @@ bool FunctionEmitter::ClassifyCFGEdges() { (edge_kind == EdgeKind::kCaseFallThrough)) { // Check for an invalid forward exit out of this construct. if (dest_info->pos >= src_construct.end_pos) { + // In most cases we're bypassing the merge block for the source + // construct. + auto end_block = src_construct.end_id; + const char* end_block_desc = "merge block"; + if (src_construct.kind == Construct::kLoop) { + // For a loop construct, we have two valid places to go: the + // continue target or the merge for the loop header, which is + // further down. + const auto loop_merge = + GetBlockInfo(src_construct.begin_id)->merge_for_header; + if (dest_info->pos >= GetBlockInfo(loop_merge)->pos) { + // We're bypassing the loop's merge block. + end_block = loop_merge; + } else { + // We're bypassing the loop's continue target, and going into + // the middle of the continue construct. + end_block_desc = "continue target"; + } + } return Fail() << "Branch from block " << src << " to block " << dest << " is an invalid exit from construct starting at block " - << src_construct.begin_id << "; branch bypasses block " - << src_construct.end_id; + << src_construct.begin_id << "; branch bypasses " + << end_block_desc << " " << end_block; } normal_forward_edges.push_back(dest); diff --git a/src/reader/spirv/function_cfg_test.cc b/src/reader/spirv/function_cfg_test.cc index 6026c7577e..cae468f93a 100644 --- a/src/reader/spirv/function_cfg_test.cc +++ b/src/reader/spirv/function_cfg_test.cc @@ -4066,7 +4066,7 @@ TEST_F(SpvParserTest, FindSwitchCaseHeaders_CaseIsDefault) { EXPECT_THAT(*(bi20->case_values.get()), UnorderedElementsAre(200)); } -TEST_F(SpvParserTest, FindSwitchCaseHeaders_ManyCasesWithSameValue_Error) { +TEST_F(SpvParserTest, FindSwitchCaseHeaders_ManyCasesWithSameValue_IsError) { auto assembly = CommonTypes() + R"( %100 = OpFunction %void None %voidfn @@ -4618,6 +4618,32 @@ TEST_F(SpvParserTest, ClassifyCFGEdges_IfBreak_FromIfThenElse) { EXPECT_EQ(bi50->succ_edge[99], EdgeKind::kIfBreak); } +TEST_F(SpvParserTest, ClassifyCFGEdge_IfBreak_BypassesMerge_IsError) { + auto assembly = CommonTypes() + R"( + %100 = OpFunction %void None %voidfn + + %10 = OpLabel + OpSelectionMerge %50 None + OpBranchConditional %cond %20 %50 + + %20 = OpLabel + OpBranch %99 + + %50 = OpLabel ; merge + OpBranch %99 + + %99 = OpLabel + OpReturn +)"; + auto* p = parser(test::Assemble(assembly)); + ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << p->error(); + FunctionEmitter fe(p, *spirv_function(100)); + EXPECT_FALSE(FlowClassifyCFGEdges(&fe)); + EXPECT_THAT(p->error(), + Eq("Branch from block 20 to block 99 is an invalid exit from " + "construct starting at block 10; branch bypasses merge block 50")); +} + TEST_F(SpvParserTest, ClassifyCFGEdges_SwitchBreak_FromSwitchCaseDirect) { auto assembly = CommonTypes() + R"( %100 = OpFunction %void None %voidfn @@ -4785,6 +4811,32 @@ TEST_F(SpvParserTest, ClassifyCFGEdges_SwitchBreak_FromNestedIf_Conditional) { EXPECT_EQ(bi->succ_edge[99], EdgeKind::kSwitchBreak); } +TEST_F(SpvParserTest, ClassifyCFGEdges_SwitchBreak_BypassesMerge_IsError) { + auto assembly = CommonTypes() + R"( + %100 = OpFunction %void None %voidfn + + %10 = OpLabel + OpSelectionMerge %50 None + OpSwitch %selector %50 20 %20 + + %20 = OpLabel + OpBranch %99 ; invalid exit + + %50 = OpLabel ; switch merge + OpBranch %99 + + %99 = OpLabel + OpReturn +)"; + auto* p = parser(test::Assemble(assembly)); + ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << p->error(); + FunctionEmitter fe(p, *spirv_function(100)); + EXPECT_FALSE(FlowClassifyCFGEdges(&fe)); + EXPECT_THAT(p->error(), + Eq("Branch from block 20 to block 99 is an invalid exit from " + "construct starting at block 10; branch bypasses merge block 50")); +} + TEST_F(SpvParserTest, ClassifyCFGEdges_SwitchBreak_FromNestedLoop_IsError) { // It's an error because the break can only go as far as the loop. auto assembly = CommonTypes() + R"( @@ -4816,7 +4868,7 @@ TEST_F(SpvParserTest, ClassifyCFGEdges_SwitchBreak_FromNestedLoop_IsError) { EXPECT_FALSE(FlowClassifyCFGEdges(&fe)); EXPECT_THAT(p->error(), Eq("Branch from block 30 to block 99 is an invalid exit from " - "construct starting at block 20; branch bypasses block 70")); + "construct starting at block 20; branch bypasses merge block 80")); } TEST_F(SpvParserTest, ClassifyCFGEdges_SwitchBreak_FromNestedSwitch_IsError) { @@ -4847,7 +4899,7 @@ TEST_F(SpvParserTest, ClassifyCFGEdges_SwitchBreak_FromNestedSwitch_IsError) { EXPECT_FALSE(FlowClassifyCFGEdges(&fe)); EXPECT_THAT(p->error(), Eq("Branch from block 30 to block 99 is an invalid exit from " - "construct starting at block 20; branch bypasses block 80")); + "construct starting at block 20; branch bypasses merge block 80")); } TEST_F(SpvParserTest, ClassifyCFGEdges_LoopBreak_FromLoopBody) { @@ -5065,6 +5117,75 @@ TEST_F(SpvParserTest, "(violates post-dominance rule)")); } +TEST_F(SpvParserTest, + ClassifyCFGEdges_LoopBreak_FromLoopBypassesMerge_IsError) { + auto assembly = CommonTypes() + R"( + %100 = OpFunction %void None %voidfn + + %10 = OpLabel + OpBranch %20 + + %20 = OpLabel + OpLoopMerge %50 %40 None + OpBranchConditional %cond %30 %50 + + %30 = OpLabel + OpBranch %99 ; bad exit + + %40 = OpLabel ; continue construct + OpBranch %20 + + %50 = OpLabel + OpBranch %99 + + %99 = OpLabel + OpReturn +)"; + auto* p = parser(test::Assemble(assembly)); + ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << p->error(); + FunctionEmitter fe(p, *spirv_function(100)); + EXPECT_FALSE(FlowClassifyCFGEdges(&fe)); + EXPECT_THAT(p->error(), + Eq("Branch from block 30 to block 99 is an invalid exit from " + "construct starting at block 20; branch bypasses merge block 50")); +} + +TEST_F(SpvParserTest, + ClassifyCFGEdges_LoopBreak_FromContinueBypassesMerge_IsError) { + auto assembly = CommonTypes() + R"( + %100 = OpFunction %void None %voidfn + + %10 = OpLabel + OpBranch %20 + + %20 = OpLabel + OpLoopMerge %50 %40 None + OpBranchConditional %cond %30 %50 + + %30 = OpLabel + OpBranch %40 + + %40 = OpLabel ; continue construct + OpBranch %45 + + %45 = OpLabel + OpBranchConditional %cond2 %20 %99 ; branch to %99 is bad exit + + %50 = OpLabel + OpBranch %99 + + %99 = OpLabel + OpReturn +)"; + auto* p = parser(test::Assemble(assembly)); + ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << p->error(); + FunctionEmitter fe(p, *spirv_function(100)); + EXPECT_FALSE(FlowClassifyCFGEdges(&fe)); + EXPECT_THAT(p->error(), + Eq("Branch from block 45 to block 99 is an invalid exit from " + "construct starting at block 40; branch bypasses merge block 50")); +} + TEST_F(SpvParserTest, ClassifyCFGEdges_LoopContinue_LoopBodyToContinue) { auto assembly = CommonTypes() + R"( %100 = OpFunction %void None %voidfn @@ -5212,7 +5333,7 @@ TEST_F(SpvParserTest, } TEST_F(SpvParserTest, - ClassifyCFGEdges_LoopContinue_FromNestedSwitchCaseDirect_Error) { + ClassifyCFGEdges_LoopContinue_FromNestedSwitchCaseDirect_IsError) { auto assembly = CommonTypes() + R"( %100 = OpFunction %void None %voidfn @@ -5250,7 +5371,7 @@ TEST_F(SpvParserTest, } TEST_F(SpvParserTest, - ClassifyCFGEdges_LoopContinue_FromNestedSwitchDefaultDirect_Error) { + ClassifyCFGEdges_LoopContinue_FromNestedSwitchDefaultDirect_IsError) { auto assembly = CommonTypes() + R"( %100 = OpFunction %void None %voidfn @@ -5798,7 +5919,7 @@ TEST_F(SpvParserTest, %30 = OpLabel OpBranch %60 - %50 = OpLabel + %50 = OpLabel ; continue target OpBranch %60 %60 = OpLabel @@ -5816,7 +5937,7 @@ TEST_F(SpvParserTest, EXPECT_FALSE(FlowClassifyCFGEdges(&fe)); EXPECT_THAT(p->error(), Eq("Branch from block 30 to block 60 is an invalid exit from " - "construct starting at block 20; branch bypasses block 50")); + "construct starting at block 20; branch bypasses continue target 50")); } TEST_F(SpvParserTest, @@ -5849,7 +5970,7 @@ TEST_F(SpvParserTest, EXPECT_FALSE(FlowClassifyCFGEdges(&fe)); EXPECT_THAT(p->error(), Eq("Branch from block 50 to block 60 is an invalid exit from " - "construct starting at block 50; branch bypasses block 80")); + "construct starting at block 50; branch bypasses merge block 80")); } TEST_F(SpvParserTest, ClassifyCFGEdges_TooManyBackedges) { @@ -6351,7 +6472,7 @@ TEST_F(SpvParserTest, } TEST_F(SpvParserTest, - FindIfSelectionInternalHeaders_Premerge_MultiCandidate_Error) { + FindIfSelectionInternalHeaders_Premerge_MultiCandidate_IsError) { auto assembly = CommonTypes() + R"( %100 = OpFunction %void None %voidfn @@ -6491,7 +6612,7 @@ TEST_F(SpvParserTest, } TEST_F(SpvParserTest, - FindIfSelectionInternalHeaders_IfBreak_WithForwardToPremerge_Error) { + FindIfSelectionInternalHeaders_IfBreak_WithForwardToPremerge_IsError) { auto assembly = CommonTypes() + R"( %100 = OpFunction %void None %voidfn