-
Notifications
You must be signed in to change notification settings - Fork 126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rework vulkan optimizer #1658
base: dev
Are you sure you want to change the base?
Rework vulkan optimizer #1658
Conversation
CI gfxreconstruct build queued with queue ID 231913. |
CI gfxreconstruct build # 4572 running. |
CI gfxreconstruct build # 4572 passed. |
CI gfxreconstruct build queued with queue ID 243984. |
CI gfxreconstruct build # 4695 running. |
CI gfxreconstruct build # 4695 passed. |
CI gfxreconstruct build queued with queue ID 244265. |
CI gfxreconstruct build # 4699 running. |
CI gfxreconstruct build # 4699 passed. |
CI gfxreconstruct build queued with queue ID 341189. |
CI gfxreconstruct build # 5747 running. |
CI gfxreconstruct build # 5747 failed. |
// To test this example you need any trace that performs an acceleration structure copy with | ||
// VK_COPY_ACCELERATION_STRUCTURE_MODE_COMPACT_KHR mode, for example: | ||
// https://github.com/ARM-software/tracetooltests/blob/main/src/vulkan_as_3.cpp | ||
class VulkanExampleModifier : public util::VulkanModifierBase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is the class called VulkanExampleModifier
?
- meaning it's not used as test-case, but in production code
- it actually claims it 'Adds vkSetDebugUtilsObjectNameEXT calls after relevant vkCreateAccelerationStructureKHR'
I think it should have another name to better express what it is/does
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's meant for demonstration only, to showcase how the optimization interface was intended to be used, thus the name. Has to real use other than that, can be removed completely.
|
||
// TODO: This is the same code used by CaptureManager to write function call data. It could be moved to a format | ||
// utility. | ||
void VulkanFileOptimizer::WriteFunctionCall(format::ApiCallId call_id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the same code used by CaptureManager
if that's the case we 'should' avoid that copy/paste, can look into that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I think we should have a separate PR focused on refactoring. This duplication in particular is a recurring problem as there are several abstractions already that need to write calls to a file. Could use a more generic "FileWriter"
decode::DecodeAllocator::Begin(); | ||
decoder.DecodeFunctionCall(call_id, call_info, buffer.GetData(), buffer.GetDataSize()); | ||
decode::DecodeAllocator::End(); | ||
decoder.RemoveConsumer(modifier.get()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is the consumer added and removed every time in the loop?
could that go outside maybe, along with decode::DecodeAllocator::Begin()/End()
calls?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem here was multiple modifiers might want to change the call parameters, so the values of buffer.GetData() and GetDataSize() might change after every modifier pass. So we can't really have a decoder with multiple consumers as the architecture suggests, but it can still work if the consumers are added and removed in a loop like that.
The call gets decoded, then is processed by consumer, the consumer modifies the parameter buffer by encoding modified data into it, then the next consumer receives modified buffer that needs to be decoded again.
Alternatively we could define a new consumer class that allows for modification of the calls parameters once decoded and avoid this decode-encode loop.
I'm currently investigating the failed CI-run from last week. it looks many tests related to |
CI gfxreconstruct build queued with queue ID 350987. |
CI gfxreconstruct build # 5879 running. |
CI gfxreconstruct build # 5879 failed. |
Current implementation does not allow to implement different kind of optimizations in a modular and scalable way. This commit established base infrastructure allowing imlementation of modules that can process vulkan calls and apply modifications to them. Gfxrecon-optimize uses 2 pass approach - first pass to generate optimization data and a second pass to generate modified trace. Modifications are performed in predefined order for each call separately. Each modifier is given a pointer to same modifiable parameter buffer, a method to insert new calls before current call and a flag to request removal of current call from trace. Change-Id: Ic1f396d70cd58eb0d416273bf0fec4e7c9c02f58
Change-Id: Ibf8290f90a73aaa7aac0442a16f0bad70bc3e3dc
- add required 'override' statements - tested on macOS14.5, clang-1500.3.9.4
dfc7788
to
84bf02f
Compare
CI gfxreconstruct build queued with queue ID 352651. |
CI gfxreconstruct build # 5894 running. |
CI gfxreconstruct build # 5894 passed. |
Current implementation does not allow to implement different kind of optimizations in a modular and scalable way.
This commit established base infrastructure allowing implementation of modules that can process vulkan calls and apply modifications to them.
Gfxrecon-optimize for vulkan uses 2 pass approach:
Modifications are performed in predefined order for each call separately.
Each modifier is given a pointer to same modifiable parameter buffer, a method to insert new calls before current call and a flag to request removal of current call from trace.