Revert change and stop running remove duplicates.

Revert "Don't merge types of resources"

This reverts commit f393b0e480, but leaves
the tests that were added.  Added new test. These test are the so that,
if someone tries the same change I made, they will see the test that
they need to handle.

Don't run remove duplicates in -O and -Os

Romve duplicates was run to help reduce compile time when looking for
types in the type manager.  I've run compile time test on three sets
of shaders, and the compile time does not seem to change.

It should be safe to remove it.

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())

source/opt/remove_duplicates_pass.cpp

@@ -105,22 +105,6 @@ bool RemoveDuplicatesPass::RemoveDuplicateTypes(
     return modified;
   }
 
-  std::unordered_set<uint32_t> 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<Instruction*> visited_types;
   std::vector<Instruction*> 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<uint32_t>* 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

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<uint32_t>* set_of_ids) const;
 };
 
 }  // namespace opt

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);