diff --git a/source/opt/optimizer.cpp b/source/opt/optimizer.cpp index f9919e8c8..b72bd3c21 100644 --- a/source/opt/optimizer.cpp +++ b/source/opt/optimizer.cpp @@ -137,8 +137,7 @@ Optimizer& Optimizer::RegisterLegalizationPasses() { } Optimizer& Optimizer::RegisterPerformancePasses() { - return RegisterPass(CreateRemoveDuplicatesPass()) - .RegisterPass(CreateMergeReturnPass()) + return RegisterPass(CreateMergeReturnPass()) .RegisterPass(CreateInlineExhaustivePass()) .RegisterPass(CreateAggressiveDCEPass()) .RegisterPass(CreateLocalSingleBlockLoadStoreElimPass()) @@ -173,8 +172,7 @@ Optimizer& Optimizer::RegisterPerformancePasses() { } Optimizer& Optimizer::RegisterSizePasses() { - return RegisterPass(CreateRemoveDuplicatesPass()) - .RegisterPass(CreateMergeReturnPass()) + return RegisterPass(CreateMergeReturnPass()) .RegisterPass(CreateInlineExhaustivePass()) .RegisterPass(CreateAggressiveDCEPass()) .RegisterPass(CreateScalarReplacementPass()) diff --git a/source/opt/remove_duplicates_pass.cpp b/source/opt/remove_duplicates_pass.cpp index 753f23245..0a54d76ea 100644 --- a/source/opt/remove_duplicates_pass.cpp +++ b/source/opt/remove_duplicates_pass.cpp @@ -105,22 +105,6 @@ bool RemoveDuplicatesPass::RemoveDuplicateTypes( return modified; } - std::unordered_set types_ids_of_resources; - for (auto& decoration_inst : ir_context->annotations()) { - if (decoration_inst.opcode() != SpvOpDecorate) { - continue; - } - - if (decoration_inst.GetSingleWordInOperand(1) != SpvDecorationBinding) { - continue; - } - - uint32_t var_id = decoration_inst.GetSingleWordInOperand(0); - ir::Instruction* var_inst = ir_context->get_def_use_mgr()->GetDef(var_id); - uint32_t type_id = var_inst->type_id(); - AddStructuresToSet(type_id, ir_context, &types_ids_of_resources); - } - std::vector visited_types; std::vector to_delete; for (auto* i = &*ir_context->types_values_begin(); i; i = i->NextNode()) { @@ -132,26 +116,12 @@ bool RemoveDuplicatesPass::RemoveDuplicateTypes( // Is the current type equal to one of the types we have aready visited? SpvId id_to_keep = 0u; - bool i_is_resource_type = types_ids_of_resources.count(i->result_id()) != 0; - // TODO(dneto0): Use a trie to avoid quadratic behaviour? Extract the // ResultIdTrie from unify_const_pass.cpp for this. - for (auto& j : visited_types) { - if (!j) { - continue; - } - + for (auto j : visited_types) { if (AreTypesEqual(*i, *j, ir_context)) { - if (!i_is_resource_type) { - id_to_keep = j->result_id(); - break; - } else if (!types_ids_of_resources.count(j->result_id())) { - ir_context->KillNamesAndDecorates(j->result_id()); - ir_context->ReplaceAllUsesWith(j->result_id(), i->result_id()); - modified = true; - to_delete.emplace_back(j); - j = nullptr; - } + id_to_keep = j->result_id(); + break; } } @@ -172,7 +142,7 @@ bool RemoveDuplicatesPass::RemoveDuplicateTypes( } return modified; -} // namespace opt +} // TODO(pierremoreau): Duplicate decoration groups should be removed. For // example, in @@ -232,31 +202,5 @@ bool RemoveDuplicatesPass::AreTypesEqual(const Instruction& inst1, return false; } -void RemoveDuplicatesPass::AddStructuresToSet( - uint32_t type_id, ir::IRContext* ctx, - std::unordered_set* set_of_ids) const { - ir::Instruction* type_inst = ctx->get_def_use_mgr()->GetDef(type_id); - switch (type_inst->opcode()) { - case SpvOpTypeStruct: - set_of_ids->insert(type_id); - for (uint32_t i = 0; i < type_inst->NumInOperands(); ++i) { - AddStructuresToSet(type_inst->GetSingleWordInOperand(i), ctx, - set_of_ids); - } - break; - case SpvOpTypeArray: - case SpvOpTypeRuntimeArray: - set_of_ids->insert(type_id); - AddStructuresToSet(type_inst->GetSingleWordInOperand(0), ctx, set_of_ids); - break; - case SpvOpTypePointer: - set_of_ids->insert(type_id); - AddStructuresToSet(type_inst->GetSingleWordInOperand(1), ctx, set_of_ids); - break; - default: - break; - } -} - } // namespace opt } // namespace spvtools diff --git a/source/opt/remove_duplicates_pass.h b/source/opt/remove_duplicates_pass.h index c55d4cf59..d766f6733 100644 --- a/source/opt/remove_duplicates_pass.h +++ b/source/opt/remove_duplicates_pass.h @@ -59,11 +59,6 @@ class RemoveDuplicatesPass : public Pass { // // Returns true if the module was modified, false otherwise. bool RemoveDuplicateDecorations(ir::IRContext* ir_context) const; - - // Adds |type_id| to |set_of_ids| if |type_id| is the id of a a structure. - // Adds any structures that are subtypes of |type_id| to |set_of_ids|. - void AddStructuresToSet(uint32_t type_id, ir::IRContext* ctx, - std::unordered_set* set_of_ids) const; }; } // namespace opt diff --git a/test/opt/pass_remove_duplicates_test.cpp b/test/opt/pass_remove_duplicates_test.cpp index d944be1b9..84aa9f8f3 100644 --- a/test/opt/pass_remove_duplicates_test.cpp +++ b/test/opt/pass_remove_duplicates_test.cpp @@ -307,6 +307,9 @@ OpGroupDecorate %2 %4 EXPECT_EQ(GetErrorMessage(), ""); } +// Test what happens when a type is a resource type. For now we are merging +// them, but, if we want to merge types and make reflection work (issue #1372), +// we will not be able to merge %2 and %3 below. TEST_F(RemoveDuplicatesTest, DontMergeNestedResourceTypes) { const std::string spirv = R"(OpCapability Shader OpMemoryModel Logical GLSL450 @@ -334,10 +337,33 @@ OpDecorate %4 Binding 0 %4 = OpVariable %7 Uniform )"; - EXPECT_EQ(RunPass(spirv), spirv); + const std::string result = R"(OpCapability Shader +OpMemoryModel Logical GLSL450 +OpSource HLSL 600 +OpName %1 "PositionAdjust" +OpMemberName %1 0 "XAdjust" +OpMemberName %3 0 "AdjustXYZ" +OpMemberName %3 1 "AdjustDir" +OpName %4 "Constants" +OpMemberDecorate %1 0 Offset 0 +OpMemberDecorate %3 0 Offset 0 +OpMemberDecorate %3 1 Offset 16 +OpDecorate %3 Block +OpDecorate %4 DescriptorSet 0 +OpDecorate %4 Binding 0 +%5 = OpTypeFloat 32 +%6 = OpTypeVector %5 3 +%1 = OpTypeStruct %6 +%3 = OpTypeStruct %1 %1 +%7 = OpTypePointer Uniform %3 +%4 = OpVariable %7 Uniform +)"; + + EXPECT_EQ(RunPass(spirv), result); EXPECT_EQ(GetErrorMessage(), ""); } +// See comment for DontMergeNestedResourceTypes. TEST_F(RemoveDuplicatesTest, DontMergeResourceTypes) { const std::string spirv = R"(OpCapability Shader OpMemoryModel Logical GLSL450 @@ -363,10 +389,30 @@ OpDecorate %4 Binding 0 %4 = OpVariable %8 Uniform )"; - EXPECT_EQ(RunPass(spirv), spirv); + const std::string result = R"(OpCapability Shader +OpMemoryModel Logical GLSL450 +OpSource HLSL 600 +OpName %1 "PositionAdjust" +OpMemberName %1 0 "XAdjust" +OpName %3 "Constants" +OpMemberDecorate %1 0 Offset 0 +OpDecorate %3 DescriptorSet 0 +OpDecorate %3 Binding 0 +OpDecorate %4 DescriptorSet 1 +OpDecorate %4 Binding 0 +%5 = OpTypeFloat 32 +%6 = OpTypeVector %5 3 +%1 = OpTypeStruct %6 +%7 = OpTypePointer Uniform %1 +%3 = OpVariable %7 Uniform +%4 = OpVariable %7 Uniform +)"; + + EXPECT_EQ(RunPass(spirv), result); EXPECT_EQ(GetErrorMessage(), ""); } +// See comment for DontMergeNestedResourceTypes. TEST_F(RemoveDuplicatesTest, DontMergeResourceTypesContainingArray) { const std::string spirv = R"(OpCapability Shader OpMemoryModel Logical GLSL450 @@ -396,7 +442,29 @@ OpDecorate %4 Binding 0 %4 = OpVariable %12 Uniform )"; - EXPECT_EQ(RunPass(spirv), spirv); + const std::string result = R"(OpCapability Shader +OpMemoryModel Logical GLSL450 +OpSource HLSL 600 +OpName %1 "PositionAdjust" +OpMemberName %1 0 "XAdjust" +OpName %3 "Constants" +OpMemberDecorate %1 0 Offset 0 +OpDecorate %3 DescriptorSet 0 +OpDecorate %3 Binding 0 +OpDecorate %4 DescriptorSet 1 +OpDecorate %4 Binding 0 +%5 = OpTypeFloat 32 +%6 = OpTypeVector %5 3 +%1 = OpTypeStruct %6 +%7 = OpTypeInt 32 0 +%8 = OpConstant %7 4 +%9 = OpTypeArray %1 %8 +%11 = OpTypePointer Uniform %9 +%3 = OpVariable %11 Uniform +%4 = OpVariable %11 Uniform +)"; + + EXPECT_EQ(RunPass(spirv), result); EXPECT_EQ(GetErrorMessage(), ""); } @@ -450,6 +518,8 @@ OpDecorate %3 Binding 0 // Test that we merge the type of a resource with a type that is not the type // a resource. The resource type appears second in this case. We must keep // the resource type. +// +// See comment for DontMergeNestedResourceTypes. TEST_F(RemoveDuplicatesTest, MergeResourceTypeWithNonresourceType2) { const std::string spirv = R"(OpCapability Shader OpMemoryModel Logical GLSL450 @@ -476,18 +546,96 @@ OpDecorate %3 Binding 0 const std::string result = R"(OpCapability Shader OpMemoryModel Logical GLSL450 OpSource HLSL 600 -OpName %2 "NormalAdjust" -OpMemberName %2 0 "XDir" +OpName %1 "PositionAdjust" +OpMemberName %1 0 "XAdjust" OpName %3 "Constants" -OpMemberDecorate %2 0 Offset 0 +OpMemberDecorate %1 0 Offset 0 OpDecorate %3 DescriptorSet 0 OpDecorate %3 Binding 0 %4 = OpTypeFloat 32 %5 = OpTypeVector %4 3 -%2 = OpTypeStruct %5 -%7 = OpTypePointer Uniform %2 -%8 = OpVariable %7 Uniform -%3 = OpVariable %7 Uniform +%1 = OpTypeStruct %5 +%6 = OpTypePointer Uniform %1 +%8 = OpVariable %6 Uniform +%3 = OpVariable %6 Uniform +)"; + + EXPECT_EQ(RunPass(spirv), result); + EXPECT_EQ(GetErrorMessage(), ""); +} + +// In this test, %8 and %9 are the same and only %9 is used in a resource. +// However, we cannot merge them unless we also merge %2 and %3, which cannot +// happen because both are used in resources. +// +// If we try to avoid replaces resource types, then remove duplicates should +// have not change in this case. That is not currently implemented. +TEST_F(RemoveDuplicatesTest, MergeResourceTypeWithNonresourceType3) { + const std::string spirv = R"(OpCapability Shader +OpMemoryModel Logical GLSL450 +OpEntryPoint GLCompute %1 "main" +OpSource HLSL 600 +OpName %2 "PositionAdjust" +OpMemberName %2 0 "XAdjust" +OpName %3 "NormalAdjust" +OpMemberName %3 0 "XDir" +OpName %4 "Constants" +OpMemberDecorate %2 0 Offset 0 +OpMemberDecorate %3 0 Offset 0 +OpDecorate %4 DescriptorSet 0 +OpDecorate %4 Binding 0 +OpDecorate %5 DescriptorSet 1 +OpDecorate %5 Binding 0 +%6 = OpTypeFloat 32 +%7 = OpTypeVector %6 3 +%2 = OpTypeStruct %7 +%3 = OpTypeStruct %7 +%8 = OpTypePointer Uniform %3 +%9 = OpTypePointer Uniform %2 +%10 = OpTypeStruct %3 +%11 = OpTypePointer Uniform %10 +%5 = OpVariable %9 Uniform +%4 = OpVariable %11 Uniform +%12 = OpTypeVoid +%13 = OpTypeFunction %12 +%14 = OpTypeInt 32 0 +%15 = OpConstant %14 0 +%1 = OpFunction %12 None %13 +%16 = OpLabel +%17 = OpAccessChain %8 %4 %15 +OpReturn +OpFunctionEnd +)"; + + const std::string result = R"(OpCapability Shader +OpMemoryModel Logical GLSL450 +OpEntryPoint GLCompute %1 "main" +OpSource HLSL 600 +OpName %2 "PositionAdjust" +OpMemberName %2 0 "XAdjust" +OpName %4 "Constants" +OpMemberDecorate %2 0 Offset 0 +OpDecorate %4 DescriptorSet 0 +OpDecorate %4 Binding 0 +OpDecorate %5 DescriptorSet 1 +OpDecorate %5 Binding 0 +%6 = OpTypeFloat 32 +%7 = OpTypeVector %6 3 +%2 = OpTypeStruct %7 +%8 = OpTypePointer Uniform %2 +%10 = OpTypeStruct %2 +%11 = OpTypePointer Uniform %10 +%5 = OpVariable %8 Uniform +%4 = OpVariable %11 Uniform +%12 = OpTypeVoid +%13 = OpTypeFunction %12 +%14 = OpTypeInt 32 0 +%15 = OpConstant %14 0 +%1 = OpFunction %12 None %13 +%16 = OpLabel +%17 = OpAccessChain %8 %4 %15 +OpReturn +OpFunctionEnd )"; EXPECT_EQ(RunPass(spirv), result);