masahi commented on a change in pull request #7833:
URL: https://github.com/apache/tvm/pull/7833#discussion_r612729533
##########
File path: src/runtime/vulkan/vulkan.cc
##########
@@ -60,35 +91,21 @@ class VulkanThreadEntry {
pool.reset();
streams_.clear();
for (const auto& kv : staging_buffers_) {
- if (!kv.second) {
- continue;
- }
- auto& buf = *(kv.second);
- if (buf.host_addr != nullptr) {
- vkUnmapMemory(buf.device, buf.memory);
- }
- if (buf.memory != VK_NULL_HANDLE) {
- vkFreeMemory(buf.device, buf.memory, nullptr);
- }
- if (buf.buffer != VK_NULL_HANDLE) {
- vkDestroyBuffer(buf.device, buf.buffer, nullptr);
- }
+ DeleteHostVisibleBuffer(kv.second.get());
Review comment:
This change is only refactoring, it doesn't change what the current code
does with staging buffer during `VulkanThreadEntry` destructor and
`StagingBuffer` reallocation. I just did the refactoring to support the same
destruction + reallocation for UBO.
Are you saying we need to add `Synchronize` in the existing code? Also, what
about `memcpy` of pack_args to UBO as in
https://github.com/apache/tvm/blob/e8369881ef69a46eaafd143a1c9bfa0eba2eb003/src/runtime/vulkan/vulkan.cc#L1158,
do we need `Synchronize` there as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
