diff options
| -rw-r--r-- | source/fuzz/transformation_add_function.cpp | 20 | ||||
| -rw-r--r-- | test/fuzz/transformation_add_function_test.cpp | 55 |
2 files changed, 21 insertions, 54 deletions
diff --git a/source/fuzz/transformation_add_function.cpp b/source/fuzz/transformation_add_function.cpp index 380d59c4..66b379ef 100644 --- a/source/fuzz/transformation_add_function.cpp +++ b/source/fuzz/transformation_add_function.cpp @@ -488,7 +488,20 @@ bool TransformationAddFunction::TryToAddLoopLimiters( // move on from this loop. continue; } - auto back_edge_block = ir_context->cfg()->block(back_edge_block_id); + + // If the loop's merge block is unreachable, then there are no constraints + // on where the merge block appears in relation to the blocks of the loop. + // This means we need to be careful when adding a branch from the back-edge + // block to the merge block: the branch might make the loop merge reachable, + // and it might then be dominated by the loop header and possibly by other + // blocks in the loop. Since a block needs to appear before those blocks it + // strictly dominates, this could make the module invalid. To avoid this + // problem we bail out in the case where the loop header does not dominate + // the loop merge. + if (!ir_context->GetDominatorAnalysis(added_function) + ->Dominates(loop_header->id(), loop_header->MergeBlockId())) { + return false; + } // Go through the sequence of loop limiter infos and find the one // corresponding to this loop. @@ -560,6 +573,7 @@ bool TransformationAddFunction::TryToAddLoopLimiters( // %t4 = OpLogicalOr %bool %c %t3 // OpBranchConditional %t4 %loop_merge %loop_header + auto back_edge_block = ir_context->cfg()->block(back_edge_block_id); auto back_edge_block_terminator = back_edge_block->terminator(); bool compare_using_greater_than_equal; if (back_edge_block_terminator->opcode() == SpvOpBranch) { @@ -675,10 +689,6 @@ bool TransformationAddFunction::TryToAddLoopLimiters( } // Add the new edge, by changing OpBranch to OpBranchConditional. - // TODO(https://github.com/KhronosGroup/SPIRV-Tools/issues/3162): This - // could be a problem if the merge block was originally unreachable: it - // might now be dominated by other blocks that it appears earlier than in - // the module. back_edge_block_terminator->SetOpcode(SpvOpBranchConditional); back_edge_block_terminator->SetInOperands(opt::Instruction::OperandList( {{SPV_OPERAND_TYPE_ID, {loop_limiter_info.compare_id()}}, diff --git a/test/fuzz/transformation_add_function_test.cpp b/test/fuzz/transformation_add_function_test.cpp index bbd915b0..e0963484 100644 --- a/test/fuzz/transformation_add_function_test.cpp +++ b/test/fuzz/transformation_add_function_test.cpp @@ -2246,7 +2246,7 @@ TEST(TransformationAddFunctionTest, LoopLimitersHeaderIsBackEdgeBlock) { ASSERT_TRUE(IsEqual(env, expected, context.get())); } -TEST(TransformationAddFunctionTest, InfiniteLoop1) { +TEST(TransformationAddFunctionTest, InfiniteLoop) { std::string shader = R"( OpCapability Shader %1 = OpExtInstImport "GLSL.std.450" @@ -2337,54 +2337,11 @@ TEST(TransformationAddFunctionTest, InfiniteLoop1) { loop_limiter_info.set_logical_op_id(105); TransformationAddFunction add_livesafe_function(instructions, 100, 32, {loop_limiter_info}, 0, {}); - ASSERT_TRUE(add_livesafe_function.IsApplicable(context.get(), - transformation_context)); - add_livesafe_function.Apply(context.get(), &transformation_context); - ASSERT_TRUE(IsValid(env, context.get())); - std::string expected = R"( - OpCapability Shader - %1 = OpExtInstImport "GLSL.std.450" - OpMemoryModel Logical GLSL450 - OpEntryPoint Fragment %4 "main" - OpExecutionMode %4 OriginUpperLeft - OpSource ESSL 310 - %2 = OpTypeVoid - %3 = OpTypeFunction %2 - %8 = OpTypeInt 32 1 - %9 = OpTypePointer Function %8 - %11 = OpConstant %8 0 - %18 = OpConstant %8 10 - %19 = OpTypeBool - %26 = OpConstantTrue %19 - %27 = OpConstantFalse %19 - %28 = OpTypeInt 32 0 - %29 = OpTypePointer Function %28 - %30 = OpConstant %28 0 - %31 = OpConstant %28 1 - %32 = OpConstant %28 5 - %22 = OpConstant %8 1 - %4 = OpFunction %2 None %3 - %5 = OpLabel - OpReturn - OpFunctionEnd - %6 = OpFunction %2 None %3 - %7 = OpLabel - %100 = OpVariable %29 Function %30 - %10 = OpVariable %9 Function - OpStore %10 %11 - OpBranch %12 - %12 = OpLabel - %102 = OpLoad %28 %100 - %103 = OpIAdd %28 %102 %31 - OpStore %100 %103 - %104 = OpUGreaterThanEqual %19 %102 %32 - OpLoopMerge %14 %12 None - OpBranchConditional %104 %14 %12 - %14 = OpLabel - OpReturn - OpFunctionEnd - )"; - ASSERT_TRUE(IsEqual(env, expected, context.get())); + + // To make sure the loop's merge block is reachable, it must be dominated by + // the loop header. + ASSERT_FALSE(add_livesafe_function.IsApplicable(context.get(), + transformation_context)); } TEST(TransformationAddFunctionTest, UnreachableContinueConstruct) { |