Handle decorations better in some optimizations (#2716)

There are a couple spots where we are not looking at decorations when we should.

1. Value numbering is suppose to assign a different value number to ids if they have different decorations.  However that is not being done for OpCopyObject and OpPhi.

1. Instruction simplification is propagating OpCopyObject instruction without checking for decorations.  It should only do that if no decorations are being lost.

Add a new function to the decoration manager to check if the decorations of one id are a subset of the decorations of another.

Fixes #2715.

source/opt/decoration_manager.cpp

@@ -22,6 +22,33 @@
 
 #include "source/opt/ir_context.h"
 
+namespace {
+using InstructionVector = std::vector<const spvtools::opt::Instruction*>;
+using DecorationSet = std::set<std::u32string>;
+
+// Returns true if |a| is a subet of |b|.
+bool IsSubset(const DecorationSet& a, const DecorationSet& b) {
+  auto it1 = a.begin();
+  auto it2 = b.begin();
+
+  while (it1 != a.end()) {
+    if (it2 == b.end() || *it1 < *it2) {
+      // |*it1| is in |a|, but not in |b|.
+      return false;
+    }
+    if (*it1 == *it2) {
+      // Found the element move to the next one.
+      it1++;
+      it2++;
+    } else /* *it1 > *it2 */ {
+      // Did not find |*it1| yet, check the next element in |b|.
+      it2++;
+    }
+  }
+  return true;
+}
+}  // namespace
+
 namespace spvtools {
 namespace opt {
 namespace analysis {
@@ -160,18 +187,15 @@ std::vector<const Instruction*> DecorationManager::GetDecorationsFor(
 
 bool DecorationManager::HaveTheSameDecorations(uint32_t id1,
                                                uint32_t id2) const {
-  using InstructionList = std::vector<const Instruction*>;
+  const InstructionVector decorations_for1 = GetDecorationsFor(id1, false);
-  using DecorationSet = std::set<std::u32string>;
+  const InstructionVector decorations_for2 = GetDecorationsFor(id2, false);
-
-  const InstructionList decorations_for1 = GetDecorationsFor(id1, false);
-  const InstructionList decorations_for2 = GetDecorationsFor(id2, false);
 
   // This function splits the decoration instructions into different sets,
   // based on their opcode; only OpDecorate, OpDecorateId,
   // OpDecorateStringGOOGLE, and OpMemberDecorate are considered, the other
   // opcodes are ignored.
   const auto fillDecorationSets =
-      [](const InstructionList& decoration_list, DecorationSet* decorate_set,
+      [](const InstructionVector& decoration_list, DecorationSet* decorate_set,
          DecorationSet* decorate_id_set, DecorationSet* decorate_string_set,
          DecorationSet* member_decorate_set) {
         for (const Instruction* inst : decoration_list) {
@@ -227,6 +251,73 @@ bool DecorationManager::HaveTheSameDecorations(uint32_t id1,
   return result;
 }
 
+bool DecorationManager::HaveSubsetOfDecorations(uint32_t id1,
+                                                uint32_t id2) const {
+  const InstructionVector decorations_for1 = GetDecorationsFor(id1, false);
+  const InstructionVector decorations_for2 = GetDecorationsFor(id2, false);
+
+  // This function splits the decoration instructions into different sets,
+  // based on their opcode; only OpDecorate, OpDecorateId,
+  // OpDecorateStringGOOGLE, and OpMemberDecorate are considered, the other
+  // opcodes are ignored.
+  const auto fillDecorationSets =
+      [](const InstructionVector& decoration_list, DecorationSet* decorate_set,
+         DecorationSet* decorate_id_set, DecorationSet* decorate_string_set,
+         DecorationSet* member_decorate_set) {
+        for (const Instruction* inst : decoration_list) {
+          std::u32string decoration_payload;
+          // Ignore the opcode and the target as we do not want them to be
+          // compared.
+          for (uint32_t i = 1u; i < inst->NumInOperands(); ++i) {
+            for (uint32_t word : inst->GetInOperand(i).words) {
+              decoration_payload.push_back(word);
+            }
+          }
+
+          switch (inst->opcode()) {
+            case SpvOpDecorate:
+              decorate_set->emplace(std::move(decoration_payload));
+              break;
+            case SpvOpMemberDecorate:
+              member_decorate_set->emplace(std::move(decoration_payload));
+              break;
+            case SpvOpDecorateId:
+              decorate_id_set->emplace(std::move(decoration_payload));
+              break;
+            case SpvOpDecorateStringGOOGLE:
+              decorate_string_set->emplace(std::move(decoration_payload));
+              break;
+            default:
+              break;
+          }
+        }
+      };
+
+  DecorationSet decorate_set_for1;
+  DecorationSet decorate_id_set_for1;
+  DecorationSet decorate_string_set_for1;
+  DecorationSet member_decorate_set_for1;
+  fillDecorationSets(decorations_for1, &decorate_set_for1,
+                     &decorate_id_set_for1, &decorate_string_set_for1,
+                     &member_decorate_set_for1);
+
+  DecorationSet decorate_set_for2;
+  DecorationSet decorate_id_set_for2;
+  DecorationSet decorate_string_set_for2;
+  DecorationSet member_decorate_set_for2;
+  fillDecorationSets(decorations_for2, &decorate_set_for2,
+                     &decorate_id_set_for2, &decorate_string_set_for2,
+                     &member_decorate_set_for2);
+
+  const bool result =
+      IsSubset(decorate_set_for1, decorate_set_for2) &&
+      IsSubset(decorate_id_set_for1, decorate_id_set_for2) &&
+      IsSubset(member_decorate_set_for1, member_decorate_set_for2) &&
+      // Compare string sets last in case the strings are long.
+      IsSubset(decorate_string_set_for1, decorate_string_set_for2);
+  return result;
+}
+
 // TODO(pierremoreau): If OpDecorateId is referencing an OpConstant, one could
 //                     check that the constants are the same rather than just
 //                     looking at the constant ID.

source/opt/decoration_manager.h

@@ -74,6 +74,12 @@ class DecorationManager {
   // instructions that apply the same decorations but to different IDs, still
   // count as being the same.
   bool HaveTheSameDecorations(uint32_t id1, uint32_t id2) const;
+
+  // Returns whether two IDs have the same decorations. Two SpvOpGroupDecorate
+  // instructions that apply the same decorations but to different IDs, still
+  // count as being the same.
+  bool HaveSubsetOfDecorations(uint32_t id1, uint32_t id2) const;
+
   // Returns whether the two decorations instructions are the same and are
   // applying the same decorations; unless |ignore_target| is false, the targets
   // to which they are applied to does not matter, except for the member part.

source/opt/simplification_pass.cpp

@@ -55,8 +55,12 @@ bool SimplificationPass::SimplifyFunction(Function* function) {
             process_phis.insert(inst);
           }
 
-          if (inst->opcode() == SpvOpCopyObject ||
+          bool is_foldable_copy =
-              folder.FoldInstruction(inst)) {
+              inst->opcode() == SpvOpCopyObject &&
+              context()->get_decoration_mgr()->HaveSubsetOfDecorations(
+                  inst->result_id(), inst->GetSingleWordInOperand(0));
+
+          if (is_foldable_copy || folder.FoldInstruction(inst)) {
             modified = true;
             context()->AnalyzeUses(inst);
             get_def_use_mgr()->ForEachUser(inst, [&work_list, &process_phis,
@@ -85,7 +89,13 @@ bool SimplificationPass::SimplifyFunction(Function* function) {
   for (size_t i = 0; i < work_list.size(); ++i) {
     Instruction* inst = work_list[i];
     in_work_list.erase(inst);
-    if (inst->opcode() == SpvOpCopyObject || folder.FoldInstruction(inst)) {
+
+    bool is_foldable_copy =
+        inst->opcode() == SpvOpCopyObject &&
+        context()->get_decoration_mgr()->HaveSubsetOfDecorations(
+            inst->result_id(), inst->GetSingleWordInOperand(0));
+
+    if (is_foldable_copy || folder.FoldInstruction(inst)) {
       modified = true;
       context()->AnalyzeUses(inst);
       get_def_use_mgr()->ForEachUser(

source/opt/value_number_table.cpp

@@ -78,8 +78,12 @@ uint32_t ValueNumberTable::AssignValueNumber(Instruction* inst) {
     return value;
   }
 
+  analysis::DecorationManager* dec_mgr = context()->get_decoration_mgr();
+
   // When we copy an object, the value numbers should be the same.
-  if (inst->opcode() == SpvOpCopyObject) {
+  if (inst->opcode() == SpvOpCopyObject &&
+      dec_mgr->HaveTheSameDecorations(inst->result_id(),
+                                      inst->GetSingleWordInOperand(0))) {
     value = GetValueNumber(inst->GetSingleWordInOperand(0));
     if (value != 0) {
       id_to_value_[inst->result_id()] = value;
@@ -89,7 +93,9 @@ uint32_t ValueNumberTable::AssignValueNumber(Instruction* inst) {
 
   // Phi nodes are a type of copy.  If all of the inputs have the same value
   // number, then we can assign the result of the phi the same value number.
-  if (inst->opcode() == SpvOpPhi) {
+  if (inst->opcode() == SpvOpPhi &&
+      dec_mgr->HaveTheSameDecorations(inst->result_id(),
+                                      inst->GetSingleWordInOperand(0))) {
     value = GetValueNumber(inst->GetSingleWordInOperand(0));
     if (value != 0) {
       for (uint32_t op = 2; op < inst->NumInOperands(); op += 2) {

test/opt/decoration_manager_test.cpp

@@ -1277,6 +1277,232 @@ OpDecorateStringGOOGLE %2 HlslSemanticGOOGLE "hello"
   EXPECT_FALSE(decoManager->HaveTheSameDecorations(1u, 2u));
 }
 
+TEST_F(DecorationManagerTest, SubSetTestOpDecorate1) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpCapability Linkage
+OpMemoryModel Logical GLSL450
+OpDecorate %1 Restrict
+OpDecorate %2 Constant
+OpDecorate %2 Restrict
+OpDecorate %1 Constant
+%u32    = OpTypeInt 32 0
+%1      = OpVariable %u32 Uniform
+%2      = OpVariable %u32 Uniform
+)";
+  DecorationManager* decoManager = GetDecorationManager(spirv);
+  EXPECT_THAT(GetErrorMessage(), "");
+  EXPECT_TRUE(decoManager->HaveSubsetOfDecorations(1u, 2u));
+}
+
+TEST_F(DecorationManagerTest, SubSetTestOpDecorate2) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpCapability Linkage
+OpMemoryModel Logical GLSL450
+OpDecorate %1 Restrict
+OpDecorate %2 Constant
+OpDecorate %2 Restrict
+%u32    = OpTypeInt 32 0
+%1      = OpVariable %u32 Uniform
+%2      = OpVariable %u32 Uniform
+)";
+  DecorationManager* decoManager = GetDecorationManager(spirv);
+  EXPECT_THAT(GetErrorMessage(), "");
+  EXPECT_TRUE(decoManager->HaveSubsetOfDecorations(1u, 2u));
+}
+
+TEST_F(DecorationManagerTest, SubSetTestOpDecorate3) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpCapability Linkage
+OpMemoryModel Logical GLSL450
+OpDecorate %1 Constant
+OpDecorate %2 Constant
+OpDecorate %2 Restrict
+%u32    = OpTypeInt 32 0
+%1      = OpVariable %u32 Uniform
+%2      = OpVariable %u32 Uniform
+)";
+  DecorationManager* decoManager = GetDecorationManager(spirv);
+  EXPECT_THAT(GetErrorMessage(), "");
+  EXPECT_TRUE(decoManager->HaveSubsetOfDecorations(1u, 2u));
+}
+
+TEST_F(DecorationManagerTest, SubSetTestOpDecorate4) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpCapability Linkage
+OpMemoryModel Logical GLSL450
+OpDecorate %1 Restrict
+OpDecorate %2 Constant
+OpDecorate %2 Restrict
+OpDecorate %1 Constant
+%u32    = OpTypeInt 32 0
+%1      = OpVariable %u32 Uniform
+%2      = OpVariable %u32 Uniform
+)";
+  DecorationManager* decoManager = GetDecorationManager(spirv);
+  EXPECT_THAT(GetErrorMessage(), "");
+  EXPECT_TRUE(decoManager->HaveSubsetOfDecorations(2u, 1u));
+}
+
+TEST_F(DecorationManagerTest, SubSetTestOpDecorate5) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpCapability Linkage
+OpMemoryModel Logical GLSL450
+OpDecorate %1 Restrict
+OpDecorate %2 Constant
+OpDecorate %2 Restrict
+%u32    = OpTypeInt 32 0
+%1      = OpVariable %u32 Uniform
+%2      = OpVariable %u32 Uniform
+)";
+  DecorationManager* decoManager = GetDecorationManager(spirv);
+  EXPECT_THAT(GetErrorMessage(), "");
+  EXPECT_FALSE(decoManager->HaveSubsetOfDecorations(2u, 1u));
+}
+
+TEST_F(DecorationManagerTest, SubSetTestOpDecorate6) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpCapability Linkage
+OpMemoryModel Logical GLSL450
+OpDecorate %1 Constant
+OpDecorate %2 Constant
+OpDecorate %2 Restrict
+%u32    = OpTypeInt 32 0
+%1      = OpVariable %u32 Uniform
+%2      = OpVariable %u32 Uniform
+)";
+  DecorationManager* decoManager = GetDecorationManager(spirv);
+  EXPECT_THAT(GetErrorMessage(), "");
+  EXPECT_FALSE(decoManager->HaveSubsetOfDecorations(2u, 1u));
+}
+
+TEST_F(DecorationManagerTest, SubSetTestOpDecorate7) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpCapability Linkage
+OpMemoryModel Logical GLSL450
+OpDecorate %1 Constant
+OpDecorate %2 Constant
+OpDecorate %2 Restrict
+OpDecorate %1 Invariant
+%u32    = OpTypeInt 32 0
+%1      = OpVariable %u32 Uniform
+%2      = OpVariable %u32 Uniform
+)";
+  DecorationManager* decoManager = GetDecorationManager(spirv);
+  EXPECT_THAT(GetErrorMessage(), "");
+  EXPECT_FALSE(decoManager->HaveSubsetOfDecorations(2u, 1u));
+  EXPECT_FALSE(decoManager->HaveSubsetOfDecorations(1u, 2u));
+}
+
+TEST_F(DecorationManagerTest, SubSetTestOpMemberDecorate1) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpCapability Linkage
+OpMemoryModel Logical GLSL450
+OpMemberDecorate %1 0 Offset 0
+OpMemberDecorate %1 0 Offset 4
+OpMemberDecorate %2 0 Offset 0
+OpMemberDecorate %2 0 Offset 4
+%u32    = OpTypeInt 32 0
+%1      = OpTypeStruct %u32 %u32 %u32
+%2      = OpTypeStruct %u32 %u32 %u32
+)";
+  DecorationManager* decoManager = GetDecorationManager(spirv);
+  EXPECT_THAT(GetErrorMessage(), "");
+  EXPECT_TRUE(decoManager->HaveSubsetOfDecorations(1u, 2u));
+  EXPECT_TRUE(decoManager->HaveSubsetOfDecorations(2u, 1u));
+}
+
+TEST_F(DecorationManagerTest, SubSetTestOpMemberDecorate2) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpCapability Linkage
+OpMemoryModel Logical GLSL450
+OpMemberDecorate %1 0 Offset 0
+OpMemberDecorate %2 0 Offset 0
+OpMemberDecorate %2 0 Offset 4
+%u32    = OpTypeInt 32 0
+%1      = OpTypeStruct %u32 %u32 %u32
+%2      = OpTypeStruct %u32 %u32 %u32
+)";
+  DecorationManager* decoManager = GetDecorationManager(spirv);
+  EXPECT_THAT(GetErrorMessage(), "");
+  EXPECT_TRUE(decoManager->HaveSubsetOfDecorations(1u, 2u));
+  EXPECT_FALSE(decoManager->HaveSubsetOfDecorations(2u, 1u));
+}
+
+TEST_F(DecorationManagerTest, SubSetTestOpDecorateId1) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpCapability Linkage
+OpMemoryModel Logical GLSL450
+OpDecorateId %1 AlignmentId %2
+%u32    = OpTypeInt 32 0
+%1      = OpVariable %u32 Uniform
+%3      = OpVariable %u32 Uniform
+%2      = OpSpecConstant %u32 0
+)";
+  DecorationManager* decoManager = GetDecorationManager(spirv);
+  EXPECT_THAT(GetErrorMessage(), "");
+  EXPECT_FALSE(decoManager->HaveSubsetOfDecorations(1u, 3u));
+  EXPECT_TRUE(decoManager->HaveSubsetOfDecorations(3u, 1u));
+}
+
+TEST_F(DecorationManagerTest, SubSetTestOpDecorateId2) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpCapability Linkage
+OpMemoryModel Logical GLSL450
+OpDecorateId %1 AlignmentId %2
+OpDecorateId %3 AlignmentId %4
+%u32    = OpTypeInt 32 0
+%1      = OpVariable %u32 Uniform
+%3      = OpVariable %u32 Uniform
+%2      = OpSpecConstant %u32 0
+%4      = OpSpecConstant %u32 1
+)";
+  DecorationManager* decoManager = GetDecorationManager(spirv);
+  EXPECT_THAT(GetErrorMessage(), "");
+  EXPECT_FALSE(decoManager->HaveSubsetOfDecorations(1u, 3u));
+  EXPECT_FALSE(decoManager->HaveSubsetOfDecorations(3u, 1u));
+}
+
+TEST_F(DecorationManagerTest, SubSetTestOpDecorateString1) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpCapability Linkage
+OpExtension "SPV_GOOGLE_hlsl_functionality1"
+OpExtension "SPV_GOOGLE_decorate_string"
+OpMemoryModel Logical GLSL450
+OpDecorateString %1 HlslSemanticGOOGLE "hello"
+OpDecorateString %2 HlslSemanticGOOGLE "world"
+)";
+  DecorationManager* decoManager = GetDecorationManager(spirv);
+  EXPECT_THAT(GetErrorMessage(), "");
+  EXPECT_FALSE(decoManager->HaveSubsetOfDecorations(1u, 2u));
+  EXPECT_FALSE(decoManager->HaveSubsetOfDecorations(2u, 1u));
+}
+
+TEST_F(DecorationManagerTest, SubSetTestOpDecorateString2) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpCapability Linkage
+OpExtension "SPV_GOOGLE_hlsl_functionality1"
+OpExtension "SPV_GOOGLE_decorate_string"
+OpMemoryModel Logical GLSL450
+OpDecorateString %1 HlslSemanticGOOGLE "hello"
+)";
+  DecorationManager* decoManager = GetDecorationManager(spirv);
+  EXPECT_THAT(GetErrorMessage(), "");
+  EXPECT_FALSE(decoManager->HaveSubsetOfDecorations(1u, 2u));
+  EXPECT_TRUE(decoManager->HaveSubsetOfDecorations(2u, 1u));
+}
 }  // namespace
 }  // namespace analysis
 }  // namespace opt

test/opt/simplification_test.cpp

@@ -202,6 +202,83 @@ TEST_F(SimplificationTest, ThroughLoops) {
   SinglePassRunAndMatch<SimplificationPass>(text, false);
 }
 
+TEST_F(SimplificationTest, CopyObjectWithDecorations1) {
+  // Don't simplify OpCopyObject if the result id has a decoration that the
+  // operand does not.
+  const std::string text = R"(OpCapability Shader
+OpCapability ShaderNonUniformEXT
+%1 = OpExtInstImport "GLSL.std.450"
+OpMemoryModel Logical GLSL450
+OpEntryPoint Fragment %2 "main"
+OpExecutionMode %2 OriginUpperLeft
+OpSource GLSL 430
+OpSourceExtension "GL_GOOGLE_cpp_style_line_directive"
+OpSourceExtension "GL_GOOGLE_include_directive"
+OpDecorate %3 NonUniformEXT
+%void = OpTypeVoid
+%5 = OpTypeFunction %void
+%int = OpTypeInt 32 1
+%2 = OpFunction %void None %5
+%7 = OpLabel
+%8 = OpUndef %int
+%3 = OpCopyObject %int %8
+%9 = OpIAdd %int %3 %3
+OpReturn
+OpFunctionEnd
+)";
+
+  SinglePassRunAndCheck<SimplificationPass>(text, text, false);
+}
+
+TEST_F(SimplificationTest, CopyObjectWithDecorations2) {
+  // Simplify OpCopyObject if the result id is a subset of the decorations of
+  // the operand.
+  const std::string before = R"(OpCapability Shader
+OpCapability ShaderNonUniformEXT
+%1 = OpExtInstImport "GLSL.std.450"
+OpMemoryModel Logical GLSL450
+OpEntryPoint Fragment %2 "main"
+OpExecutionMode %2 OriginUpperLeft
+OpSource GLSL 430
+OpSourceExtension "GL_GOOGLE_cpp_style_line_directive"
+OpSourceExtension "GL_GOOGLE_include_directive"
+OpDecorate %3 NonUniformEXT
+%void = OpTypeVoid
+%5 = OpTypeFunction %void
+%int = OpTypeInt 32 1
+%2 = OpFunction %void None %5
+%7 = OpLabel
+%3 = OpUndef %int
+%8 = OpCopyObject %int %3
+%9 = OpIAdd %int %8 %8
+OpReturn
+OpFunctionEnd
+)";
+
+  const std::string after = R"(OpCapability Shader
+OpCapability ShaderNonUniformEXT
+%1 = OpExtInstImport "GLSL.std.450"
+OpMemoryModel Logical GLSL450
+OpEntryPoint Fragment %2 "main"
+OpExecutionMode %2 OriginUpperLeft
+OpSource GLSL 430
+OpSourceExtension "GL_GOOGLE_cpp_style_line_directive"
+OpSourceExtension "GL_GOOGLE_include_directive"
+OpDecorate %3 NonUniformEXT
+%void = OpTypeVoid
+%5 = OpTypeFunction %void
+%int = OpTypeInt 32 1
+%2 = OpFunction %void None %5
+%7 = OpLabel
+%3 = OpUndef %int
+%9 = OpIAdd %int %3 %3
+OpReturn
+OpFunctionEnd
+)";
+
+  SinglePassRunAndCheck<SimplificationPass>(before, after, false);
+}
+
 }  // namespace
 }  // namespace opt
 }  // namespace spvtools

test/opt/value_table_test.cpp

@@ -455,6 +455,34 @@ TEST_F(ValueTableTest, CopyObject) {
   EXPECT_EQ(vtable.GetValueNumber(inst1), vtable.GetValueNumber(inst2));
 }
 
+TEST_F(ValueTableTest, CopyObjectWitDecoration) {
+  const std::string text = R"(
+               OpCapability Shader
+          %1 = OpExtInstImport "GLSL.std.450"
+               OpMemoryModel Logical GLSL450
+               OpEntryPoint Fragment %2 "main"
+               OpExecutionMode %2 OriginUpperLeft
+               OpSource GLSL 430
+               OpDecorate %3 NonUniformEXT
+          %4 = OpTypeVoid
+          %5 = OpTypeFunction %4
+          %6 = OpTypeFloat 32
+          %7 = OpTypePointer Function %6
+          %2 = OpFunction %4 None %5
+          %8 = OpLabel
+          %9 = OpVariable %7 Function
+         %10 = OpLoad %6 %9
+          %3 = OpCopyObject %6 %10
+               OpReturn
+               OpFunctionEnd
+  )";
+  auto context = BuildModule(SPV_ENV_UNIVERSAL_1_2, nullptr, text);
+  ValueNumberTable vtable(context.get());
+  Instruction* inst1 = context->get_def_use_mgr()->GetDef(10);
+  Instruction* inst2 = context->get_def_use_mgr()->GetDef(3);
+  EXPECT_NE(vtable.GetValueNumber(inst1), vtable.GetValueNumber(inst2));
+}
+
 // Test that a phi where the operands have the same value assigned that value
 // to the result of the phi.
 TEST_F(ValueTableTest, PhiTest1) {
@@ -495,6 +523,45 @@ TEST_F(ValueTableTest, PhiTest1) {
   EXPECT_EQ(vtable.GetValueNumber(inst1), vtable.GetValueNumber(phi));
 }
 
+TEST_F(ValueTableTest, PhiTest1WithDecoration) {
+  const std::string text = R"(
+               OpCapability Shader
+          %1 = OpExtInstImport "GLSL.std.450"
+               OpMemoryModel Logical GLSL450
+               OpEntryPoint Fragment %2 "main"
+               OpExecutionMode %2 OriginUpperLeft
+               OpSource GLSL 430
+               OpDecorate %3 NonUniformEXT
+          %4 = OpTypeVoid
+          %5 = OpTypeFunction %5
+          %6 = OpTypeFloat 32
+          %7 = OpTypePointer Uniform %6
+          %8 = OpTypeBool
+          %9 = OpConstantTrue %8
+          %10 = OpVariable %7 Uniform
+          %2 = OpFunction %4 None %5
+         %11 = OpLabel
+               OpBranchConditional %9 %12 %13
+         %12 = OpLabel
+         %14 = OpLoad %6 %10
+               OpBranch %15
+         %13 = OpLabel
+         %16 = OpLoad %6 %10
+               OpBranch %15
+         %15 = OpLabel
+         %3 = OpPhi %6 %14 %12 %16 %13
+               OpReturn
+               OpFunctionEnd
+  )";
+  auto context = BuildModule(SPV_ENV_UNIVERSAL_1_2, nullptr, text);
+  ValueNumberTable vtable(context.get());
+  Instruction* inst1 = context->get_def_use_mgr()->GetDef(14);
+  Instruction* inst2 = context->get_def_use_mgr()->GetDef(16);
+  Instruction* phi = context->get_def_use_mgr()->GetDef(3);
+  EXPECT_EQ(vtable.GetValueNumber(inst1), vtable.GetValueNumber(inst2));
+  EXPECT_NE(vtable.GetValueNumber(inst1), vtable.GetValueNumber(phi));
+}
+
 // When the values for the inputs to a phi do not match, then the phi should
 // have its own value number.
 TEST_F(ValueTableTest, PhiTest2) {