spirv-opt: Add specific handling of vulkan debug info differences #4398
Conversation
There was a problem hiding this comment.
This change contains four functional components
- disallow forward reference for non-semantic debug info
- handle
DebugFunctionDefinitionin the function inline pass - handle non-semantic debug info in IR loader
- handle non-semantic debug info in binary dump
Could you break them up into four PR?
source/opt/inline_pass.cpp
Outdated
| @@ -403,6 +403,14 @@ bool InlinePass::InlineEntryBlock( | |||
| callee2caller, inlined_at_ctx, new_blk_ptr, callee_first_block); | |||
|
|
|||
| while (callee_inst_itr != callee_first_block->end()) { | |||
| // don't inline function definition links, the calling function is not a | |||
| // definition | |||
There was a problem hiding this comment.
// Don't inline function definition links, the calling function is not a
// definition.
source/opt/ir_loader.cpp
Outdated
| default: { | ||
| Errorf(consumer_, src, loc, | ||
| "Debug info extension instruction other than DebugScope, " | ||
| "DebugNoScope, DebugDeclare, and DebugValue found inside " |
There was a problem hiding this comment.
It does not contain DebugFunctionDefinition.
Debug info extension instruction other than DebugScope, DebugNoScope, DebugDeclare, DebugValue, and DebugFunctionDefinition found inside ...
source/opt/module.cpp
Outdated
| scope.ToBinary(dbg_inst->type_id(), context()->TakeNextId(), | ||
| dbg_inst->GetSingleWordOperand(2), binary); | ||
| // don't emit scope changes when outside of blocks in | ||
| // Vulkan.NonSemantic.DebugInfo.100 |
There was a problem hiding this comment.
I cannot understand this comment. Could you please rephrase it?
In addition, please start the sentence with a large character instead of a small one e.g., Don't .. instead of don't
and end the sentence with a dot ..
There was a problem hiding this comment.
Nonsemantic instructions cannot be emitted before any phi instructions in a block. This logic will make sure scope instructions are emitted after any phi instructions in a block for Vulkan.NonSemantic.DebugInfo.100. I have updated the comment to make this clearer.
|
I think I have addressed all current concerns regarding comments and error messages.
Yes I could and I will if you want me to, although I would request that in the name of expediency we move ahead with these as is and not go through the extra work of creating separate PRs. I would suggest that these changes are all related since they all have the goal of enabling processing of Vulkan debug info. Since Vulkan debug info processing is dependent on all of these, they really are not very useful independently and thus very little would be gained by breaking them up. If you still feel there is value in splitting these, let me know and I will do that. |
|
Ok. I will approve this PR. I rerun the CI-macos-clang-debug test. I will submit it when the test passes. |
|
Thanks Jaebaek! Looks like it passed. |
No description provided.