spirv-opt: fix adce when ns debug info present (#6217) #6420
Conversation
s-perron
left a comment
s-perron
left a comment
There was a problem hiding this comment.
I don't think we can merge this as is. The changes to debugdeclare will generate invalid code, and the validator will fail.
We make need to change the order of the optimizations and be sure to run ssa-rewrite before DCE. We generally tried to avoid that because we wanted to clear many of the simple cases first. Then ssa-rewrite runs much faster.
The other option is to have ADCE replace the debug declare with debug value instruction when the variable is never loaded.
| bool is_debug_value = | ||
| debug_opcode == NonSemanticShaderDebugInfo100DebugValue; | ||
|
|
||
| if ((!is_debug_declare && !is_debug_value) || IsLive(inst)) return true; |
There was a problem hiding this comment.
What will happen with DebugGlobalVariable? The same goes for other debug instructions that reference an id in the code.
There was a problem hiding this comment.
With respect to DebugGlobalVariable specifically, it is handled by line 738 and following: https://github.com/KhronosGroup/SPIRV-Tools/pull/6420/files#diff-657fcd309c1fcfb52296e09db7050c0047e0182eb1ed429a337d12a94f5d633bL710
I believe the other debug info is correctly gated by IsLive(inst). But I will review that.
| constexpr uint32_t kTypePointerTypeInIdx = 1; | ||
| constexpr uint32_t kTypeArrayElementTypeInIdx = 0; | ||
| constexpr uint32_t kTypeArrayLengthInIdx = 1; | ||
| constexpr uint32_t kTypeVectorComponentTypeInIdx = 0; |
There was a problem hiding this comment.
The compiler is telling me these are unused.
There was a problem hiding this comment.
Thanks, I missed cleaning those up...
|
|
||
| auto undef_inst = get_def_use_mgr()->GetDef(undef_id); | ||
| live_insts_.Set(undef_inst->unique_id()); | ||
| inst->SetInOperand(var_operand_idx, {undef_id}); |
There was a problem hiding this comment.
I don't think this is legal for DebugDeclare.
Variable must be the of an OpVariable instruction that defines the local variable.
Even if we did make it legal, I want to convince myself that this will not drop useful debug information. What are your thoughts on that?
The case I am thinking about is one where the variable is stored to, but not loaded. I believe the variable will be considered dead in that case. Replacing the OpVariable with an undef breaks the connection between the stores and the debugdeclare. That is why the original design was to wait for the ssa rewrite pass to replace the debug declare with the debug value instructions. Then the next pass of DCE could remove the OpVariable.
s-perron
left a comment
s-perron
left a comment
There was a problem hiding this comment.
You should also add a new test for a debug declare where the corresponding variable is dead.
|
I tested this patch and confirmed that it fixes all of the issues I've seen in #6217. The specific tests I ran were with RenderDoc v1.41 and generating shader debug information then doing Test: cs2 capture at main menu |
I agree that my current attempt is broken, I think it's getting cleaned up in a subsequent pass. llvm has ConvertDebugDeclareToDebugValue, PromoteMemoryToRegister.cpp#L173 as an example. It seems like the only real reaction we can have to a DebugDeclare with a dead Variable is to convert it to a DebugValue on the spot. In ADCE, is it sufficient to only create DebugValue's of Undef (because Variable has been killed to get us to this decision)? Otherwise, we might be stuck running ssa-rewrite first if we need more of the logic shown in llvm. I'll try replacing w/ Undef DebugValue's... |
3 participants
This PR fixes two problems in ADCE while processing NonSemantic debug info. The first change is to treat DebugDeclare similarly to DebugValue: if a Variable is dead, replace it with OpUndef. This mirrors the treatment of DebugValue for the Value operand. This fixes #6217.
Next, we declare all DebugLocalVariable, DebugExpression, and DebugOperation instructions to be live at initialization since there is no reason to ever kill them. They do not have variables that, if declared live, would have follow-on impact to DCE. This fixes microsoft/DirectXShaderCompiler#7874.