From 497958d899c3f5fb4a4bda1a942c6fd7c0132ba6 Mon Sep 17 00:00:00 2001
From: Steven Perron <stevenperron@google.com>
Date: Fri, 5 Oct 2018 08:23:09 -0400
Subject: [PATCH]  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 | 20 ++++++-
 test/opt/aggressive_dead_code_elim_test.cpp   | 53 +++++++++++++++++++
 2 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/source/opt/aggressive_dead_code_elim_pass.cpp b/source/opt/aggressive_dead_code_elim_pass.cpp
index d45db2c7a..0d98c7086 100644
--- a/source/opt/aggressive_dead_code_elim_pass.cpp
+++ b/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.
diff --git a/test/opt/aggressive_dead_code_elim_test.cpp b/test/opt/aggressive_dead_code_elim_test.cpp
index 5d73172b1..dc767dd5f 100644
--- a/test/opt/aggressive_dead_code_elim_test.cpp
+++ b/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