-
Notifications
You must be signed in to change notification settings - Fork 3.8k
[Vulkan][Refactor] Split out vulkan.cc into separate distinct functionality #8157
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
Conversation
|
Potential reviewers: @masahi @tmoreau89 |
src/runtime/vulkan/vulkan_stream.h
Outdated
| * finish. The queued commands can also be explicitly pushed/waited | ||
| * on by calling VulkanStream::Synchronize. | ||
| * | ||
| * If the device is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work with the additional comments here, but it seems this paragraph is incomplete!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for that catch, and next time I should probably separate out the documentation from the refactoring to make those items be easier to find ahead of time. I have no memory of what I was going to finish that paragraph with, so I have deleted it for now.
Also, fixed up the linting errors.
…nality. This is in preparation for additional refactoring. Functions are organized according to group similar functionality together, to minimize the amount of file-to-file transfers needed later. The main divisions are between VulkanDeviceAPI, VulkanModuleNode/VulkanWrappedFunc, VulkanThreadEntry, and VulkanContext. Other than minimal renaming of private functions and addition of some comments, this commit should have zero changes to the functions definitions themselves, only to their arrangement within the src/runtime/vulkan directory.
8d29ca0 to
e829625
Compare
|
thanks @Lunderberg @tmoreau89 |
…nality. (apache#8157) This is in preparation for additional refactoring. Functions are organized according to group similar functionality together, to minimize the amount of file-to-file transfers needed later. The main divisions are between VulkanDeviceAPI, VulkanModuleNode/VulkanWrappedFunc, VulkanThreadEntry, and VulkanContext. Other than minimal renaming of private functions and addition of some comments, this commit should have zero changes to the functions definitions themselves, only to their arrangement within the src/runtime/vulkan directory. Co-authored-by: Eric Lunderberg <elunderberg@octoml.ai>
…nality. (apache#8157) This is in preparation for additional refactoring. Functions are organized according to group similar functionality together, to minimize the amount of file-to-file transfers needed later. The main divisions are between VulkanDeviceAPI, VulkanModuleNode/VulkanWrappedFunc, VulkanThreadEntry, and VulkanContext. Other than minimal renaming of private functions and addition of some comments, this commit should have zero changes to the functions definitions themselves, only to their arrangement within the src/runtime/vulkan directory. Co-authored-by: Eric Lunderberg <elunderberg@octoml.ai>
…nality. (apache#8157) This is in preparation for additional refactoring. Functions are organized according to group similar functionality together, to minimize the amount of file-to-file transfers needed later. The main divisions are between VulkanDeviceAPI, VulkanModuleNode/VulkanWrappedFunc, VulkanThreadEntry, and VulkanContext. Other than minimal renaming of private functions and addition of some comments, this commit should have zero changes to the functions definitions themselves, only to their arrangement within the src/runtime/vulkan directory. Co-authored-by: Eric Lunderberg <elunderberg@octoml.ai>
This is in preparation for additional refactoring. Functions are organized according to group similar functionality together, to minimize the amount of file-to-file transfers needed later. The main divisions are between VulkanDeviceAPI, VulkanModuleNode/VulkanWrappedFunc, VulkanThreadEntry, and VulkanContext.
Other than minimal renaming of private functions and the addition of some documenting comments, this commit should have zero changes to the functions definitions themselves, only to their arrangement within the
src/runtime/vulkandirectory.This rearrangement is in preparation for the following planned changes. (These changes are not implemented in the current PR, but are mentioned here as the motivation for why the functionality is grouped as it is. Each is intended to be a separate PR following this one, with the file rearrangement done such that each later change will touch the minimum number of files.)
New class, VulkanInstance
Currently,
VulkanDeviceAPIconstructs the VkInstance in the constructor, then destroys in the destructor. If an exception is thrown aftervkCreateInstance, before the end of the constructor, theVulkanDeviceAPIdestructor andvkDestroyInstaceis never called. This can cause segfaults on NV drivers.VulkanInstanceclass to be constructed inVulkanDeviceAPI's constructor. Last line ofVulkanInstanceconstructor callsvkCreateInstance, and the destructor callsvkDestroyInstance.Rename,
VulkanContext->VulkanDeviceIn keeping with the general renaming of
tvm.contexttotvm.device, applying the same here.VulkanDevice, constructor/destructor
In constructor, initialize itself based on a
VkPhysicalDevice. In destructor, callvkDestroyDevice. These are pulled out of theVulkanDeviceAPIconstructor.VulkanBuffer, constructor/destructor
CreateBufferfunction becomes a method ofVulkanDevice, returns a buffer object. EachVulkanBufferobject is owned by theVulkanDevicethat created it. TheVulkanBufferobjects can be moved, but not copied, so aVulkanBuffer*can still be returned byAllocDataSpace. TheVulkanBuffercallsvkDestroyBufferandvkFreeMemoryin its destructor.Remove maps from VulkanThreadEntry
Some device-specific parameters are stored in
VulkanDevice, while others are stored inVulkanThreadEntry. This would move all device-specific parameters into a single location. TheVulkanDevicewould hold aThreadLocalStorewith aVulkanStream,VulkanStagingBuffer, andVulkanUnformBufferin order to maintain separate per-device parameters.The active device and the
WorkspacePoolwould remain with theVulkanThreadEntry, since these are not device-specific.Unify
LaunchandLaunchDeferredAs far as I can tell, the reason to have
LaunchDeferredis to prevent multiple kernels that have the same descriptor set from submitted to the GPU at the same time.Launchis allowed to push to the command buffer without any checks. This requires the calling scope to know many internals of theVulkanStream, such as knowing that the deferred_initializer should only ever set descriptor sets.Instead, having
VulkanStream::Launchaccept an argument of which descriptors are needed (thestd::vector<VkDescriptorBufferInfo>). This pulls most of the UseImmediate-specific logic into a single place.Also, at this point, rename
UseImmediatetoUsePushDescriptors.UseImmediatesuggests that the mode is similar to immediate mode of OpenGL or Direct2D, which is not the case. Whether or not push descriptors are used, the command buffer isn't submitted to the GPU until thevkQueueSubmitcall inVulkanStream::Synchronize.Track uniform buffer bytes in the VulkanStream
Along with the descriptor sets, the uniform buffer space is also shared between all kernels simultaneously submitted to the GPU and using the same the uniform buffer. In the current implementation, if two consecutive functions each require UBO for scalar inputs, the second one will overwrite the values of the first.
Instead, we should allocate the maximum uniform buffer range allowed on a GPU, then maintain a count of how many bytes are in use. Similar to how the current
LaunchDeferredimplementation handles conflicting descriptor sets, if a new kernel cannot be submitted without overflowing the range available, make a call toSynchronize.Move
VulkanPipelinecleanup to destructorCurrently in
VulkanModuleNodedestructor, would be implemented inVulkanPipelinedestructor instead.