spirv-reader: disallow OpSwitch without preceding OpSelectionMerge
This was updated/clarified by the SPIR WG.
Bug: tint:3
Change-Id: Ie4c503f0e5f80ffeabada9c526375588e81a5ceb
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/45740
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 0b03991..09eaa5b 100644
--- a/src/reader/spirv/function.cc
+++ b/src/reader/spirv/function.cc
@@ -2621,8 +2621,14 @@
return true;
}
case SpvOpSwitch:
- // TODO(dneto)
- break;
+ // An OpSelectionMerge must precede an OpSwitch. That is clarified
+ // in the resolution to Khronos-internal SPIR-V issue 115.
+ // A new enough version of the SPIR-V validator checks this case.
+ // But issue an error in this case, as a defensive measure.
+ return Fail() << "invalid structured control flow: found an OpSwitch "
+ "that is not preceded by an "
+ "OpSelectionMerge: "
+ << terminator.PrettyPrint();
default:
break;
}
diff --git a/src/reader/spirv/function_cfg_test.cc b/src/reader/spirv/function_cfg_test.cc
index d5a5756..62ef821 100644
--- a/src/reader/spirv/function_cfg_test.cc
+++ b/src/reader/spirv/function_cfg_test.cc
@@ -13542,9 +13542,31 @@
"a structured header (it has no merge instruction)"));
}
+TEST_F(SpvParserTest, Switch_NotAsSelectionHeader_Simple) {
+ auto assembly = CommonTypes() + R"(
+ %100 = OpFunction %void None %voidfn
+
+ %10 = OpLabel
+ OpSwitch %uint_0 %99
+
+ %99 = OpLabel
+ OpReturn
+
+ OpFunctionEnd
+)";
+ auto p = parser(test::Assemble(assembly));
+ ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << p->error();
+ FunctionEmitter fe(p.get(), *spirv_function(p.get(), 100));
+ EXPECT_FALSE(fe.EmitBody());
+ EXPECT_THAT(
+ p->error(),
+ HasSubstr("invalid structured control flow: found an OpSwitch that "
+ "is not preceded by an OpSelectionMerge:"));
+}
+
TEST_F(SpvParserTest,
- DISABLED_Switch_NotAsSelectionHeader_NonDefaultBranchesAreContinue) {
- // Adapted from SPIRV-Tools test MissingMergeOneUnseenTargetSwitchGood
+ Switch_NotAsSelectionHeader_NonDefaultBranchesAreContinue) {
+ // Adapted from SPIRV-Tools test MissingMergeOneUnseenTargetSwitchBad
auto p = parser(test::Assemble(CommonTypes() + R"(
%100 = OpFunction %void None %voidfn
%entry = OpLabel
@@ -13571,15 +13593,15 @@
)"));
ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << p->error();
FunctionEmitter fe(p.get(), *spirv_function(p.get(), 100));
- EXPECT_TRUE(fe.EmitBody()) << p->error();
- auto got = ToString(p->builder(), fe.ast_body());
- auto* expect = "unhandled case";
- ASSERT_EQ(expect, got);
+ EXPECT_FALSE(fe.EmitBody());
+ EXPECT_THAT(
+ p->error(),
+ HasSubstr("invalid structured control flow: found an OpSwitch that "
+ "is not preceded by an OpSelectionMerge:"));
}
-TEST_F(SpvParserTest,
- DISABLED_Switch_NotAsSelectionHeader_DefaultBranchIsContinue) {
- // Adapted from SPIRV-Tools test MissingMergeOneUnseenTargetSwitchGood
+TEST_F(SpvParserTest, Switch_NotAsSelectionHeader_DefaultBranchIsContinue) {
+ // Adapted from SPIRV-Tools test MissingMergeOneUnseenTargetSwitchBad
auto p = parser(test::Assemble(CommonTypes() + R"(
%100 = OpFunction %void None %voidfn
%entry = OpLabel
@@ -13606,10 +13628,11 @@
)"));
ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << p->error();
FunctionEmitter fe(p.get(), *spirv_function(p.get(), 100));
- EXPECT_TRUE(fe.EmitBody()) << p->error();
- auto got = ToString(p->builder(), fe.ast_body());
- auto* expect = "unhandled case";
- ASSERT_EQ(expect, got);
+ EXPECT_FALSE(fe.EmitBody());
+ EXPECT_THAT(
+ p->error(),
+ HasSubstr("invalid structured control flow: found an OpSwitch that "
+ "is not preceded by an OpSelectionMerge:"));
}
TEST_F(SpvParserTest, SiblingLoopConstruct_Null) {
