From b334829a91d6bcbbf6e101ca54adccb113a24fa9 Mon Sep 17 00:00:00 2001 From: alan-baker Date: Wed, 27 Nov 2019 16:45:57 -0500 Subject: [PATCH] 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 --- source/opt/block_merge_util.cpp | 17 +++ source/opt/inline_pass.cpp | 22 +-- source/val/basic_block.h | 4 +- source/val/function.cpp | 7 +- source/val/validate_cfg.cpp | 23 ++- .../transformation_add_dead_continue_test.cpp | 112 -------------- test/fuzz/transformation_copy_object_test.cpp | 2 +- ...formation_replace_id_with_synonym_test.cpp | 20 ++- test/opt/aggressive_dead_code_elim_test.cpp | 1 - test/opt/block_merge_test.cpp | 47 +++++- test/opt/dead_branch_elim_test.cpp | 8 +- test/opt/inline_test.cpp | 6 +- test/opt/local_ssa_elim_test.cpp | 70 +++------ test/opt/loop_optimizations/fusion_legal.cpp | 6 +- test/opt/pass_merge_return_test.cpp | 9 +- test/reduce/reducer_test.cpp | 17 ++- .../validation_during_reduction_test.cpp | 33 ++-- test/val/val_cfg_test.cpp | 143 ++++++++++++++---- 18 files changed, 291 insertions(+), 256 deletions(-) diff --git a/source/opt/block_merge_util.cpp b/source/opt/block_merge_util.cpp index 107723dfd..263a069f9 100644 --- a/source/opt/block_merge_util.cpp +++ b/source/opt/block_merge_util.cpp @@ -49,6 +49,18 @@ bool IsMerge(IRContext* context, BasicBlock* block) { 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 @@ bool CanMergeWithSuccessor(IRContext* context, BasicBlock* block) { 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 36991ec56..3c874a7ef 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 @@ bool InlinePass::GenInlineCode( 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 @@ bool InlinePass::GenInlineCode( --loop_merge_itr; assert(loop_merge_itr->opcode() == SpvOpLoopMerge); std::unique_ptr 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 efbd243b6..876105c27 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 #include +#include #include #include #include @@ -28,7 +28,7 @@ namespace val { enum BlockType : uint32_t { kBlockTypeUndefined, - kBlockTypeHeader, + kBlockTypeSelection, kBlockTypeLoop, kBlockTypeMerge, kBlockTypeBreak, diff --git a/source/val/function.cpp b/source/val/function.cpp index 876a6081a..028177002 100644 --- a/source/val/function.cpp +++ b/source/val/function.cpp @@ -14,9 +14,8 @@ #include "source/val/function.h" -#include - #include +#include #include #include #include @@ -99,7 +98,7 @@ spv_result_t Function::RegisterLoopMerge(uint32_t merge_id, 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 @@ int Function::GetBlockDepth(BasicBlock* bb) { 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 4801fc58a..acce1fbde 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 #include #include @@ -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 @@ spv_result_t StructuredControlFlowChecks( << header_name << " " << 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(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 8173e7269..5892848e2 100644 --- a/test/fuzz/transformation_add_dead_continue_test.cpp +++ b/test/fuzz/transformation_add_dead_continue_test.cpp @@ -209,117 +209,6 @@ TEST(TransformationAddDeadContinueTest, SimpleExample) { 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 @@ TEST(TransformationAddDeadContinueTest, Miscellaneous2) { 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 9d3fde002..b489f7159 100644 --- a/test/fuzz/transformation_copy_object_test.cpp +++ b/test/fuzz/transformation_copy_object_test.cpp @@ -291,7 +291,7 @@ TEST(TransformationCopyObjectTest, CheckIllegalCases) { %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 75e117ae8..41b611667 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 @@ const std::string kComplexShader = R"( %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 @@ TEST(TransformationReplaceIdWithSynonymTest, LegalTransformations) { %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 9e5197d5b..e7303baab 100644 --- a/test/opt/aggressive_dead_code_elim_test.cpp +++ b/test/opt/aggressive_dead_code_elim_test.cpp @@ -5932,7 +5932,6 @@ OpBranch %42 %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 f87fd8048..11fba736c 100644 --- a/test/opt/block_merge_test.cpp +++ b/test/opt/block_merge_test.cpp @@ -447,10 +447,53 @@ OpBranch %header 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(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 5d41bdc10..e61286762 100644 --- a/test/opt/dead_branch_elim_test.cpp +++ b/test/opt/dead_branch_elim_test.cpp @@ -2798,7 +2798,9 @@ OpReturn OpFunctionEnd )"; - SinglePassRunAndMatch(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(predefs + body, false); } TEST_F(DeadBranchElimTest, SelectionMergeWithNestedLoop) { @@ -2942,7 +2944,9 @@ OpReturn OpFunctionEnd )"; - SinglePassRunAndMatch(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(body, false); } TEST_F(DeadBranchElimTest, UnreachableMergeAndContinueSameBlock) { diff --git a/test/opt/inline_test.cpp b/test/opt/inline_test.cpp index fac49ca6a..f44c04a05 100644 --- a/test/opt/inline_test.cpp +++ b/test/opt/inline_test.cpp @@ -1819,7 +1819,7 @@ OpFunctionEnd %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 @@ OpFunctionEnd 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 @@ OpBranch %19 %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 f10d118e7..7afbb4cf4 100644 --- a/test/opt/local_ssa_elim_test.cpp +++ b/test/opt/local_ssa_elim_test.cpp @@ -193,7 +193,24 @@ OpDecorate %fo Location 0 )"; 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 @@ OpBranch %31 %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 @@ OpStore %f %40 %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 @@ OpReturn 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(predefs + before, predefs + after, true, - true); + SinglePassRunAndMatch(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 41d796fd9..56b0b76f4 100644 --- a/test/opt/loop_optimizations/fusion_legal.cpp +++ b/test/opt/loop_optimizations/fusion_legal.cpp @@ -3177,7 +3177,7 @@ TEST_F(FusionLegalTest, OuterloopWithBreakContinueInInner) { %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 @@ TEST_F(FusionLegalTest, OuterloopWithBreakContinueInInner) { 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 a30568063..411816960 100644 --- a/test/opt/pass_merge_return_test.cpp +++ b/test/opt/pass_merge_return_test.cpp @@ -1390,7 +1390,6 @@ TEST_F(MergeReturnPassTest, SerialLoopsUpdateBlockMapping) { OpLoopMerge %36 %40 None OpBranch %35 %35 = OpLabel - OpSelectionMerge %40 None OpBranchConditional %77 %39 %40 %39 = OpLabel OpReturnValue %16 @@ -1402,7 +1401,6 @@ TEST_F(MergeReturnPassTest, SerialLoopsUpdateBlockMapping) { OpLoopMerge %45 %49 None OpBranch %44 %44 = OpLabel - OpSelectionMerge %49 None OpBranchConditional %77 %48 %49 %48 = OpLabel OpReturnValue %16 @@ -1415,7 +1413,6 @@ TEST_F(MergeReturnPassTest, SerialLoopsUpdateBlockMapping) { OpLoopMerge %64 %68 None OpBranch %63 %63 = OpLabel - OpSelectionMerge %68 None OpBranchConditional %77 %67 %68 %67 = OpLabel OpReturnValue %16 @@ -1813,12 +1810,14 @@ TEST_F(MergeReturnPassTest, PhiInSecondMerge) { 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 a650d3bec..59f280369 100644 --- a/test/reduce/reducer_test.cpp +++ b/test/reduce/reducer_test.cpp @@ -125,19 +125,21 @@ TEST(ReducerTest, ExprToConstantAndRemoveUnreferenced) { %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 @@ TEST(ReducerTest, ExprToConstantAndRemoveUnreferenced) { 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 @@ const std::string kShaderWithLoopsDivAndMul = R"( 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 4051bac1e..2981c2ed8 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 reduce { 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 @@ TEST(ValidationDuringReductionTest, CheckInvalidPassMakesNoProgress) { %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 @@ TEST(ValidationDuringReductionTest, CheckNotAlwaysInvalidCanMakeProgress) { %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 @@ TEST(ValidationDuringReductionTest, CheckNotAlwaysInvalidCanMakeProgress) { %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 f06f36c14..22dd11792 100644 --- a/test/val/val_cfg_test.cpp +++ b/test/val/val_cfg_test.cpp @@ -23,7 +23,6 @@ #include #include "gmock/gmock.h" - #include "source/diagnostic.h" #include "source/spirv_target_env.h" #include "source/val/validate.h" @@ -1355,7 +1354,7 @@ std::string GetReachableMergeAndContinue(SpvCapability cap, } 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 @@ TEST_P(ValidateCFG, BackEdgeBlockDoesntPostDominateContinueTargetBad) { 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({loop2, exit}); str += loop2 >> std::vector({loop2, loop2_merge}); - str += loop2_merge >> std::vector({be_block, exit}); + str += loop2_merge >> loop1_cont; + str += loop1_cont >> std::vector({be_block, exit}); str += be_block >> loop1; str += exit; str += "OpFunctionEnd"; @@ -1678,7 +1679,7 @@ TEST_P(ValidateCFG, BackEdgeBlockDoesntPostDominateContinueTargetBad) { 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 @@ TEST_P(ValidateCFG, 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 @@ TEST_P(ValidateCFG, ContinueTargetCanBeMergeBlockForNestedStructureGood) { 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 @@ OpBranch %7 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 @@ OpFunctionEnd CompileSuccessfully(text); EXPECT_EQ(SPV_ERROR_INVALID_CFG, ValidateInstructions()); EXPECT_THAT(getDiagnosticString(), - HasSubstr("block 13[%13] exits the loop headed by " + HasSubstr("block 15[%15] exits the loop headed by " "10[%10], but not via a structured exit")); } @@ -3726,9 +3737,11 @@ OpBranchConditional %undef %11 %12 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 @@ OpFunctionEnd 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