Skip to content
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

[Vulkan] Support uniform buffer object for passing many scalar arguments (Take 2) #7833

Merged
merged 28 commits into from
Apr 16, 2021

Conversation

masahi
Copy link
Member

@masahi masahi commented Apr 13, 2021

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

if (!(ty.propertyFlags & VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT)) continue;

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

src/runtime/vulkan/vulkan_common.h Outdated Show resolved Hide resolved
src/runtime/vulkan/vulkan_common.h Outdated Show resolved Hide resolved
src/runtime/vulkan/vulkan.cc Outdated Show resolved Hide resolved
src/runtime/vulkan/vulkan.cc Outdated Show resolved Hide resolved
src/runtime/vulkan/vulkan.cc Show resolved Hide resolved
@tqchen
Copy link
Member

tqchen commented Apr 13, 2021

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
@masahi
Copy link
Member Author

masahi commented Apr 14, 2021

@tqchen I believe I've addressed all of your comments.

Copy link
Member

@tqchen tqchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please document the rationale behind why do we do sync after re-alloc(due to deferred execution) so future maintainers can understand the rationale

Copy link
Member Author

@masahi masahi Apr 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@masahi you are right, we should sync before we call DeleteHostVisible buffer, great catch!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@masahi
Copy link
Member Author

masahi commented Apr 16, 2021

@tqchen good to go?

@tqchen tqchen merged commit aa9cb63 into apache:main Apr 16, 2021
@tqchen
Copy link
Member

tqchen commented Apr 16, 2021

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

mehrdadh pushed a commit to mehrdadh/tvm that referenced this pull request Apr 22, 2021
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request May 6, 2021
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request May 6, 2021
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request May 6, 2021
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants