Skip to content

Commit f289d04

Browse files
s-perrons-perron
authored
[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
1 parent 132103f commit f289d04

File tree

5 files changed

+163
-1
lines changed

5 files changed

+163
-1
lines changed

‎source/opt/inline_exhaustive_pass.cpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,11 @@ Pass::Status InlineExhaustivePass::InlineExhaustive(Function* func) {
5555
}
5656
}
5757
}
58+
59+
if (modified) {
60+
FixDebugDeclares(func);
61+
}
62+
5863
return (modified ? Status::SuccessWithChange : Status::SuccessWithoutChange);
5964
}
6065

‎source/opt/inline_pass.cpp

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,8 @@ namespace {
3030
constexpr int kSpvFunctionCallFunctionId = 2;
3131
constexpr int kSpvFunctionCallArgumentId = 3;
3232
constexpr int kSpvReturnValueId = 0;
33+
constexpr int kSpvDebugDeclareVarInIdx = 3;
34+
constexpr int kSpvAccessChainBaseInIdx = 0;
3335
} // namespace
3436

3537
uint32_t InlinePass::AddPointerToType(uint32_t type_id,
@@ -858,5 +860,68 @@ void InlinePass::InitializeInline() {
858860

859861
InlinePass::InlinePass() {}
860862

863+
void InlinePass::FixDebugDeclares(Function* func) {
864+
std::map<uint32_t, Instruction*> access_chains;
865+
std::vector<Instruction*> debug_declare_insts;
866+
867+
func->ForEachInst([&access_chains, &debug_declare_insts](Instruction* inst) {
868+
if (inst->opcode() == spv::Op::OpAccessChain) {
869+
access_chains[inst->result_id()] = inst;
870+
}
871+
if (inst->GetCommonDebugOpcode() == CommonDebugInfoDebugDeclare) {
872+
debug_declare_insts.push_back(inst);
873+
}
874+
});
875+
876+
for (auto& inst : debug_declare_insts) {
877+
FixDebugDeclare(inst, access_chains);
878+
}
879+
}
880+
881+
void InlinePass::FixDebugDeclare(
882+
Instruction* dbg_declare_inst,
883+
const std::map<uint32_t, Instruction*>& access_chains) {
884+
do {
885+
uint32_t var_id =
886+
dbg_declare_inst->GetSingleWordInOperand(kSpvDebugDeclareVarInIdx);
887+
888+
// The def-use chains are not kept up to date while inlining, so we need to
889+
// get the variable by traversing the functions.
890+
auto it = access_chains.find(var_id);
891+
if (it == access_chains.end()) {
892+
return;
893+
}
894+
Instruction* access_chain = it->second;
895+
896+
// If the variable id in the debug declare is an access chain, it is
897+
// invalid. it needs to be fixed up. The debug declare will be updated so
898+
// that its Var operand becomes the base of the access chain. The indexes of
899+
// the access chain are prepended before the indexes of the debug declare.
900+
901+
std::vector<Operand> operands;
902+
for (int i = 0; i < kSpvDebugDeclareVarInIdx; i++) {
903+
operands.push_back(dbg_declare_inst->GetInOperand(i));
904+
}
905+
906+
uint32_t access_chain_base =
907+
access_chain->GetSingleWordInOperand(kSpvAccessChainBaseInIdx);
908+
operands.push_back(Operand(SPV_OPERAND_TYPE_ID, {access_chain_base}));
909+
operands.push_back(
910+
dbg_declare_inst->GetInOperand(kSpvDebugDeclareVarInIdx + 1));
911+
912+
for (uint32_t i = kSpvAccessChainBaseInIdx + 1;
913+
i < access_chain->NumInOperands(); ++i) {
914+
operands.push_back(access_chain->GetInOperand(i));
915+
}
916+
917+
for (uint32_t i = kSpvDebugDeclareVarInIdx + 2;
918+
i < dbg_declare_inst->NumInOperands(); ++i) {
919+
operands.push_back(dbg_declare_inst->GetInOperand(i));
920+
}
921+
922+
dbg_declare_inst->SetInOperands(std::move(operands));
923+
} while (true);
924+
}
925+
861926
} // namespace opt
862927
} // namespace spvtools

‎source/opt/inline_pass.h

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,12 @@ class InlinePass : public Pass {
150150
// Initialize state for optimization of |module|
151151
void InitializeInline();
152152

153+
// Fixes invalid debug declare functions in `func` that were caused by
154+
// inlining. This function cannot be called while in the middle of inlining
155+
// because it needs to be able to find the instructions that define an
156+
// id.
157+
void FixDebugDeclares(Function* func);
158+
153159
// Map from function's result id to function.
154160
std::unordered_map<uint32_t, Function*> id2function_;
155161

@@ -241,6 +247,11 @@ class InlinePass : public Pass {
241247
// structural dominance.
242248
void UpdateSingleBlockLoopContinueTarget(
243249
uint32_t new_id, std::vector<std::unique_ptr<BasicBlock>>* new_blocks);
250+
251+
// Replaces the `var` operand of `dbg_declare_inst` and updates the indexes
252+
// accordingly, if it is the id of an access chain in `access_chains`.
253+
void FixDebugDeclare(Instruction* dbg_declare_inst,
254+
const std::map<uint32_t, Instruction*>& access_chains);
244255
};
245256

246257
} // namespace opt

‎source/val/validate_function.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -258,7 +258,8 @@ spv_result_t ValidateFunctionCall(ValidationState_t& _,
258258
_.HasCapability(spv::Capability::VariablePointers) &&
259259
sc == spv::StorageClass::Workgroup;
260260
const bool uc_ptr = sc == spv::StorageClass::UniformConstant;
261-
if (!ssbo_vptr && !wg_vptr && !uc_ptr) {
261+
if (!_.options()->before_hlsl_legalization && !ssbo_vptr &&
262+
!wg_vptr && !uc_ptr) {
262263
return _.diag(SPV_ERROR_INVALID_ID, inst)
263264
<< "Pointer operand " << _.getIdName(argument_id)
264265
<< " must be a memory object declaration";

‎test/opt/inline_test.cpp

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4471,6 +4471,86 @@ TEST_F(InlineTest, DecorateReturnVariableWithAliasedPointer) {
44714471
SinglePassRunAndMatch<InlineExhaustivePass>(text, true);
44724472
}
44734473

4474+
TEST_F(InlineTest, DebugDeclareWithAccessChain) {
4475+
const std::string text = R"(
4476+
; CHECK: [[EmptyStruct:%[\w]+]] = OpTypeStruct %float
4477+
; CHECK-DAG: [[Struct:%[\w]+]] = OpTypeStruct [[EmptyStruct]]
4478+
; CHECK-DAG: [[PtrType:%[\w]+]] = OpTypePointer Function [[Struct]]
4479+
; CHECK-DAG: [[EmptyPtrType:%[\w]+]] = OpTypePointer Function [[EmptyStruct]]
4480+
OpCapability Shader
4481+
OpExtension "SPV_KHR_non_semantic_info"
4482+
OpExtension "SPV_KHR_relaxed_extended_instruction"
4483+
%1 = OpExtInstImport "NonSemantic.Shader.DebugInfo.100"
4484+
OpMemoryModel Logical GLSL450
4485+
OpEntryPoint GLCompute %2 "computeMain"
4486+
OpExecutionMode %2 LocalSize 1 1 1
4487+
%3 = OpString "s.hlsl"
4488+
%4 = OpString "float"
4489+
%5 = OpString "source"
4490+
%6 = OpString "a"
4491+
%7 = OpString "SomeStruct"
4492+
%8 = OpString "SomeStruct.getA"
4493+
%9 = OpString ""
4494+
%10 = OpString "this"
4495+
%int = OpTypeInt 32 1
4496+
%int_0 = OpConstant %int 0
4497+
%uint = OpTypeInt 32 0
4498+
%uint_0 = OpConstant %uint 0
4499+
%uint_32 = OpConstant %uint 32
4500+
%float = OpTypeFloat 32
4501+
%void = OpTypeVoid
4502+
%uint_3 = OpConstant %uint 3
4503+
%uint_1 = OpConstant %uint 1
4504+
%uint_4 = OpConstant %uint 4
4505+
%uint_5 = OpConstant %uint 5
4506+
%uint_11 = OpConstant %uint 11
4507+
%uint_8 = OpConstant %uint 8
4508+
%uint_288 = OpConstant %uint 288
4509+
%25 = OpTypeFunction %void
4510+
%_struct_26 = OpTypeStruct %float
4511+
%_struct_27 = OpTypeStruct %_struct_26
4512+
%_ptr_Function__struct_27 = OpTypePointer Function %_struct_27
4513+
%_ptr_Function__struct_26 = OpTypePointer Function %_struct_26
4514+
%_ptr_Function_float = OpTypePointer Function %float
4515+
%30 = OpTypeFunction %float %_ptr_Function__struct_26 %_ptr_Function_float
4516+
%31 = OpUndef %float
4517+
%32 = OpExtInst %void %1 DebugTypeBasic %4 %uint_32 %uint_3 %uint_0
4518+
%33 = OpExtInst %void %1 DebugSource %3 %5
4519+
%34 = OpExtInst %void %1 DebugCompilationUnit %uint_1 %uint_4 %33 %uint_5
4520+
%35 = OpExtInst %void %1 DebugTypeMember %6 %32 %33 %uint_3 %uint_11 %uint_0 %uint_32 %uint_3
4521+
%36 = OpExtInstWithForwardRefsKHR %void %1 DebugTypeComposite %7 %uint_1 %33 %uint_1 %uint_8 %34 %7 %uint_32 %uint_3 %35 %37
4522+
%38 = OpExtInst %void %1 DebugTypeFunction %uint_3 %32 %36
4523+
%37 = OpExtInst %void %1 DebugFunction %8 %38 %33 %uint_4 %uint_5 %36 %9 %uint_3 %uint_4
4524+
%39 = OpExtInst %void %1 DebugLocalVariable %10 %36 %33 %uint_4 %uint_5 %37 %uint_288 %uint_1
4525+
%52 = OpExtInst %void %1 DebugLocalVariable %10 %32 %33 %uint_4 %uint_5 %37 %uint_288 %uint_1
4526+
%40 = OpExtInst %void %1 DebugExpression
4527+
; CHECK: OpFunction %void None
4528+
; CHECK: [[Var:%[\w]+]] = OpVariable [[PtrType]] Function
4529+
; CHECK: OpExtInst %void {{%[\w+]+}} DebugDeclare {{%[\w+]+}} [[Var]] {{%[\w+]+}} %int_0
4530+
; CHECK: OpExtInst %void {{%[\w+]+}} DebugDeclare {{%[\w+]+}} [[Var]] {{%[\w+]+}} %int_0 %int_0
4531+
%2 = OpFunction %void None %25
4532+
%41 = OpLabel
4533+
%42 = OpVariable %_ptr_Function__struct_27 Function
4534+
%43 = OpAccessChain %_ptr_Function__struct_26 %42 %int_0
4535+
%49 = OpAccessChain %_ptr_Function_float %43 %int_0
4536+
%44 = OpFunctionCall %float %45 %43 %49
4537+
OpReturn
4538+
OpFunctionEnd
4539+
; CHECK: OpFunction %float None
4540+
%45 = OpFunction %float None %30
4541+
%46 = OpFunctionParameter %_ptr_Function__struct_26
4542+
%50 = OpFunctionParameter %_ptr_Function_float
4543+
%47 = OpLabel
4544+
%48 = OpExtInst %void %1 DebugDeclare %39 %46 %40
4545+
%51 = OpExtInst %void %1 DebugDeclare %52 %50 %40
4546+
OpReturnValue %31
4547+
OpFunctionEnd
4548+
)";
4549+
4550+
SetTargetEnv(SPV_ENV_VULKAN_1_2);
4551+
SinglePassRunAndMatch<InlineExhaustivePass>(text, true);
4552+
}
4553+
44744554
// TODO(greg-lunarg): Add tests to verify handling of these cases:
44754555
//
44764556
// Empty modules

0 commit comments

Comments
 (0)