[Vulkan] Support uniform buffer object for passing many scalar arguments (Take 2) #7833
Conversation
|
cc @echuraev @csullivan @ajtulloch @antinucleon would be great if you can help review this PR |
commit 1a3dbee Author: Masahiro Masuda <masahi129@gmail.com> Date: Sat Apr 10 09:19:06 2021 +0900 fix typo commit 706fb3e Author: Masahiro Masuda <masahi129@gmail.com> Date: Thu Apr 8 17:34:18 2021 +0900 doc update commit a75a5b0 Author: Masahiro Masuda <masahi129@gmail.com> Date: Thu Apr 8 17:15:48 2021 +0900 let vkmap/unmap allocate and delete host_buf commit 9a67f4a Author: Masahiro Masuda <masahi129@gmail.com> Date: Thu Apr 8 16:51:58 2021 +0900 query push constant size using runtime API commit 95ec1db Author: Masahiro Masuda <masahi129@gmail.com> Date: Mon Mar 22 17:23:32 2021 +0900 fix cpplint and revert float64 change commit bfec9d3 Author: Masahiro Masuda <masahi129@gmail.com> Date: Mon Mar 22 17:19:54 2021 +0900 introduce value kind for ubo commit 17597ae Author: Masahiro Masuda <masahi129@gmail.com> Date: Sun Mar 21 13:41:23 2021 +0900 minor fix commit a5a97f4 Author: Masahiro Masuda <masahi129@gmail.com> Date: Sun Mar 21 13:39:40 2021 +0900 refactored codegen commit 69f2d05 Author: Masahiro Masuda <masahi129@gmail.com> Date: Sun Mar 21 13:25:07 2021 +0900 revert BufferArgument change commit fb27fbb Author: Masahiro Masuda <masahi129@gmail.com> Date: Sun Mar 21 13:22:11 2021 +0900 formatting commit c1b1c88 Author: Masahiro Masuda <masahi129@gmail.com> Date: Sun Mar 21 13:19:19 2021 +0900 cleaning up commit 6c04669 Author: Masahiro Masuda <masahi@129@gmail.com> Date: Sun Mar 21 12:07:25 2021 +0900 remove log commit 436ff80 Author: Masahiro Masuda <masahi129@gmail.com> Date: Sun Mar 21 11:58:14 2021 +0900 cumsum and nms test working with ubo commit 7cfea18 Author: Masahiro Masuda <masahi129@gmail.com> Date: Sun Mar 21 08:37:11 2021 +0900 do not delete ubo when not using it commit 23b1f40 Author: Masahiro Masuda <masahi129@gmail.com> Date: Sun Mar 21 08:25:48 2021 +0900 add more log commit a8de459 Author: Masahiro Masuda <masahi129@gmail.com> Date: Sun Mar 21 08:20:27 2021 +0900 trying an approach similar to push constant commit 5f9f82d Author: Masahiro Masuda <masahi129@gmail.com> Date: Sun Mar 21 07:55:30 2021 +0900 do not use float64 commit 432ff24 Author: Masahiro Masuda <masahi129@gmail.com> Date: Sat Mar 20 13:18:43 2021 +0900 refactor commit 665d5ff Author: Masahiro Masuda <masahi129@gmail.com> Date: Sat Mar 20 12:57:39 2021 +0900 query memory type for uniform commit e1788b8 Author: Masahiro Masuda <masahi129@gmail.com> Date: Sat Mar 20 04:34:44 2021 +0900 allocate and bind ubo commit 7d2ed2b Author: Masahiro Masuda <masahi129@gmail.com> Date: Fri Mar 19 11:20:19 2021 +0900 begin runtime change for UBO commit 4f5ca8c Author: Masahiro Masuda <masahi129@gmail.com> Date: Fri Mar 19 11:05:55 2021 +0900 ubo codegen first cut
This reverts commit 3a3d068.
|
@tqchen I believe I've addressed all of your comments. |
There was a problem hiding this comment.
Thanks @masahi The overall logic looks good.
It would be great if we can add a few more code comments, in particular wrt to the way we handle arguments and staging buffer, as well as the deferred synchronization. So future developers can take note and don't need to redo our excercise as in this PR's iteration
| VulkanStagingBuffer* VulkanThreadEntry::StagingBuffer(int device_id, size_t size) { | ||
| if (!staging_buffers_[device_id]) { | ||
| staging_buffers_[device_id] = std::unique_ptr<VulkanStagingBuffer>(new VulkanStagingBuffer()); | ||
| VulkanHostVisibleBuffer* GetOrAllocate( |
There was a problem hiding this comment.
Please document the rationale behind why do we do sync after re-alloc(due to deferred execution) so future maintainers can understand the rationale
There was a problem hiding this comment.
ok I wrote something but as I wrote, I realized that I didn't really understand this issue deeply. I'm not confident what I wrote is accurate, please have a look!
My main confusion point is: if we want to make sure that old tasks get finished before we delete the buffer they use, shouldn't we sync before we call DeleteHostVisibleBuffer? Or DeleteHostVisibleBuffer itself, that calls vkFreeMemory etc, is async and it only records the delete/free commands to the queue (so sync after DeleteHostVisibleBuffer as in this PR makes sense, before/after doesn't matter)?
There was a problem hiding this comment.
@masahi you are right, we should sync before we call DeleteHostVisible buffer, great catch!
There was a problem hiding this comment.
ok updated. Though at this point I'm not sure the way we sync on staging buffer, namely after realloc + memcpy is done, is correct with respect to this point.
There was a problem hiding this comment.
In the case of staging buffer the sync happens everytime after the copy, so it should be fine. So there won't be stale staging buffer(because previous copy was synced)
|
@tqchen good to go? |
|
Thanks @masahi for the great work to bring this new feature the codebase, more importantly make the codebase more readable! This PR is now merged |
A follow up to #7717 per discussion #7717 (comment)
All issues raised have been addressed. Now one UBO is allocated per
VulkanThreadEntry, in a similar way as staging buffer.We always allocate UBO in a host visible memory. @tqchen asked if we can assume that we can always find such memory. Actually we are already making such assumption when allocating staging buffer here
tvm/src/runtime/vulkan/vulkan.cc
Line 658 in be87d56
I did find a slide https://gpuopen.com/wp-content/uploads/2016/05/Most-common-mistakes-in-Vulkan-apps.pdf where it says "At least one memory type is host-visible & host-coherent" (page 19) but so far I'm not finding the official statement in the spec.
Since the way we allocate and use UBO is similar to staging buffer, I refactored vk runtime code to cleanly support both cases.
please review @tqchen @tmoreau89 @ajtulloch