Removing HLSLCounterBuffer decorations when not needed. (#1954)

The HlslCounterBufferGOOGLE that was introduced changed the OpDecorateId
so that is can now reference an id other than the target.  If that other
id is used only in the decoration, then the definition of the id will be
removed because decoration do not count as real uses.

However, if the target of the decoration is still live the decoration
will not be removed.  This leaves a reference to an id that is not
defined.

There are two solutions to consider.  The first is that is the decoration
is kept, then the definition of the id should be kept live.  Implementing
this change would be involved because the way ADCE handles decorations
will have to be reimplemented.

The other solution is to remove the decoration the id is otherwise dead.
This works for this specific case.  Also this is the more desirable
behaviour in this case.  The id will always be the id of a variable that
belongs to a descriptor set.  If that variable is not bound and we do
not remove it, the driver will complain.

I chose to implement the second solution.  The first will be left to when
a case for it comes up.

Fixes https://github.com/KhronosGroup/SPIRV-Tools/issues/1885.

source/opt/aggressive_dead_code_elim_pass.cpp

@@ -627,13 +627,31 @@ bool AggressiveDCEPass::ProcessGlobalValues() {
     switch (annotation->opcode()) {
       case SpvOpDecorate:
       case SpvOpMemberDecorate:
-      case SpvOpDecorateId:
       case SpvOpDecorateStringGOOGLE:
         if (IsTargetDead(annotation)) {
           context()->KillInst(annotation);
           modified = true;
         }
         break;
+      case SpvOpDecorateId:
+        if (IsTargetDead(annotation)) {
+          context()->KillInst(annotation);
+          modified = true;
+        } else {
+          if (annotation->GetSingleWordInOperand(1) ==
+              SpvDecorationHlslCounterBufferGOOGLE) {
+            // HlslCounterBuffer will reference an id other than the target.
+            // If that id is dead, then the decoration can be removed as well.
+            uint32_t counter_buffer_id = annotation->GetSingleWordInOperand(2);
+            Instruction* counter_buffer_inst =
+                get_def_use_mgr()->GetDef(counter_buffer_id);
+            if (IsDead(counter_buffer_inst)) {
+              context()->KillInst(annotation);
+              modified = true;
+            }
+          }
+        }
+        break;
       case SpvOpGroupDecorate: {
         // Go through the targets of this group decorate. Remove each dead
         // target. If all targets are dead, remove this decoration.

test/opt/aggressive_dead_code_elim_test.cpp

@@ -5807,6 +5807,59 @@ OpFunctionEnd
   SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
   SinglePassRunAndCheck<AggressiveDCEPass>(test, test, true, true);
 }
+
+TEST_F(AggressiveDCETest, DeadHlslCounterBufferGOOGLE) {
+  // We are able to remove "local2" because it is not loaded, but have to keep
+  // the stores to "local1".
+  const std::string test =
+      R"(
+; CHECK-NOT: OpDecorateId
+; CHECK: [[var:%\w+]] = OpVariable
+; CHECK-NOT: OpVariable
+; CHECK: [[ac:%\w+]] = OpAccessChain {{%\w+}} [[var]]
+; CHECK: OpStore [[ac]]
+               OpCapability Shader
+               OpExtension "SPV_GOOGLE_hlsl_functionality1"
+               OpMemoryModel Logical GLSL450
+               OpEntryPoint GLCompute %1 "main"
+               OpExecutionMode %1 LocalSize 32 1 1
+               OpSource HLSL 600
+               OpDecorate %_runtimearr_v2float ArrayStride 8
+               OpMemberDecorate %_struct_3 0 Offset 0
+               OpDecorate %_struct_3 BufferBlock
+               OpMemberDecorate %_struct_4 0 Offset 0
+               OpDecorate %_struct_4 BufferBlock
+               OpDecorateId %5 HlslCounterBufferGOOGLE %6
+               OpDecorate %5 DescriptorSet 0
+               OpDecorate %5 Binding 0
+               OpDecorate %6 DescriptorSet 0
+               OpDecorate %6 Binding 1
+      %float = OpTypeFloat 32
+    %v2float = OpTypeVector %float 2
+%_runtimearr_v2float = OpTypeRuntimeArray %v2float
+  %_struct_3 = OpTypeStruct %_runtimearr_v2float
+%_ptr_Uniform__struct_3 = OpTypePointer Uniform %_struct_3
+        %int = OpTypeInt 32 1
+  %_struct_4 = OpTypeStruct %int
+%_ptr_Uniform__struct_4 = OpTypePointer Uniform %_struct_4
+       %void = OpTypeVoid
+         %13 = OpTypeFunction %void
+         %19 = OpConstantNull %v2float
+      %int_0 = OpConstant %int 0
+%_ptr_Uniform_v2float = OpTypePointer Uniform %v2float
+          %5 = OpVariable %_ptr_Uniform__struct_3 Uniform
+          %6 = OpVariable %_ptr_Uniform__struct_4 Uniform
+          %1 = OpFunction %void None %13
+         %22 = OpLabel
+         %23 = OpAccessChain %_ptr_Uniform_v2float %5 %int_0 %int_0
+               OpStore %23 %19
+               OpReturn
+               OpFunctionEnd
+)";
+
+  SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
+  SinglePassRunAndMatch<AggressiveDCEPass>(test, true);
+}
 // TODO(greg-lunarg): Add tests to verify handling of these cases:
 //
 //    Check that logical addressing required