Validate nested constructs (#3068)
* Validate that if a construct contains a header and it's merge is
reachable, the construct also contains the merge
* updated block merging to not merge into the continue
* update inlining to mark the original block of a single block loop as
the continue
* updated some tests
* remove dead code
* rename kBlockTypeHeader to kBlockTypeSelection for clarity
diff --git a/source/opt/block_merge_util.cpp b/source/opt/block_merge_util.cpp
index 107723d..263a069 100644
--- a/source/opt/block_merge_util.cpp
+++ b/source/opt/block_merge_util.cpp
@@ -49,6 +49,18 @@
return IsMerge(context, block->id());
}
+// Returns true if |id| is the continue target of a merge instruction.
+bool IsContinue(IRContext* context, uint32_t id) {
+ return !context->get_def_use_mgr()->WhileEachUse(
+ id, [](Instruction* user, uint32_t index) {
+ SpvOp op = user->opcode();
+ if (op == SpvOpLoopMerge && index == 1u) {
+ return false;
+ }
+ return true;
+ });
+}
+
// Removes any OpPhi instructions in |block|, which should have exactly one
// predecessor, replacing uses of OpPhi ids with the ids associated with the
// predecessor.
@@ -86,6 +98,11 @@
return false;
}
+ if (pred_is_merge && IsContinue(context, lab_id)) {
+ // Cannot merge a continue target with a merge block.
+ return false;
+ }
+
// Don't bother trying to merge unreachable blocks.
if (auto dominators = context->GetDominatorAnalysis(block->GetParent())) {
if (!dominators->IsReachable(block)) return false;
diff --git a/source/opt/inline_pass.cpp b/source/opt/inline_pass.cpp
index 36991ec..3c874a7 100644
--- a/source/opt/inline_pass.cpp
+++ b/source/opt/inline_pass.cpp
@@ -27,7 +27,6 @@
static const int kSpvFunctionCallFunctionId = 2;
static const int kSpvFunctionCallArgumentId = 3;
static const int kSpvReturnValueId = 0;
-static const int kSpvLoopMergeContinueTargetIdInIdx = 1;
namespace spvtools {
namespace opt {
@@ -285,19 +284,14 @@
if (rid != 0) callee_result_ids.insert(rid);
});
- // If the caller is in a single-block loop, and the callee has multiple
- // blocks, then the normal inlining logic will place the OpLoopMerge in
- // the last of several blocks in the loop. Instead, it should be placed
- // at the end of the first block. First determine if the caller is in a
- // single block loop. We'll wait to move the OpLoopMerge until the end
- // of the regular inlining logic, and only if necessary.
- bool caller_is_single_block_loop = false;
+ // If the caller is a loop header and the callee has multiple blocks, then the
+ // normal inlining logic will place the OpLoopMerge in the last of several
+ // blocks in the loop. Instead, it should be placed at the end of the first
+ // block. We'll wait to move the OpLoopMerge until the end of the regular
+ // inlining logic, and only if necessary.
bool caller_is_loop_header = false;
- if (auto* loop_merge = call_block_itr->GetLoopMergeInst()) {
+ if (call_block_itr->GetLoopMergeInst()) {
caller_is_loop_header = true;
- caller_is_single_block_loop =
- call_block_itr->id() ==
- loop_merge->GetSingleWordInOperand(kSpvLoopMergeContinueTargetIdInIdx);
}
bool callee_begins_with_structured_header =
@@ -611,10 +605,6 @@
--loop_merge_itr;
assert(loop_merge_itr->opcode() == SpvOpLoopMerge);
std::unique_ptr<Instruction> cp_inst(loop_merge_itr->Clone(context()));
- if (caller_is_single_block_loop) {
- // Also, update its continue target to point to the last block.
- cp_inst->SetInOperand(kSpvLoopMergeContinueTargetIdInIdx, {last->id()});
- }
first->tail().InsertBefore(std::move(cp_inst));
// Remove the loop merge from the last block.
diff --git a/source/val/basic_block.h b/source/val/basic_block.h
index efbd243..876105c 100644
--- a/source/val/basic_block.h
+++ b/source/val/basic_block.h
@@ -15,8 +15,8 @@
#ifndef SOURCE_VAL_BASIC_BLOCK_H_
#define SOURCE_VAL_BASIC_BLOCK_H_
-#include <cstdint>
#include <bitset>
+#include <cstdint>
#include <functional>
#include <memory>
#include <vector>
@@ -28,7 +28,7 @@
enum BlockType : uint32_t {
kBlockTypeUndefined,
- kBlockTypeHeader,
+ kBlockTypeSelection,
kBlockTypeLoop,
kBlockTypeMerge,
kBlockTypeBreak,
diff --git a/source/val/function.cpp b/source/val/function.cpp
index 876a608..0281770 100644
--- a/source/val/function.cpp
+++ b/source/val/function.cpp
@@ -14,9 +14,8 @@
#include "source/val/function.h"
-#include <cassert>
-
#include <algorithm>
+#include <cassert>
#include <sstream>
#include <unordered_map>
#include <unordered_set>
@@ -99,7 +98,7 @@
spv_result_t Function::RegisterSelectionMerge(uint32_t merge_id) {
RegisterBlock(merge_id, false);
BasicBlock& merge_block = blocks_.at(merge_id);
- current_block_->set_type(kBlockTypeHeader);
+ current_block_->set_type(kBlockTypeSelection);
merge_block.set_type(kBlockTypeMerge);
merge_block_header_[&merge_block] = current_block_;
@@ -344,7 +343,7 @@
BasicBlock* header = merge_block_header_[bb];
assert(header);
block_depth_[bb] = GetBlockDepth(header);
- } else if (bb_dom->is_type(kBlockTypeHeader) ||
+ } else if (bb_dom->is_type(kBlockTypeSelection) ||
bb_dom->is_type(kBlockTypeLoop)) {
// The dominator of the given block is a header block. So, the nesting
// depth of this block is: 1 + nesting depth of the header.
diff --git a/source/val/validate_cfg.cpp b/source/val/validate_cfg.cpp
index 4801fc5..acce1fb 100644
--- a/source/val/validate_cfg.cpp
+++ b/source/val/validate_cfg.cpp
@@ -12,8 +12,6 @@
// See the License for the specific language governing permissions and
// limitations under the License.
-#include "source/val/validate.h"
-
#include <algorithm>
#include <cassert>
#include <functional>
@@ -34,6 +32,7 @@
#include "source/val/basic_block.h"
#include "source/val/construct.h"
#include "source/val/function.h"
+#include "source/val/validate.h"
#include "source/val/validation_state.h"
namespace spvtools {
@@ -755,6 +754,26 @@
<< header_name << " <ID> " << header->id();
}
}
+
+ if (block->is_type(BlockType::kBlockTypeSelection) ||
+ block->is_type(BlockType::kBlockTypeLoop)) {
+ size_t index = (block->terminator() - &_.ordered_instructions()[0]) - 1;
+ const auto& merge_inst = _.ordered_instructions()[index];
+ if (merge_inst.opcode() == SpvOpSelectionMerge ||
+ merge_inst.opcode() == SpvOpLoopMerge) {
+ uint32_t merge_id = merge_inst.GetOperandAs<uint32_t>(0);
+ auto merge_block = function->GetBlock(merge_id).first;
+ if (merge_block->reachable() &&
+ !construct_blocks.count(merge_block)) {
+ return _.diag(SPV_ERROR_INVALID_CFG, _.FindDef(block->id()))
+ << "Header block " << _.getIdName(block->id())
+ << " is contained in the " << construct_name
+ << " construct headed by " << _.getIdName(header->id())
+ << ", but it's merge block " << _.getIdName(merge_id)
+ << " is not";
+ }
+ }
+ }
}
// Checks rules for case constructs.
diff --git a/test/fuzz/transformation_add_dead_continue_test.cpp b/test/fuzz/transformation_add_dead_continue_test.cpp
index 8173e72..5892848 100644
--- a/test/fuzz/transformation_add_dead_continue_test.cpp
+++ b/test/fuzz/transformation_add_dead_continue_test.cpp
@@ -209,117 +209,6 @@
ASSERT_TRUE(IsEqual(env, after_transformation, context.get()));
}
-TEST(TransformationAddDeadContinueTest,
- DoNotAllowContinueToMergeBlockOfAnotherLoop) {
- // A loop header must dominate its merge block if that merge block is
- // reachable. We are thus not allowed to add a dead continue that would result
- // in violation of this property. This test checks for such a scenario.
-
- std::string shader = R"(
- OpCapability Shader
- %1 = OpExtInstImport "GLSL.std.450"
- OpMemoryModel Logical GLSL450
- OpEntryPoint Fragment %4 "main" %16 %139
- OpExecutionMode %4 OriginUpperLeft
- OpSource ESSL 310
- %2 = OpTypeVoid
- %3 = OpTypeFunction %2
- %6 = OpTypeFloat 32
- %7 = OpTypePointer Function %6
- %8 = OpTypeBool
- %14 = OpTypeVector %6 4
- %15 = OpTypePointer Input %14
- %16 = OpVariable %15 Input
- %138 = OpTypePointer Output %14
- %139 = OpVariable %138 Output
- %400 = OpConstantTrue %8
- %4 = OpFunction %2 None %3
- %5 = OpLabel
- OpBranch %500
- %500 = OpLabel
- OpLoopMerge %501 %502 None
- OpBranch %503 ; We are not allowed to change this to OpBranchConditional %400 %503 %502
- %503 = OpLabel
- OpLoopMerge %502 %504 None
- OpBranchConditional %400 %505 %504
- %505 = OpLabel
- OpBranch %502
- %504 = OpLabel
- OpBranch %503
- %502 = OpLabel
- OpBranchConditional %400 %501 %500
- %501 = OpLabel
- OpReturn
- OpFunctionEnd
- )";
-
- const auto env = SPV_ENV_UNIVERSAL_1_3;
- const auto consumer = nullptr;
- const auto context = BuildModule(env, consumer, shader, kFuzzAssembleOption);
- ASSERT_TRUE(IsValid(env, context.get()));
- FactManager fact_manager;
-
- ASSERT_FALSE(TransformationAddDeadContinue(500, true, {})
- .IsApplicable(context.get(), fact_manager));
- ASSERT_FALSE(TransformationAddDeadContinue(500, false, {})
- .IsApplicable(context.get(), fact_manager));
-}
-
-TEST(TransformationAddDeadContinueTest, DoNotAllowContinueToSelectionMerge) {
- // A selection header must dominate its merge block if that merge block is
- // reachable. We are thus not allowed to add a dead continue that would result
- // in violation of this property. This test checks for such a scenario.
-
- std::string shader = R"(
- OpCapability Shader
- %1 = OpExtInstImport "GLSL.std.450"
- OpMemoryModel Logical GLSL450
- OpEntryPoint Fragment %4 "main" %16 %139
- OpExecutionMode %4 OriginUpperLeft
- OpSource ESSL 310
- %2 = OpTypeVoid
- %3 = OpTypeFunction %2
- %6 = OpTypeFloat 32
- %7 = OpTypePointer Function %6
- %8 = OpTypeBool
- %14 = OpTypeVector %6 4
- %15 = OpTypePointer Input %14
- %16 = OpVariable %15 Input
- %138 = OpTypePointer Output %14
- %139 = OpVariable %138 Output
- %400 = OpConstantTrue %8
- %4 = OpFunction %2 None %3
- %5 = OpLabel
- OpBranch %500
- %500 = OpLabel
- OpLoopMerge %501 %502 None
- OpBranch %503 ; We are not allowed to change this to OpBranchConditional %400 %503 %502
- %503 = OpLabel
- OpSelectionMerge %502 None
- OpBranchConditional %400 %505 %504
- %505 = OpLabel
- OpBranch %502
- %504 = OpLabel
- OpBranch %502
- %502 = OpLabel
- OpBranchConditional %400 %501 %500
- %501 = OpLabel
- OpReturn
- OpFunctionEnd
- )";
-
- const auto env = SPV_ENV_UNIVERSAL_1_3;
- const auto consumer = nullptr;
- const auto context = BuildModule(env, consumer, shader, kFuzzAssembleOption);
- ASSERT_TRUE(IsValid(env, context.get()));
- FactManager fact_manager;
-
- ASSERT_FALSE(TransformationAddDeadContinue(500, true, {})
- .IsApplicable(context.get(), fact_manager));
- ASSERT_FALSE(TransformationAddDeadContinue(500, false, {})
- .IsApplicable(context.get(), fact_manager));
-}
-
TEST(TransformationAddDeadContinueTest, LoopNest) {
// Checks some allowed and disallowed scenarios for a nest of loops, including
// continuing a loop from an if or switch.
@@ -1420,7 +1309,6 @@
OpLoopMerge %1557 %1570 None
OpBranchConditional %395 %1562 %1557
%1562 = OpLabel
- OpSelectionMerge %1570 None
OpBranchConditional %395 %1571 %1570
%1571 = OpLabel
OpBranch %1557
diff --git a/test/fuzz/transformation_copy_object_test.cpp b/test/fuzz/transformation_copy_object_test.cpp
index 9d3fde0..b489f71 100644
--- a/test/fuzz/transformation_copy_object_test.cpp
+++ b/test/fuzz/transformation_copy_object_test.cpp
@@ -291,7 +291,7 @@
%31 = OpLabel
%42 = OpAccessChain %36 %18 %41
%43 = OpLoad %11 %42
- OpSelectionMerge %47 None
+ OpSelectionMerge %45 None
OpSwitch %43 %46 0 %44 1 %45
%46 = OpLabel
%69 = OpIAdd %11 %96 %27
diff --git a/test/fuzz/transformation_replace_id_with_synonym_test.cpp b/test/fuzz/transformation_replace_id_with_synonym_test.cpp
index 75e117a..41b6116 100644
--- a/test/fuzz/transformation_replace_id_with_synonym_test.cpp
+++ b/test/fuzz/transformation_replace_id_with_synonym_test.cpp
@@ -159,18 +159,20 @@
%65 = OpAccessChain %13 %11 %64
%66 = OpLoad %6 %65
%67 = OpSGreaterThan %29 %84 %66
- OpSelectionMerge %69 None
+ OpSelectionMerge %1000 None
OpBranchConditional %67 %68 %72
%68 = OpLabel
%71 = OpIAdd %6 %84 %26
- OpBranch %69
+ OpBranch %1000
%72 = OpLabel
%74 = OpIAdd %6 %84 %64
%205 = OpCopyObject %6 %74
- OpBranch %69
- %69 = OpLabel
+ OpBranch %1000
+ %1000 = OpLabel
%86 = OpPhi %6 %71 %68 %74 %72
%301 = OpPhi %6 %71 %68 %15 %72
+ OpBranch %69
+ %69 = OpLabel
OpBranch %20
%22 = OpLabel
%75 = OpAccessChain %46 %42 %50
@@ -421,18 +423,20 @@
%65 = OpAccessChain %13 %11 %64
%66 = OpLoad %6 %65
%67 = OpSGreaterThan %29 %84 %66
- OpSelectionMerge %69 None
+ OpSelectionMerge %1000 None
OpBranchConditional %67 %68 %72
%68 = OpLabel
%71 = OpIAdd %6 %84 %26
- OpBranch %69
+ OpBranch %1000
%72 = OpLabel
%74 = OpIAdd %6 %84 %64
%205 = OpCopyObject %6 %74
- OpBranch %69
- %69 = OpLabel
+ OpBranch %1000
+ %1000 = OpLabel
%86 = OpPhi %6 %71 %68 %205 %72
%301 = OpPhi %6 %71 %68 %15 %72
+ OpBranch %69
+ %69 = OpLabel
OpBranch %20
%22 = OpLabel
%75 = OpAccessChain %46 %42 %50
diff --git a/test/opt/aggressive_dead_code_elim_test.cpp b/test/opt/aggressive_dead_code_elim_test.cpp
index 9e5197d..e7303ba 100644
--- a/test/opt/aggressive_dead_code_elim_test.cpp
+++ b/test/opt/aggressive_dead_code_elim_test.cpp
@@ -5932,7 +5932,6 @@
%42 = OpLabel
%43 = OpLoad %int %i
%44 = OpSLessThan %bool %43 %int_1
-OpSelectionMerge %45 None
OpBranchConditional %44 %46 %40
%46 = OpLabel
%47 = OpLoad %int %i
diff --git a/test/opt/block_merge_test.cpp b/test/opt/block_merge_test.cpp
index f87fd80..11fba73 100644
--- a/test/opt/block_merge_test.cpp
+++ b/test/opt/block_merge_test.cpp
@@ -447,10 +447,53 @@
OpLoopMerge %merge %continue None
OpBranch %inner_header
%inner_header = OpLabel
-OpSelectionMerge %continue None
-OpBranchConditional %true %then %continue
+OpSelectionMerge %if_merge None
+OpBranchConditional %true %then %if_merge
%then = OpLabel
OpBranch %continue
+%if_merge = OpLabel
+OpBranch %continue
+%continue = OpLabel
+OpBranchConditional %false %merge %header
+%merge = OpLabel
+OpReturn
+OpFunctionEnd
+)";
+
+ SinglePassRunAndMatch<BlockMergePass>(text, true);
+}
+
+TEST_F(BlockMergeTest, CannotMergeContinue) {
+ const std::string text = R"(
+; CHECK: OpBranch [[loop_header:%\w+]]
+; CHECK: [[loop_header]] = OpLabel
+; CHECK-NEXT: OpLoopMerge {{%\w+}} [[continue:%\w+]]
+; CHECK-NEXT: OpBranchConditional {{%\w+}} [[if_header:%\w+]]
+; CHECK: [[if_header]] = OpLabel
+; CHECK-NEXT: OpSelectionMerge
+; CHECK: [[continue]] = OpLabel
+OpCapability Shader
+OpMemoryModel Logical GLSL450
+OpEntryPoint Fragment %func "func"
+OpExecutionMode %func OriginUpperLeft
+%void = OpTypeVoid
+%bool = OpTypeBool
+%true = OpConstantTrue %bool
+%false = OpConstantFalse %bool
+%functy = OpTypeFunction %void
+%func = OpFunction %void None %functy
+%entry = OpLabel
+OpBranch %header
+%header = OpLabel
+OpLoopMerge %merge %continue None
+OpBranchConditional %true %inner_header %merge
+%inner_header = OpLabel
+OpSelectionMerge %if_merge None
+OpBranchConditional %true %then %if_merge
+%then = OpLabel
+OpBranch %continue
+%if_merge = OpLabel
+OpBranch %continue
%continue = OpLabel
OpBranchConditional %false %merge %header
%merge = OpLabel
diff --git a/test/opt/dead_branch_elim_test.cpp b/test/opt/dead_branch_elim_test.cpp
index 5d41bdc..e612867 100644
--- a/test/opt/dead_branch_elim_test.cpp
+++ b/test/opt/dead_branch_elim_test.cpp
@@ -2798,7 +2798,9 @@
OpFunctionEnd
)";
- SinglePassRunAndMatch<DeadBranchElimPass>(predefs + body, true);
+ // The selection merge in the loop naming the continue target as merge is
+ // invalid, but handled by this pass so validation is disabled.
+ SinglePassRunAndMatch<DeadBranchElimPass>(predefs + body, false);
}
TEST_F(DeadBranchElimTest, SelectionMergeWithNestedLoop) {
@@ -2942,7 +2944,9 @@
OpFunctionEnd
)";
- SinglePassRunAndMatch<DeadBranchElimPass>(body, true);
+ // The selection merge in the loop naming the continue target as merge is
+ // invalid, but handled by this pass so validation is disabled.
+ SinglePassRunAndMatch<DeadBranchElimPass>(body, false);
}
TEST_F(DeadBranchElimTest, UnreachableMergeAndContinueSameBlock) {
diff --git a/test/opt/inline_test.cpp b/test/opt/inline_test.cpp
index fac49ca..f44c04a 100644
--- a/test/opt/inline_test.cpp
+++ b/test/opt/inline_test.cpp
@@ -1819,7 +1819,7 @@
%9 = OpLabel
OpBranch %10
%10 = OpLabel
-OpLoopMerge %12 %13 None
+OpLoopMerge %12 %10 None
OpBranch %13
%13 = OpLabel
OpBranchConditional %true %10 %12
@@ -1980,7 +1980,7 @@
OpBranch %13
%13 = OpLabel
%14 = OpCopyObject %bool %false
-OpLoopMerge %16 %19 None
+OpLoopMerge %16 %13 None
OpBranch %17
%17 = OpLabel
%18 = OpCopyObject %bool %true
@@ -2145,7 +2145,7 @@
%19 = OpLabel
%20 = OpCopyObject %int %int_2
%25 = OpCopyObject %int %int_0
-OpLoopMerge %23 %26 None
+OpLoopMerge %23 %19 None
OpBranch %26
%27 = OpLabel
%28 = OpCopyObject %int %int_1
diff --git a/test/opt/local_ssa_elim_test.cpp b/test/opt/local_ssa_elim_test.cpp
index f10d118..7afbb4c 100644
--- a/test/opt/local_ssa_elim_test.cpp
+++ b/test/opt/local_ssa_elim_test.cpp
@@ -193,7 +193,24 @@
)";
const std::string before =
- R"(%main = OpFunction %void None %9
+ R"(
+; CHECK: = OpFunction
+; CHECK-NEXT: [[entry:%\w+]] = OpLabel
+; CHECK: [[outer_header:%\w+]] = OpLabel
+; CHECK-NEXT: [[outer_f:%\w+]] = OpPhi %float %float_0 [[entry]] [[inner_f:%\w+]] [[outer_be:%\w+]]
+; CHECK-NEXT: [[i:%\w+]] = OpPhi %int %int_0 [[entry]] [[i_next:%\w+]] [[outer_be]]
+; CHECK-NEXT: OpSLessThan {{%\w+}} [[i]]
+; CHECK: [[inner_pre_header:%\w+]] = OpLabel
+; CHECK: [[inner_header:%\w+]] = OpLabel
+; CHECK-NEXT: [[inner_f]] = OpPhi %float [[outer_f]] [[inner_pre_header]] [[f_next:%\w+]] [[inner_be:%\w+]]
+; CHECK-NEXT: [[j:%\w+]] = OpPhi %int %int_0 [[inner_pre_header]] [[j_next:%\w+]] [[inner_be]]
+; CHECK: [[inner_be]] = OpLabel
+; CHECK: [[f_next]] = OpFAdd %float [[inner_f]]
+; CHECK: [[j_next]] = OpIAdd %int [[j]] %int_1
+; CHECK: [[outer_be]] = OpLabel
+; CHECK: [[i_next]] = OpIAdd
+; CHECK: OpStore %fo [[outer_f]]
+%main = OpFunction %void None %9
%24 = OpLabel
%f = OpVariable %_ptr_Function_float Function
%i = OpVariable %_ptr_Function_int Function
@@ -212,8 +229,8 @@
%31 = OpLabel
%32 = OpLoad %int %j
%33 = OpSLessThan %bool %32 %int_4
-OpLoopMerge %29 %34 None
-OpBranchConditional %33 %34 %29
+OpLoopMerge %50 %34 None
+OpBranchConditional %33 %34 %50
%34 = OpLabel
%35 = OpLoad %float %f
%36 = OpLoad %int %i
@@ -226,6 +243,8 @@
%42 = OpIAdd %int %41 %int_1
OpStore %j %42
OpBranch %31
+%50 = OpLabel
+OpBranch %29
%29 = OpLabel
%43 = OpLoad %int %i
%44 = OpIAdd %int %43 %int_1
@@ -238,50 +257,7 @@
OpFunctionEnd
)";
- const std::string after =
- R"(%main = OpFunction %void None %9
-%24 = OpLabel
-%f = OpVariable %_ptr_Function_float Function
-%i = OpVariable %_ptr_Function_int Function
-%j = OpVariable %_ptr_Function_int Function
-OpStore %f %float_0
-OpStore %i %int_0
-OpBranch %25
-%25 = OpLabel
-%47 = OpPhi %float %float_0 %24 %50 %29
-%46 = OpPhi %int %int_0 %24 %44 %29
-%27 = OpSLessThan %bool %46 %int_4
-OpLoopMerge %28 %29 None
-OpBranchConditional %27 %30 %28
-%30 = OpLabel
-OpStore %j %int_0
-OpBranch %31
-%31 = OpLabel
-%50 = OpPhi %float %47 %30 %40 %34
-%48 = OpPhi %int %int_0 %30 %42 %34
-%33 = OpSLessThan %bool %48 %int_4
-OpLoopMerge %29 %34 None
-OpBranchConditional %33 %34 %29
-%34 = OpLabel
-%38 = OpAccessChain %_ptr_Input_float %BC %46 %48
-%39 = OpLoad %float %38
-%40 = OpFAdd %float %50 %39
-OpStore %f %40
-%42 = OpIAdd %int %48 %int_1
-OpStore %j %42
-OpBranch %31
-%29 = OpLabel
-%44 = OpIAdd %int %46 %int_1
-OpStore %i %44
-OpBranch %25
-%28 = OpLabel
-OpStore %fo %47
-OpReturn
-OpFunctionEnd
-)";
-
- SinglePassRunAndCheck<SSARewritePass>(predefs + before, predefs + after, true,
- true);
+ SinglePassRunAndMatch<SSARewritePass>(predefs + before, true);
}
TEST_F(LocalSSAElimTest, ForLoopWithContinue) {
diff --git a/test/opt/loop_optimizations/fusion_legal.cpp b/test/opt/loop_optimizations/fusion_legal.cpp
index 41d796f..56b0b76 100644
--- a/test/opt/loop_optimizations/fusion_legal.cpp
+++ b/test/opt/loop_optimizations/fusion_legal.cpp
@@ -3177,7 +3177,7 @@
%21 = OpLabel
%29 = OpSMod %6 %96 %28
%30 = OpIEqual %17 %29 %9
- OpSelectionMerge %23 None
+ OpSelectionMerge %sel_merge None
OpBranchConditional %30 %31 %48
%31 = OpLabel
%44 = OpAccessChain %7 %41 %91 %96
@@ -3187,8 +3187,10 @@
OpStore %47 %46
OpBranch %32
%48 = OpLabel
- OpBranch %23
+ OpBranch %sel_merge
%32 = OpLabel
+ OpBranch %sel_merge
+ %sel_merge = OpLabel
OpBranch %23
%23 = OpLabel
%52 = OpIAdd %6 %96 %51
diff --git a/test/opt/pass_merge_return_test.cpp b/test/opt/pass_merge_return_test.cpp
index a305680..4118169 100644
--- a/test/opt/pass_merge_return_test.cpp
+++ b/test/opt/pass_merge_return_test.cpp
@@ -1390,7 +1390,6 @@
OpLoopMerge %36 %40 None
OpBranch %35
%35 = OpLabel
- OpSelectionMerge %40 None
OpBranchConditional %77 %39 %40
%39 = OpLabel
OpReturnValue %16
@@ -1402,7 +1401,6 @@
OpLoopMerge %45 %49 None
OpBranch %44
%44 = OpLabel
- OpSelectionMerge %49 None
OpBranchConditional %77 %48 %49
%48 = OpLabel
OpReturnValue %16
@@ -1415,7 +1413,6 @@
OpLoopMerge %64 %68 None
OpBranch %63
%63 = OpLabel
- OpSelectionMerge %68 None
OpBranchConditional %77 %67 %68
%67 = OpLabel
OpReturnValue %16
@@ -1813,12 +1810,14 @@
OpLoopMerge %11 %12 None
OpBranch %13
%13 = OpLabel
- OpLoopMerge %12 %14 None
- OpBranchConditional %8 %15 %12
+ OpLoopMerge %18 %14 None
+ OpBranchConditional %8 %15 %18
%15 = OpLabel
OpReturn
%14 = OpLabel
OpBranch %13
+ %18 = OpLabel
+ OpBranch %12
%12 = OpLabel
%16 = OpUndef %float
OpBranchConditional %8 %10 %11
diff --git a/test/reduce/reducer_test.cpp b/test/reduce/reducer_test.cpp
index a650d3b..59f2803 100644
--- a/test/reduce/reducer_test.cpp
+++ b/test/reduce/reducer_test.cpp
@@ -125,19 +125,21 @@
%29 = OpAccessChain %28 %27 %9
%30 = OpLoad %24 %29
%32 = OpFOrdGreaterThan %22 %30 %31
- OpSelectionMerge %34 None
+ OpSelectionMerge %90 None
OpBranchConditional %32 %33 %46
%33 = OpLabel
%40 = OpFAdd %24 %71 %30
%45 = OpISub %6 %73 %21
- OpBranch %34
+ OpBranch %90
%46 = OpLabel
%50 = OpFMul %24 %71 %30
%54 = OpSDiv %6 %73 %21
- OpBranch %34
- %34 = OpLabel
+ OpBranch %90
+ %90 = OpLabel
%77 = OpPhi %6 %45 %33 %54 %46
%76 = OpPhi %24 %40 %33 %50 %46
+ OpBranch %34
+ %34 = OpLabel
%57 = OpIAdd %6 %70 %56
OpBranch %10
%12 = OpLabel
@@ -193,11 +195,13 @@
OpLoopMerge %12 %34 None
OpBranchConditional %100 %11 %12
%11 = OpLabel
- OpSelectionMerge %34 None
+ OpSelectionMerge %90 None
OpBranchConditional %100 %33 %46
%33 = OpLabel
- OpBranch %34
+ OpBranch %90
%46 = OpLabel
+ OpBranch %90
+ %90 = OpLabel
OpBranch %34
%34 = OpLabel
OpBranch %10
@@ -345,7 +349,6 @@
OpLoopMerge %33 %38 None
OpBranch %32
%32 = OpLabel
- OpSelectionMerge %38 None
OpBranchConditional %30 %37 %38
%37 = OpLabel
OpSelectionMerge %42 None
diff --git a/test/reduce/validation_during_reduction_test.cpp b/test/reduce/validation_during_reduction_test.cpp
index 4051bac..2981c2e 100644
--- a/test/reduce/validation_during_reduction_test.cpp
+++ b/test/reduce/validation_during_reduction_test.cpp
@@ -13,7 +13,6 @@
// limitations under the License.
#include "source/reduce/reducer.h"
-
#include "source/reduce/reduction_opportunity.h"
#include "source/reduce/remove_instruction_reduction_opportunity.h"
#include "test/reduce/reduce_test_util.h"
@@ -23,8 +22,8 @@
namespace {
using opt::Function;
-using opt::IRContext;
using opt::Instruction;
+using opt::IRContext;
// A dumb reduction opportunity finder that finds opportunities to remove global
// values regardless of whether they are referenced. This is very likely to make
@@ -181,19 +180,21 @@
%29 = OpAccessChain %28 %27 %9
%30 = OpLoad %24 %29
%32 = OpFOrdGreaterThan %22 %30 %31
- OpSelectionMerge %34 None
+ OpSelectionMerge %90 None
OpBranchConditional %32 %33 %46
%33 = OpLabel
%40 = OpFAdd %24 %71 %30
%45 = OpISub %6 %73 %21
- OpBranch %34
+ OpBranch %90
%46 = OpLabel
%50 = OpFMul %24 %71 %30
%54 = OpSDiv %6 %73 %21
- OpBranch %34
- %34 = OpLabel
+ OpBranch %90
+ %90 = OpLabel
%77 = OpPhi %6 %45 %33 %54 %46
%76 = OpPhi %24 %40 %33 %50 %46
+ OpBranch %34
+ %34 = OpLabel
%57 = OpIAdd %6 %70 %56
OpBranch %10
%12 = OpLabel
@@ -303,19 +304,21 @@
%29 = OpAccessChain %28 %27 %9
%30 = OpLoad %24 %29
%32 = OpFOrdGreaterThan %22 %30 %31
- OpSelectionMerge %34 None
+ OpSelectionMerge %90 None
OpBranchConditional %32 %33 %46
%33 = OpLabel
%40 = OpFAdd %24 %71 %30
%45 = OpISub %6 %73 %21
- OpBranch %34
+ OpBranch %90
%46 = OpLabel
%50 = OpFMul %24 %71 %30
%54 = OpSDiv %6 %73 %21
- OpBranch %34
- %34 = OpLabel
+ OpBranch %90
+ %90 = OpLabel
%77 = OpPhi %6 %45 %33 %54 %46
%76 = OpPhi %24 %40 %33 %50 %46
+ OpBranch %34
+ %34 = OpLabel
%57 = OpIAdd %6 %70 %56
OpBranch %10
%12 = OpLabel
@@ -392,19 +395,21 @@
%29 = OpAccessChain %28 %27 %9
%30 = OpLoad %24 %29
%32 = OpFOrdGreaterThan %22 %30 %31
- OpSelectionMerge %34 None
+ OpSelectionMerge %90 None
OpBranchConditional %32 %33 %46
%33 = OpLabel
%40 = OpFAdd %24 %71 %30
%45 = OpISub %6 %73 %21
- OpBranch %34
+ OpBranch %90
%46 = OpLabel
%50 = OpFMul %24 %71 %30
%54 = OpSDiv %6 %73 %21
- OpBranch %34
- %34 = OpLabel
+ OpBranch %90
+ %90 = OpLabel
%77 = OpPhi %6 %45 %33 %54 %46
%76 = OpPhi %24 %40 %33 %50 %46
+ OpBranch %34
+ %34 = OpLabel
%57 = OpIAdd %6 %70 %56
OpBranch %10
%12 = OpLabel
diff --git a/test/val/val_cfg_test.cpp b/test/val/val_cfg_test.cpp
index f06f36c..22dd117 100644
--- a/test/val/val_cfg_test.cpp
+++ b/test/val/val_cfg_test.cpp
@@ -23,7 +23,6 @@
#include <vector>
#include "gmock/gmock.h"
-
#include "source/diagnostic.h"
#include "source/spirv_target_env.h"
#include "source/val/validate.h"
@@ -1355,7 +1354,7 @@
}
if (cap == SpvCapabilityShader) {
branch.AppendBody("OpLoopMerge %merge %target None\n");
- body.AppendBody("OpSelectionMerge %target None\n");
+ body.AppendBody("OpSelectionMerge %f None\n");
}
if (!spvIsWebGPUEnv(env))
@@ -1650,25 +1649,27 @@
Block entry("entry");
Block loop1("loop1", SpvOpBranchConditional);
Block loop2("loop2", SpvOpBranchConditional);
- Block loop2_merge("loop2_merge", SpvOpBranchConditional);
+ Block loop2_merge("loop2_merge");
+ Block loop1_cont("loop1_cont", SpvOpBranchConditional);
Block be_block("be_block");
Block exit("exit", SpvOpReturn);
entry.SetBody("%cond = OpSLessThan %boolt %one %two\n");
if (is_shader) {
- loop1.SetBody("OpLoopMerge %exit %loop2_merge None\n");
+ loop1.SetBody("OpLoopMerge %exit %loop1_cont None\n");
loop2.SetBody("OpLoopMerge %loop2_merge %loop2 None\n");
}
- std::string str = GetDefaultHeader(GetParam()) +
- nameOps("loop1", "loop2", "be_block", "loop2_merge") +
- types_consts() +
- "%func = OpFunction %voidt None %funct\n";
+ std::string str =
+ GetDefaultHeader(GetParam()) +
+ nameOps("loop1", "loop2", "be_block", "loop1_cont", "loop2_merge") +
+ types_consts() + "%func = OpFunction %voidt None %funct\n";
str += entry >> loop1;
str += loop1 >> std::vector<Block>({loop2, exit});
str += loop2 >> std::vector<Block>({loop2, loop2_merge});
- str += loop2_merge >> std::vector<Block>({be_block, exit});
+ str += loop2_merge >> loop1_cont;
+ str += loop1_cont >> std::vector<Block>({be_block, exit});
str += be_block >> loop1;
str += exit;
str += "OpFunctionEnd";
@@ -1678,7 +1679,7 @@
ASSERT_EQ(SPV_ERROR_INVALID_CFG, ValidateInstructions());
EXPECT_THAT(getDiagnosticString(),
MatchesRegex("The continue construct with the continue target "
- ".\\[%loop2_merge\\] is not post dominated by the "
+ ".\\[%loop1_cont\\] is not post dominated by the "
"back-edge block .\\[%be_block\\]\n"
" %be_block = OpLabel\n"));
} else {
@@ -2037,13 +2038,10 @@
EXPECT_EQ(SPV_SUCCESS, ValidateInstructions()) << getDiagnosticString();
}
-TEST_P(ValidateCFG, ContinueTargetCanBeMergeBlockForNestedStructureGood) {
- // This example is valid. It shows that the validator can't just add
- // an edge from the loop head to the continue target. If that edge
- // is added, then the "if_merge" block is both the continue target
- // for the loop and also the merge block for the nested selection, but
- // then it wouldn't be dominated by "if_head", the header block for the
- // nested selection.
+TEST_P(ValidateCFG, ContinueTargetCanBeMergeBlockForNestedStructure) {
+ // The continue construct cannot be the merge target of a nested selection
+ // because the loop construct must contain "if_merge" because it contains
+ // "if_head".
bool is_shader = GetParam() == SpvCapabilityShader;
Block entry("entry");
Block loop("loop");
@@ -2072,7 +2070,16 @@
str += "OpFunctionEnd";
CompileSuccessfully(str);
- EXPECT_EQ(SPV_SUCCESS, ValidateInstructions()) << getDiagnosticString();
+ if (is_shader) {
+ EXPECT_EQ(SPV_ERROR_INVALID_CFG, ValidateInstructions());
+ EXPECT_THAT(
+ getDiagnosticString(),
+ HasSubstr("Header block 3[%if_head] is contained in the loop construct "
+ "headed "
+ "by 2[%loop], but it's merge block 5[%if_merge] is not"));
+ } else {
+ EXPECT_THAT(SPV_SUCCESS, ValidateInstructions());
+ }
}
TEST_P(ValidateCFG, SingleLatchBlockMultipleBranchesToLoopHeader) {
@@ -3681,17 +3688,21 @@
OpLoopMerge %8 %9 None
OpBranch %10
%10 = OpLabel
-OpLoopMerge %9 %11 None
+OpLoopMerge %11 %12 None
+OpBranch %13
+%13 = OpLabel
+OpSelectionMerge %14 None
+OpBranchConditional %3 %14 %15
+%15 = OpLabel
+OpBranch %8
+%14 = OpLabel
OpBranch %12
%12 = OpLabel
-OpSelectionMerge %11 None
-OpBranchConditional %3 %11 %13
-%13 = OpLabel
-OpBranch %8
+OpBranchConditional %3 %10 %11
%11 = OpLabel
-OpBranchConditional %3 %9 %10
+OpBranch %9
%9 = OpLabel
-OpBranchConditional %3 %8 %7
+OpBranchConditional %3 %7 %8
%8 = OpLabel
OpReturn
OpFunctionEnd
@@ -3700,7 +3711,7 @@
CompileSuccessfully(text);
EXPECT_EQ(SPV_ERROR_INVALID_CFG, ValidateInstructions());
EXPECT_THAT(getDiagnosticString(),
- HasSubstr("block <ID> 13[%13] exits the loop headed by <ID> "
+ HasSubstr("block <ID> 15[%15] exits the loop headed by <ID> "
"10[%10], but not via a structured exit"));
}
@@ -3726,9 +3737,11 @@
OpSelectionMerge %31 None
OpBranchConditional %undef %30 %31
%30 = OpLabel
-OpSelectionMerge %37 None
-OpBranchConditional %undef %36 %37
+OpSelectionMerge %38 None
+OpBranchConditional %undef %36 %38
%36 = OpLabel
+OpBranch %38
+%38 = OpLabel
OpBranch %37
%37 = OpLabel
OpBranch %10
@@ -4100,6 +4113,80 @@
EXPECT_THAT(getDiagnosticString(), HasSubstr("Selection must be structured"));
}
+TEST_F(ValidateCFG, ContinueCannotBeSelectionMergeTarget) {
+ const std::string text = R"(
+OpCapability Shader
+OpCapability Linkage
+OpMemoryModel Logical GLSL450
+OpName %loop "loop"
+OpName %continue "continue"
+OpName %body "body"
+%void = OpTypeVoid
+%void_fn = OpTypeFunction %void
+%bool = OpTypeBool
+%undef = OpUndef %bool
+%func = OpFunction %void None %void_fn
+%entry = OpLabel
+OpBranch %loop
+%loop = OpLabel
+OpLoopMerge %exit %continue None
+OpBranch %body
+%body = OpLabel
+OpSelectionMerge %continue None
+OpBranchConditional %undef %exit %continue
+%continue = OpLabel
+OpBranch %loop
+%exit = OpLabel
+OpReturn
+OpFunctionEnd
+)";
+
+ CompileSuccessfully(text);
+ EXPECT_EQ(SPV_ERROR_INVALID_CFG, ValidateInstructions());
+ EXPECT_THAT(
+ getDiagnosticString(),
+ HasSubstr(
+ "Header block 3[%body] is contained in the loop construct headed by "
+ "1[%loop], but it's merge block 2[%continue] is not"));
+}
+
+TEST_F(ValidateCFG, ContinueCannotBeLoopMergeTarget) {
+ const std::string text = R"(
+OpCapability Shader
+OpCapability Linkage
+OpMemoryModel Logical GLSL450
+OpName %loop "loop"
+OpName %continue "continue"
+OpName %inner "inner"
+%void = OpTypeVoid
+%void_fn = OpTypeFunction %void
+%bool = OpTypeBool
+%undef = OpUndef %bool
+%func = OpFunction %void None %void_fn
+%entry = OpLabel
+OpBranch %loop
+%loop = OpLabel
+OpLoopMerge %exit %continue None
+OpBranchConditional %undef %exit %inner
+%inner = OpLabel
+OpLoopMerge %continue %inner None
+OpBranchConditional %undef %inner %continue
+%continue = OpLabel
+OpBranch %loop
+%exit = OpLabel
+OpReturn
+OpFunctionEnd
+)";
+
+ CompileSuccessfully(text);
+ EXPECT_EQ(SPV_ERROR_INVALID_CFG, ValidateInstructions());
+ EXPECT_THAT(
+ getDiagnosticString(),
+ HasSubstr(
+ "Header block 3[%inner] is contained in the loop construct headed by "
+ "1[%loop], but it's merge block 2[%continue] is not"));
+}
+
} // namespace
} // namespace val
} // namespace spvtools
