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>
diff --git a/src/reader/spirv/function.cc b/src/reader/spirv/function.cc
index d576935..7ce03ed 100644
--- a/src/reader/spirv/function.cc
+++ b/src/reader/spirv/function.cc
@@ -1358,7 +1358,7 @@
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 @@
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 @@
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 @@
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 @@
<< 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 62ef821..05a1fff 100644
--- a/src/reader/spirv/function_cfg_test.cc
+++ b/src/reader/spirv/function_cfg_test.cc
@@ -7220,8 +7220,9 @@
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 @@
"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 @@
"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 @@
"(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.
