[GitHub] [tvm] masahi commented on a change in pull request #7833: [Vulkan] Support uniform buffer object for passing many scalar arguments (Take 2)

[GitBox <gi...@apache.org>]

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:
users@infra.apache.org