Skip to content

Commit 75f2e87

Browse files
s-perrons-perron
authored
[opt] Move debug instruction when neccessary in copy prop arrays. (KhronosGroup#6142)
Debug instructions for a function scope variable may appear before the store. We use the store as the insertion point for a new access change to the source variable assuming all uses of the variable are dominated by the store. This causes problems for the debug instruction. It will then receive an operand that has not been defined. The fix this we move the debug instruction to after the new access chain if necessary. We opt for this option because: 1. We did not want to avoid doing the optimization because of debug information. 2. There is no other reasonable place to insert the access chain. If we move it to the start of the entry block, some operands may not be defined, and may not be available. If we put it anywhere else, we potentially get the same problem as we currently have with the store. 3. Moving the debug value or debug declare instructions are not a problem. Before the store the varible was undefined, so the debug information would not enable any useful information in the debugger anyway. Fixes microsoft/DirectXShaderCompiler#7025
1 parent 6039eef commit 75f2e87

2 files changed

Lines changed: 99 additions & 4 deletions

File tree

‎source/opt/copy_prop_arrays.cpp‎

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -256,11 +256,14 @@ bool CopyPropagateArrays::HasValidReferencesOnly(Instruction* ptr_inst,
256256
} else if (use->IsDecoration() || use->opcode() == spv::Op::OpName) {
257257
return true;
258258
} else if (use->opcode() == spv::Op::OpStore) {
259-
// If we are storing to part of the object it is not an candidate.
259+
// If we are storing to part of the object it is not a candidate.
260260
return ptr_inst->opcode() == spv::Op::OpVariable &&
261261
store_inst->GetSingleWordInOperand(kStorePointerInOperand) ==
262262
ptr_inst->result_id();
263263
} else if (IsDebugDeclareOrValue(use)) {
264+
// The store does not have to dominate debug instructions. We do not
265+
// want debugging info to stop the transformation. It will be fixed
266+
// up later.
264267
return true;
265268
}
266269
// Some other instruction. Be conservative.
@@ -656,6 +659,20 @@ void CopyPropagateArrays::UpdateUses(Instruction* original_ptr_inst,
656659
uint32_t index = pair.second;
657660

658661
if (use->IsCommonDebugInstr()) {
662+
// It is possible that the debug instructions are not dominated by
663+
// `new_ptr_inst`. If not, move the debug instruction to just after
664+
// `new_ptr_inst`.
665+
BasicBlock* store_block = context()->get_instr_block(new_ptr_inst);
666+
if (store_block) {
667+
Function* function = store_block->GetParent();
668+
DominatorAnalysis* dominator_analysis =
669+
context()->GetDominatorAnalysis(function);
670+
if (!dominator_analysis->Dominates(new_ptr_inst, use)) {
671+
assert(dominator_analysis->Dominates(use, new_ptr_inst));
672+
use->InsertAfter(new_ptr_inst);
673+
}
674+
}
675+
659676
switch (use->GetCommonDebugOpcode()) {
660677
case CommonDebugInfoDebugDeclare: {
661678
if (new_ptr_inst->opcode() == spv::Op::OpVariable ||
@@ -897,9 +914,7 @@ CopyPropagateArrays::MemoryObject::MemoryObject(Instruction* var_inst,
897914
iterator begin, iterator end)
898915
: variable_inst_(var_inst) {
899916
std::transform(begin, end, std::back_inserter(access_chain_),
900-
[](uint32_t id) {
901-
return AccessChainEntry{true, {id}};
902-
});
917+
[](uint32_t id) { return AccessChainEntry{true, {id}}; });
903918
}
904919

905920
std::vector<uint32_t> CopyPropagateArrays::MemoryObject::GetAccessIds() const {

‎test/opt/copy_prop_array_test.cpp‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2313,6 +2313,86 @@ TEST_F(CopyPropArrayPassTest, DebugInstNotStore) {
23132313
SinglePassRunAndMatch<CopyPropagateArrays>(before, false);
23142314
}
23152315

2316+
TEST_F(CopyPropArrayPassTest, DebugInstNotDominatingStore) {
2317+
// Move the debug value to after the new access chain instruction.
2318+
const std::string before = R"(
2319+
OpCapability Shader
2320+
OpCapability MeshShadingEXT
2321+
OpExtension "SPV_EXT_mesh_shader"
2322+
%1 = OpExtInstImport "NonSemantic.Shader.DebugInfo.100"
2323+
OpMemoryModel Logical GLSL450
2324+
OpEntryPoint MeshEXT %2 "main" %3
2325+
OpExecutionMode %2 LocalSize 1 1 1
2326+
OpExecutionMode %2 OutputTrianglesEXT
2327+
OpExecutionMode %2 OutputVertices 64
2328+
OpExecutionMode %2 OutputPrimitivesEXT 124
2329+
%4 = OpString ""
2330+
%int = OpTypeInt 32 1
2331+
%int_0 = OpConstant %int 0
2332+
%uint = OpTypeInt 32 0
2333+
%uint_32 = OpConstant %uint 32
2334+
%_arr_uint_uint_32 = OpTypeArray %uint %uint_32
2335+
%_struct_10 = OpTypeStruct %_arr_uint_uint_32
2336+
%_ptr_TaskPayloadWorkgroupEXT__struct_10 = OpTypePointer TaskPayloadWorkgroupEXT %_struct_10
2337+
%uint_124 = OpConstant %uint 124
2338+
%uint_64 = OpConstant %uint 64
2339+
%void = OpTypeVoid
2340+
%uint_6 = OpConstant %uint 6
2341+
%uint_0 = OpConstant %uint 0
2342+
%uint_1 = OpConstant %uint 1
2343+
%uint_4 = OpConstant %uint 4
2344+
%uint_5 = OpConstant %uint 5
2345+
%uint_1024 = OpConstant %uint 1024
2346+
%uint_7 = OpConstant %uint 7
2347+
%uint_8 = OpConstant %uint 8
2348+
%uint_3 = OpConstant %uint 3
2349+
%uint_96 = OpConstant %uint 96
2350+
%uint_11 = OpConstant %uint 11
2351+
%uint_12 = OpConstant %uint 12
2352+
%uint_10 = OpConstant %uint 10
2353+
%uint_16 = OpConstant %uint 16
2354+
%uint_21 = OpConstant %uint 21
2355+
%uint_17 = OpConstant %uint 17
2356+
%uint_26 = OpConstant %uint 26
2357+
%32 = OpTypeFunction %void
2358+
%_ptr_Function__arr_uint_uint_32 = OpTypePointer Function %_arr_uint_uint_32
2359+
%3 = OpVariable %_ptr_TaskPayloadWorkgroupEXT__struct_10 TaskPayloadWorkgroupEXT
2360+
%34 = OpExtInst %void %1 DebugOperation %uint_0
2361+
%35 = OpExtInst %void %1 DebugTypeBasic %4 %uint_32 %uint_6 %uint_0
2362+
%36 = OpExtInst %void %1 DebugSource %4 %4
2363+
%37 = OpExtInst %void %1 DebugCompilationUnit %uint_1 %uint_4 %36 %uint_5
2364+
%38 = OpExtInst %void %1 DebugTypeArray %35 %uint_32
2365+
%39 = OpExtInst %void %1 DebugTypeMember %4 %38 %36 %uint_7 %uint_8 %uint_0 %uint_1024 %uint_3
2366+
%40 = OpExtInst %void %1 DebugTypeComposite %4 %uint_1 %36 %uint_6 %uint_8 %37 %4 %uint_1024 %uint_3 %39
2367+
%41 = OpExtInst %void %1 DebugTypeVector %35 %uint_3
2368+
%42 = OpExtInst %void %1 DebugTypeArray %41 %uint_124
2369+
%43 = OpExtInst %void %1 DebugTypeBasic %4 %uint_32 %uint_3 %uint_0
2370+
%44 = OpExtInst %void %1 DebugTypeVector %43 %uint_3
2371+
%45 = OpExtInst %void %1 DebugTypeMember %4 %44 %36 %uint_11 %uint_12 %uint_0 %uint_96 %uint_3
2372+
%46 = OpExtInst %void %1 DebugTypeComposite %4 %uint_1 %36 %uint_10 %uint_8 %37 %4 %uint_96 %uint_3 %45
2373+
%47 = OpExtInst %void %1 DebugTypeArray %46 %uint_64
2374+
%48 = OpExtInst %void %1 DebugTypeFunction %uint_3 %void %40 %35 %42 %47
2375+
%49 = OpExtInst %void %1 DebugFunction %4 %48 %36 %uint_16 %uint_1 %37 %4 %uint_3 %uint_21
2376+
%50 = OpExtInst %void %1 DebugLocalVariable %4 %40 %36 %uint_17 %uint_26 %49 %uint_4 %uint_1
2377+
%51 = OpExtInst %void %1 DebugExpression %34
2378+
; CHECK: OpFunction
2379+
%2 = OpFunction %void None %32
2380+
%52 = OpLabel
2381+
%53 = OpVariable %_ptr_Function__arr_uint_uint_32 Function
2382+
; CHECK: [[new_ptr:%\w+]] = OpAccessChain %_ptr_TaskPayloadWorkgroupEXT__arr_uint_uint_32
2383+
; CHECK: OpExtInst %void %1 DebugValue {{%\w+}} [[new_ptr]]
2384+
%54 = OpExtInst %void %1 DebugValue %50 %53 %51 %int_0
2385+
%55 = OpLoad %_struct_10 %3
2386+
%56 = OpCompositeExtract %_arr_uint_uint_32 %55 0
2387+
OpStore %53 %56
2388+
OpReturn
2389+
OpFunctionEnd
2390+
)";
2391+
2392+
SetTargetEnv(SPV_ENV_VULKAN_1_1);
2393+
SinglePassRunAndMatch<CopyPropagateArrays>(before, false);
2394+
}
2395+
23162396
} // namespace
23172397
} // namespace opt
23182398
} // namespace spvtools

0 commit comments

Comments
 (0)