ADCE: Remove OpDecorateStringGOOGLE

Also fix a few failures to set "modified" status when removing
global values.

Add OpDecorateStringGOOGLE to decoration ordering

Fixes #1492

source/opt/aggressive_dead_code_elim_pass.cpp

@@ -1,6 +1,7 @@
 // Copyright (c) 2017 The Khronos Group Inc.
 // Copyright (c) 2017 Valve Corporation
 // Copyright (c) 2017 LunarG Inc.
+// Copyright (c) 2018 Google LLC
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -44,6 +45,7 @@ const uint32_t kLoopMergeContinueBlockIdInIdx = 1;
 // SpvOpDecorate
 // SpvOpMemberDecorate
 // SpvOpDecorateId
+// SpvOpDecorateStringGOOGLE
 // SpvOpDecorationGroup
 struct DecorationLess {
   bool operator()(const ir::Instruction* lhs,
@@ -52,32 +54,21 @@ struct DecorationLess {
     SpvOp lhsOp = lhs->opcode();
     SpvOp rhsOp = rhs->opcode();
     if (lhsOp != rhsOp) {
+#define PRIORITY_CASE(opcode)                          \
+  if (lhsOp == opcode && rhsOp != opcode) return true; \
+  if (rhsOp == opcode && lhsOp != opcode) return false;
       // OpGroupDecorate and OpGroupMember decorate are highest priority to
       // eliminate dead targets early and simplify subsequent checks.
-      if (lhsOp == SpvOpGroupDecorate && rhsOp != SpvOpGroupDecorate)
+      PRIORITY_CASE(SpvOpGroupDecorate)
-        return true;
+      PRIORITY_CASE(SpvOpGroupMemberDecorate)
-      if (rhsOp == SpvOpGroupDecorate && lhsOp != SpvOpGroupDecorate)
+      PRIORITY_CASE(SpvOpDecorate)
-        return false;
+      PRIORITY_CASE(SpvOpMemberDecorate)
-      if (lhsOp == SpvOpGroupMemberDecorate &&
+      PRIORITY_CASE(SpvOpDecorateId)
-          rhsOp != SpvOpGroupMemberDecorate)
+      PRIORITY_CASE(SpvOpDecorateStringGOOGLE)
-        return true;
-      if (rhsOp == SpvOpGroupMemberDecorate &&
-          lhsOp != SpvOpGroupMemberDecorate)
-        return false;
-      if (lhsOp == SpvOpDecorate && rhsOp != SpvOpDecorate) return true;
-      if (rhsOp == SpvOpDecorate && lhsOp != SpvOpDecorate) return false;
-      if (lhsOp == SpvOpMemberDecorate && rhsOp != SpvOpMemberDecorate)
-        return true;
-      if (rhsOp == SpvOpMemberDecorate && lhsOp != SpvOpMemberDecorate)
-        return false;
-      if (lhsOp == SpvOpDecorateId && rhsOp != SpvOpDecorateId) return true;
-      if (rhsOp == SpvOpDecorateId && lhsOp != SpvOpDecorateId) return false;
       // OpDecorationGroup is lowest priority to ensure use/def chains remain
       // usable for instructions that target this group.
-      if (lhsOp == SpvOpDecorationGroup && rhsOp != SpvOpDecorationGroup)
+      PRIORITY_CASE(SpvOpDecorationGroup)
-        return true;
+#undef PRIORITY_CASE
-      if (rhsOp == SpvOpDecorationGroup && lhsOp != SpvOpDecorationGroup)
-        return false;
     }
 
     // Fall back to maintain total ordering (compare unique ids).
@@ -603,7 +594,11 @@ bool AggressiveDCEPass::ProcessGlobalValues() {
       case SpvOpDecorate:
       case SpvOpMemberDecorate:
       case SpvOpDecorateId:
-        if (IsTargetDead(annotation)) context()->KillInst(annotation);
+      case SpvOpDecorateStringGOOGLE:
+        if (IsTargetDead(annotation)) {
+          context()->KillInst(annotation);
+          modified = true;
+        }
         break;
       case SpvOpGroupDecorate: {
         // Go through the targets of this group decorate. Remove each dead
@@ -615,12 +610,16 @@ bool AggressiveDCEPass::ProcessGlobalValues() {
           if (IsDead(opInst)) {
             // Don't increment |i|.
             annotation->RemoveOperand(i);
+            modified = true;
           } else {
             i++;
             dead = false;
           }
         }
-        if (dead) context()->KillInst(annotation);
+        if (dead) {
+          context()->KillInst(annotation);
+          modified = true;
+        }
         break;
       }
       case SpvOpGroupMemberDecorate: {
@@ -635,19 +634,25 @@ bool AggressiveDCEPass::ProcessGlobalValues() {
             // Don't increment |i|.
             annotation->RemoveOperand(i + 1);
             annotation->RemoveOperand(i);
+            modified = true;
           } else {
             i += 2;
             dead = false;
           }
         }
-        if (dead) context()->KillInst(annotation);
+        if (dead) {
+          context()->KillInst(annotation);
+          modified = true;
+        }
         break;
       }
       case SpvOpDecorationGroup:
         // By the time we hit decoration groups we've checked everything that
         // can target them. So if they have no uses they must be dead.
-        if (get_def_use_mgr()->NumUsers(annotation) == 0)
+        if (get_def_use_mgr()->NumUsers(annotation) == 0) {
           context()->KillInst(annotation);
+          modified = true;
+        }
         break;
       default:
         assert(false);

test/opt/aggressive_dead_code_elim_test.cpp

@@ -5410,6 +5410,39 @@ OpFunctionEnd
   SinglePassRunAndCheck<opt::AggressiveDCEPass>(text, text, true, true);
 }
 
+TEST_F(AggressiveDCETest, SafelyRemoveDecorateString) {
+  const std::string preamble = R"(OpCapability Shader
+OpExtension "SPV_GOOGLE_hlsl_functionality1"
+OpMemoryModel Logical GLSL450
+OpEntryPoint Fragment %1 "main"
+)";
+
+  const std::string body_before =
+      R"(OpDecorateStringGOOGLE %2 HlslSemanticGOOGLE "FOOBAR"
+%void = OpTypeVoid
+%4 = OpTypeFunction %void
+%uint = OpTypeInt 32 0
+%_ptr_StorageBuffer_uint = OpTypePointer StorageBuffer %uint
+%2 = OpVariable %_ptr_StorageBuffer_uint StorageBuffer
+%1 = OpFunction %void None %4
+%7 = OpLabel
+OpReturn
+OpFunctionEnd
+)";
+
+  const std::string body_after = R"(%void = OpTypeVoid
+%4 = OpTypeFunction %void
+%1 = OpFunction %void None %4
+%7 = OpLabel
+OpReturn
+OpFunctionEnd
+)";
+
+  SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
+  SinglePassRunAndCheck<opt::AggressiveDCEPass>(
+      preamble + body_before, preamble + body_after, true, true);
+}
+
 // TODO(greg-lunarg): Add tests to verify handling of these cases:
 //
 //    Check that logical addressing required