[OPT] Update DebugDeclare if the var is not an OpVariable (#6014)

When inlining, DXC can pass the result of an OpAccessChain as a
parameter. Even if this is not valid SPIR-V, HLSL allows this for the
`this` pointer in member function. Part of legalizing is to inline these
functions.

When this happens, the `var` operand on a debug declare for the parameter
will be replaced by the result of the OpAccessChain. This in invalid.
The debug declare needs the variable with the indexes.

This commit add a pass over each function after inlining that will fix
up the debug declares that are invalid in this way.

Fixes microsoft/DirectXShaderCompiler#5191

Author: s-perron

source/opt/inline_exhaustive_pass.cpp

@@ -55,6 +55,11 @@ Pass::Status InlineExhaustivePass::InlineExhaustive(Function* func) {
       }
     }
   }
+
+  if (modified) {
+    FixDebugDeclares(func);
+  }
+
   return (modified ? Status::SuccessWithChange : Status::SuccessWithoutChange);
 }
 

source/opt/inline_pass.cpp

@@ -30,6 +30,8 @@ namespace {
 constexpr int kSpvFunctionCallFunctionId = 2;
 constexpr int kSpvFunctionCallArgumentId = 3;
 constexpr int kSpvReturnValueId = 0;
+constexpr int kSpvDebugDeclareVarInIdx = 3;
+constexpr int kSpvAccessChainBaseInIdx = 0;
 }  // namespace
 
 uint32_t InlinePass::AddPointerToType(uint32_t type_id,
@@ -858,5 +860,68 @@ void InlinePass::InitializeInline() {
 
 InlinePass::InlinePass() {}
 
+void InlinePass::FixDebugDeclares(Function* func) {
+  std::map<uint32_t, Instruction*> access_chains;
+  std::vector<Instruction*> debug_declare_insts;
+
+  func->ForEachInst([&access_chains, &debug_declare_insts](Instruction* inst) {
+    if (inst->opcode() == spv::Op::OpAccessChain) {
+      access_chains[inst->result_id()] = inst;
+    }
+    if (inst->GetCommonDebugOpcode() == CommonDebugInfoDebugDeclare) {
+      debug_declare_insts.push_back(inst);
+    }
+  });
+
+  for (auto& inst : debug_declare_insts) {
+    FixDebugDeclare(inst, access_chains);
+  }
+}
+
+void InlinePass::FixDebugDeclare(
+    Instruction* dbg_declare_inst,
+    const std::map<uint32_t, Instruction*>& access_chains) {
+  do {
+    uint32_t var_id =
+        dbg_declare_inst->GetSingleWordInOperand(kSpvDebugDeclareVarInIdx);
+
+    // The def-use chains are not kept up to date while inlining, so we need to
+    // get the variable by traversing the functions.
+    auto it = access_chains.find(var_id);
+    if (it == access_chains.end()) {
+      return;
+    }
+    Instruction* access_chain = it->second;
+
+    // If the variable id in the debug declare is an access chain, it is
+    // invalid. it needs to be fixed up. The debug declare will be updated so
+    // that its Var operand becomes the base of the access chain. The indexes of
+    // the access chain are prepended before the indexes of the debug declare.
+
+    std::vector<Operand> operands;
+    for (int i = 0; i < kSpvDebugDeclareVarInIdx; i++) {
+      operands.push_back(dbg_declare_inst->GetInOperand(i));
+    }
+
+    uint32_t access_chain_base =
+        access_chain->GetSingleWordInOperand(kSpvAccessChainBaseInIdx);
+    operands.push_back(Operand(SPV_OPERAND_TYPE_ID, {access_chain_base}));
+    operands.push_back(
+        dbg_declare_inst->GetInOperand(kSpvDebugDeclareVarInIdx + 1));
+
+    for (uint32_t i = kSpvAccessChainBaseInIdx + 1;
+         i < access_chain->NumInOperands(); ++i) {
+      operands.push_back(access_chain->GetInOperand(i));
+    }
+
+    for (uint32_t i = kSpvDebugDeclareVarInIdx + 2;
+         i < dbg_declare_inst->NumInOperands(); ++i) {
+      operands.push_back(dbg_declare_inst->GetInOperand(i));
+    }
+
+    dbg_declare_inst->SetInOperands(std::move(operands));
+  } while (true);
+}
+
 }  // namespace opt
 }  // namespace spvtools

source/opt/inline_pass.h

@@ -150,6 +150,12 @@ class InlinePass : public Pass {
   // Initialize state for optimization of |module|
   void InitializeInline();
 
+  // Fixes invalid debug declare functions in `func` that were caused by
+  // inlining. This function cannot be called while in the middle of inlining
+  // because it needs to be able to find the instructions that define an
+  // id.
+  void FixDebugDeclares(Function* func);
+
   // Map from function's result id to function.
   std::unordered_map<uint32_t, Function*> id2function_;
 
@@ -241,6 +247,11 @@ class InlinePass : public Pass {
   // structural dominance.
   void UpdateSingleBlockLoopContinueTarget(
       uint32_t new_id, std::vector<std::unique_ptr<BasicBlock>>* new_blocks);
+
+  // Replaces the `var` operand of `dbg_declare_inst` and updates the indexes
+  // accordingly, if it is the id of an access chain in `access_chains`.
+  void FixDebugDeclare(Instruction* dbg_declare_inst,
+                       const std::map<uint32_t, Instruction*>& access_chains);
 };
 
 }  // namespace opt

source/val/validate_function.cpp

@@ -258,7 +258,8 @@ spv_result_t ValidateFunctionCall(ValidationState_t& _,
               _.HasCapability(spv::Capability::VariablePointers) &&
               sc == spv::StorageClass::Workgroup;
           const bool uc_ptr = sc == spv::StorageClass::UniformConstant;
-          if (!ssbo_vptr && !wg_vptr && !uc_ptr) {
+          if (!_.options()->before_hlsl_legalization && !ssbo_vptr &&
+              !wg_vptr && !uc_ptr) {
             return _.diag(SPV_ERROR_INVALID_ID, inst)
                    << "Pointer operand " << _.getIdName(argument_id)
                    << " must be a memory object declaration";

test/opt/inline_test.cpp

@@ -4471,6 +4471,86 @@ TEST_F(InlineTest, DecorateReturnVariableWithAliasedPointer) {
   SinglePassRunAndMatch<InlineExhaustivePass>(text, true);
 }
 
+TEST_F(InlineTest, DebugDeclareWithAccessChain) {
+  const std::string text = R"(
+; CHECK: [[EmptyStruct:%[\w]+]] = OpTypeStruct %float
+; CHECK-DAG: [[Struct:%[\w]+]] = OpTypeStruct [[EmptyStruct]]
+; CHECK-DAG: [[PtrType:%[\w]+]] = OpTypePointer Function [[Struct]]
+; CHECK-DAG: [[EmptyPtrType:%[\w]+]] = OpTypePointer Function [[EmptyStruct]]
+               OpCapability Shader
+               OpExtension "SPV_KHR_non_semantic_info"
+               OpExtension "SPV_KHR_relaxed_extended_instruction"
+          %1 = OpExtInstImport "NonSemantic.Shader.DebugInfo.100"
+               OpMemoryModel Logical GLSL450
+               OpEntryPoint GLCompute %2 "computeMain"
+               OpExecutionMode %2 LocalSize 1 1 1
+          %3 = OpString "s.hlsl"
+          %4 = OpString "float"
+          %5 = OpString "source"
+          %6 = OpString "a"
+          %7 = OpString "SomeStruct"
+          %8 = OpString "SomeStruct.getA"
+          %9 = OpString ""
+         %10 = OpString "this"
+        %int = OpTypeInt 32 1
+      %int_0 = OpConstant %int 0
+       %uint = OpTypeInt 32 0
+     %uint_0 = OpConstant %uint 0
+    %uint_32 = OpConstant %uint 32
+      %float = OpTypeFloat 32
+       %void = OpTypeVoid
+     %uint_3 = OpConstant %uint 3
+     %uint_1 = OpConstant %uint 1
+     %uint_4 = OpConstant %uint 4
+     %uint_5 = OpConstant %uint 5
+    %uint_11 = OpConstant %uint 11
+     %uint_8 = OpConstant %uint 8
+   %uint_288 = OpConstant %uint 288
+         %25 = OpTypeFunction %void
+ %_struct_26 = OpTypeStruct %float
+ %_struct_27 = OpTypeStruct %_struct_26
+%_ptr_Function__struct_27 = OpTypePointer Function %_struct_27
+%_ptr_Function__struct_26 = OpTypePointer Function %_struct_26
+%_ptr_Function_float = OpTypePointer Function %float
+         %30 = OpTypeFunction %float %_ptr_Function__struct_26 %_ptr_Function_float
+         %31 = OpUndef %float
+         %32 = OpExtInst %void %1 DebugTypeBasic %4 %uint_32 %uint_3 %uint_0
+         %33 = OpExtInst %void %1 DebugSource %3 %5
+         %34 = OpExtInst %void %1 DebugCompilationUnit %uint_1 %uint_4 %33 %uint_5
+         %35 = OpExtInst %void %1 DebugTypeMember %6 %32 %33 %uint_3 %uint_11 %uint_0 %uint_32 %uint_3
+         %36 = OpExtInstWithForwardRefsKHR %void %1 DebugTypeComposite %7 %uint_1 %33 %uint_1 %uint_8 %34 %7 %uint_32 %uint_3 %35 %37
+         %38 = OpExtInst %void %1 DebugTypeFunction %uint_3 %32 %36
+         %37 = OpExtInst %void %1 DebugFunction %8 %38 %33 %uint_4 %uint_5 %36 %9 %uint_3 %uint_4
+         %39 = OpExtInst %void %1 DebugLocalVariable %10 %36 %33 %uint_4 %uint_5 %37 %uint_288 %uint_1
+         %52 = OpExtInst %void %1 DebugLocalVariable %10 %32 %33 %uint_4 %uint_5 %37 %uint_288 %uint_1
+         %40 = OpExtInst %void %1 DebugExpression
+; CHECK: OpFunction %void None
+; CHECK: [[Var:%[\w]+]] = OpVariable [[PtrType]] Function
+; CHECK: OpExtInst %void {{%[\w+]+}} DebugDeclare {{%[\w+]+}} [[Var]] {{%[\w+]+}} %int_0
+; CHECK: OpExtInst %void {{%[\w+]+}} DebugDeclare {{%[\w+]+}} [[Var]] {{%[\w+]+}} %int_0 %int_0
+          %2 = OpFunction %void None %25
+         %41 = OpLabel
+         %42 = OpVariable %_ptr_Function__struct_27 Function
+         %43 = OpAccessChain %_ptr_Function__struct_26 %42 %int_0
+         %49 = OpAccessChain %_ptr_Function_float %43 %int_0
+         %44 = OpFunctionCall %float %45 %43 %49
+               OpReturn
+               OpFunctionEnd
+; CHECK: OpFunction %float None
+         %45 = OpFunction %float None %30
+         %46 = OpFunctionParameter %_ptr_Function__struct_26
+         %50 = OpFunctionParameter %_ptr_Function_float
+         %47 = OpLabel
+         %48 = OpExtInst %void %1 DebugDeclare %39 %46 %40
+         %51 = OpExtInst %void %1 DebugDeclare %52 %50 %40
+               OpReturnValue %31
+               OpFunctionEnd
+)";
+
+  SetTargetEnv(SPV_ENV_VULKAN_1_2);
+  SinglePassRunAndMatch<InlineExhaustivePass>(text, true);
+}
+
 // TODO(greg-lunarg): Add tests to verify handling of these cases:
 //
 //    Empty modules