encounter
/
dawn-cmake
mirror of https://github.com/encounter/dawn-cmake.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

spirv-reader: fix overly aggressive dominance check 

When finding the true-head, false-head, and potentially the
premerge-head blocks of an if-selection, there was an overly
aggressive check for the true-branch or false-branch landing
on a merge block interior to the if-selection. The check was
determining if the merge block actually corresponded to the selection
header in question.  If not, then it was throwing an error.

The bug was that this check must be performed only if the
target in question is actually inside the selection body.
There are cases where the target could represent a structured
exit, e.g. to an enclosing loop's merge or continue, or
an enclosing switch construct's merge.

There is still a latent bug: if either the true branch
or false branch represent such a kLoopBreak, kLoopContinue, or
kSwitchBreak edge, then those are not properly generated.
That will be fixed in a followup CL.

Bug: tint:243, tint:494
Change-Id: I141cce07fa0a1dfe5fad20dd2989315e4cd7b688
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47482
Auto-Submit: David Neto <dneto@google.com>
Commit-Queue: Alan Baker <alanbaker@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Alan Baker <alanbaker@google.com>
This commit is contained in:
David Neto 2021-04-13 18:35:37 +00:00 committed by Commit Bot service account
parent 095cd1c255
commit 0386472762
2 changed files with 230 additions and 14 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

27
src/reader/spirv/function.cc
View File

@ -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

217
src/reader/spirv/function_cfg_test.cc
View File

@ -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.