Require an OpSelectionMerge before an OpSwitch (#4154)
Fixes #4153
* OpSwitch is required to be preceeded by OpSelectionMerge
* Update newly invalid tests
diff --git a/source/val/validate_cfg.cpp b/source/val/validate_cfg.cpp
index a5f6e6a..36f632a 100644
--- a/source/val/validate_cfg.cpp
+++ b/source/val/validate_cfg.cpp
@@ -654,18 +654,15 @@
<< "Selection must be structured";
}
} else if (terminator->opcode() == SpvOpSwitch) {
- uint32_t count = 0;
- // Mark the targets as seen now, but only error out if this block was
- // missing a merge instruction and there were multiple unseen labels.
+ if (!merge) {
+ return _.diag(SPV_ERROR_INVALID_CFG, terminator)
+ << "OpSwitch must be preceeded by an OpSelectionMerge "
+ "instruction";
+ }
+ // Mark the targets as seen.
for (uint32_t i = 1; i < terminator->operands().size(); i += 2) {
const auto target = terminator->GetOperandAs<uint32_t>(i);
- if (seen.insert(target).second) {
- count++;
- }
- }
- if (!merge && count > 1) {
- return _.diag(SPV_ERROR_INVALID_CFG, terminator)
- << "Selection must be structured";
+ seen.insert(target);
}
}
}
diff --git a/test/opt/aggressive_dead_code_elim_test.cpp b/test/opt/aggressive_dead_code_elim_test.cpp
index 972e6e5..5b4291d 100644
--- a/test/opt/aggressive_dead_code_elim_test.cpp
+++ b/test/opt/aggressive_dead_code_elim_test.cpp
@@ -3948,12 +3948,15 @@
OpLoopMerge %14 %15 None
OpBranch %16
%16 = OpLabel
-OpSwitch %13 %14 0 %17 1 %15
+OpSelectionMerge %18 None
+OpSwitch %13 %18 0 %17 1 %15
%17 = OpLabel
OpStore %3 %uint_1
OpBranch %15
%15 = OpLabel
OpBranch %12
+%18 = OpLabel
+OpBranch %14
%14 = OpLabel
OpStore %3 %uint_0
OpReturn
diff --git a/test/opt/block_merge_test.cpp b/test/opt/block_merge_test.cpp
index 7381908..140a5c0 100644
--- a/test/opt/block_merge_test.cpp
+++ b/test/opt/block_merge_test.cpp
@@ -744,6 +744,7 @@
; CHECK: OpLoopMerge [[merge:%\w+]] [[cont:%\w+]] None
; CHECK-NEXT: OpBranch [[ret:%\w+]]
; CHECK: [[ret:%\w+]] = OpLabel
+; CHECK-NEXT: OpSelectionMerge
; CHECK-NEXT: OpSwitch
; CHECK-DAG: [[cont]] = OpLabel
; CHECK-DAG: [[merge]] = OpLabel
@@ -763,6 +764,7 @@
OpLoopMerge %3 %4 None
OpBranch %5
%5 = OpLabel
+OpSelectionMerge %6 None
OpSwitch %int_0 %6
%6 = OpLabel
OpReturn
diff --git a/test/opt/dead_branch_elim_test.cpp b/test/opt/dead_branch_elim_test.cpp
index 41ce31d..f89befb 100644
--- a/test/opt/dead_branch_elim_test.cpp
+++ b/test/opt/dead_branch_elim_test.cpp
@@ -2722,6 +2722,7 @@
; CHECK: [[bb1]] = OpLabel
; CHECK-NEXT: OpBranch [[bb2:%\w+]]
; CHECK: [[bb2]] = OpLabel
+; CHECK-NEXT: OpSelectionMerge
; CHECK-NEXT: OpSwitch {{%\w+}} [[bb3:%\w+]] 0 [[loop_merge]] 1 [[bb3:%\w+]]
; CHECK: [[bb3]] = OpLabel
; CHECK-NEXT: OpBranch [[sel_merge:%\w+]]
@@ -2739,6 +2740,7 @@
OpSelectionMerge %sel_merge None
OpBranchConditional %true %bb2 %bb4
%bb2 = OpLabel
+OpSelectionMerge %bb3 None
OpSwitch %undef_int %bb3 0 %loop_merge 1 %bb3
%bb3 = OpLabel
OpBranch %sel_merge
@@ -2782,6 +2784,7 @@
; CHECK: [[bb1]] = OpLabel
; CHECK-NEXT: OpBranch [[bb2:%\w+]]
; CHECK: [[bb2]] = OpLabel
+; CHECK-NEXT: OpSelectionMerge
; CHECK-NEXT: OpSwitch {{%\w+}} [[bb3:%\w+]] 0 [[loop_cont]] 1 [[bb3:%\w+]]
; CHECK: [[bb3]] = OpLabel
; CHECK-NEXT: OpBranch [[sel_merge:%\w+]]
@@ -2799,6 +2802,7 @@
OpSelectionMerge %sel_merge None
OpBranchConditional %true %bb2 %bb4
%bb2 = OpLabel
+OpSelectionMerge %bb3 None
OpSwitch %undef_int %bb3 0 %cont 1 %bb3
%bb3 = OpLabel
OpBranch %sel_merge
diff --git a/test/val/val_cfg_test.cpp b/test/val/val_cfg_test.cpp
index 9698fb1..9715a5a 100644
--- a/test/val/val_cfg_test.cpp
+++ b/test/val/val_cfg_test.cpp
@@ -3520,7 +3520,10 @@
CompileSuccessfully(text);
EXPECT_EQ(SPV_ERROR_INVALID_CFG, ValidateInstructions());
- EXPECT_THAT(getDiagnosticString(), HasSubstr("Selection must be structured"));
+ EXPECT_THAT(
+ getDiagnosticString(),
+ HasSubstr(
+ "OpSwitch must be preceeded by an OpSelectionMerge instruction"));
}
TEST_F(ValidateCFG, MissingMergeSwitchBad2) {
@@ -3544,7 +3547,10 @@
CompileSuccessfully(text);
EXPECT_EQ(SPV_ERROR_INVALID_CFG, ValidateInstructions());
- EXPECT_THAT(getDiagnosticString(), HasSubstr("Selection must be structured"));
+ EXPECT_THAT(
+ getDiagnosticString(),
+ HasSubstr(
+ "OpSwitch must be preceeded by an OpSelectionMerge instruction"));
}
TEST_F(ValidateCFG, MissingMergeOneBranchToMergeGood) {
@@ -3594,7 +3600,7 @@
EXPECT_EQ(SPV_SUCCESS, ValidateInstructions());
}
-TEST_F(ValidateCFG, MissingMergeOneTargetSwitchGood) {
+TEST_F(ValidateCFG, MissingMergeOneTargetSwitchBad) {
const std::string text = R"(
OpCapability Shader
OpCapability Linkage
@@ -3612,10 +3618,14 @@
)";
CompileSuccessfully(text);
- EXPECT_EQ(SPV_SUCCESS, ValidateInstructions());
+ EXPECT_EQ(SPV_ERROR_INVALID_CFG, ValidateInstructions());
+ EXPECT_THAT(
+ getDiagnosticString(),
+ HasSubstr(
+ "OpSwitch must be preceeded by an OpSelectionMerge instruction"));
}
-TEST_F(ValidateCFG, MissingMergeOneUnseenTargetSwitchGood) {
+TEST_F(ValidateCFG, MissingMergeOneUnseenTargetSwitchBad) {
const std::string text = R"(
OpCapability Shader
OpCapability Linkage
@@ -3640,7 +3650,11 @@
)";
CompileSuccessfully(text);
- EXPECT_EQ(SPV_SUCCESS, ValidateInstructions());
+ EXPECT_EQ(SPV_ERROR_INVALID_CFG, ValidateInstructions());
+ EXPECT_THAT(
+ getDiagnosticString(),
+ HasSubstr(
+ "OpSwitch must be preceeded by an OpSelectionMerge instruction"));
}
TEST_F(ValidateCFG, MissingMergeLoopBreakGood) {
diff --git a/test/val/val_limits_test.cpp b/test/val/val_limits_test.cpp
index 0ef61e2..8fb80a4 100644
--- a/test/val/val_limits_test.cpp
+++ b/test/val/val_limits_test.cpp
@@ -212,6 +212,7 @@
%5 = OpFunction %1 None %2
%7 = OpLabel
%8 = OpIAdd %3 %4 %4
+ OpSelectionMerge %10 None
OpSwitch %4 %10)";
// Now add the (literal, label) pairs
@@ -240,6 +241,7 @@
%5 = OpFunction %1 None %2
%7 = OpLabel
%8 = OpIAdd %3 %4 %4
+ OpSelectionMerge %10 None
OpSwitch %4 %10)";
// Now add the (literal, label) pairs
@@ -271,6 +273,7 @@
%5 = OpFunction %1 None %2
%7 = OpLabel
%8 = OpIAdd %3 %4 %4
+ OpSelectionMerge %10 None
OpSwitch %4 %10)";
// Now add the (literal, label) pairs
@@ -301,6 +304,7 @@
%5 = OpFunction %1 None %2
%7 = OpLabel
%8 = OpIAdd %3 %4 %4
+ OpSelectionMerge %10 None
OpSwitch %4 %10)";
// Now add the (literal, label) pairs
