tlopex commented on PR #18672: URL: https://github.com/apache/tvm/pull/18672#issuecomment-4629238595
Thanks for working on this. The overall direction makes sense, but I think this needs another revision before merge. I found a few runtime issues that look more serious than style nits: 1. `src/runtime/vulkan/vulkan_device_api.cc:434`: `FreeDataSpace` now queues a lambda that calls `delete` during command-buffer recording. This can destroy `VkBuffer` / `VkImage` objects before previously recorded GPU work has been submitted and waited on. The previous implementation synchronized before deletion. Please keep the Vulkan resources alive until the relevant command buffer has completed. 2. `src/runtime/vulkan/vulkan_resource.h:55`: `VulkanMemory::~VulkanMemory` does not call `vkFreeMemory`, while `VulkanBuffer::~VulkanBuffer` no longer frees memory. This appears to leak every `VkDeviceMemory` allocation. The memory owner needs to retain the `VkDevice` and free the allocation, or the old ownership model should be restored. 3. `src/runtime/vulkan/vulkan_device_api.cc:487`: the new image copy paths use `VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL` / `VK_IMAGE_LAYOUT_TRANSFER_SRC_OPTIMAL`, but several paths do not transition the image into those layouts before copying. The image-to-CPU path also assumes `oldLayout = VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL`, while descriptors are bound with `VK_IMAGE_LAYOUT_GENERAL`. This is undefined behavior and should be fixed with explicit layout tracking/transitions. 4. `src/runtime/vulkan/vulkan_device_api.cc:455`: `FreeWorkspace` constructs a `Buffer` with only `data` set, but `VulkanPooledAllocator::Free` keys the pool by `buffer.size`. Workspace frees therefore go into the size-0 pool and cannot be reused correctly. This looks like a regression from the previous `WorkspacePool` path. 5. `src/runtime/file_utils.cc:80`: adding `storage_scopes` into the binary `FunctionInfo` serialization format makes `Load` fail on modules serialized by older TVM builds. JSON load is optional, but binary load is not. Please preserve backward compatibility or add a versioned/fallback read. 6. `python/tvm/testing/utils.py:999`: `requires_opencl_vulkan` still has `parent_features=["opencl", "gpu"]`, so Vulkan-only configurations inherit the OpenCL requirement and will skip/fail the tests this PR is trying to enable. This should be modeled as OpenCL-or-Vulkan, not Vulkan plus OpenCL. One smaller unrelated cleanup: `include/tvm/runtime/logging.h` changes MSVC pragmas from `#pragma warning(push/pop)` to `#pragma disagnostic push/pop`, which looks accidental and should be reverted. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
