-
Notifications
You must be signed in to change notification settings - Fork 685
Remove support for VkSemaphore #13070
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
Summary: ## Changes Revert some changes to the D78360038 / pytorch#12527 stack which enabled support for submitting command buffers with an associated semaphore. ## Context The original intent was to allow command buffers to be correctly ordered when submitting multiple command buffers for model inference. Previously it was thought that the Vulkan API would not be aware of the dependency between two separate command buffer submissions, so a semaphore would be needed to ensure correct execution order between them. However, I noticed the following validation layer error on Mac: ``` Validation 0 vkQueueSubmit(): pSubmits[0].pSignalSemaphores[0] (VkSemaphore 0x10f000000010f) is being signaled by VkQueue 0x1181082a8, but it was previously signaled by VkQueue 0x1181082a8 and has not since been waited on. The Vulkan spec states: Each binary semaphore element of the pSignalSemaphores member of any element of pSubmits must be unsignaled when the semaphore signal operation it defines is executed on the device (https://vulkan.lunarg.com/doc/view/1.4.313.0/mac/antora/spec/latest/chapters/cmdbuffers.html#VUID-vkQueueSubmit-pSignalSemaphores-00067) ``` The reason this happens is because we store the `VkSemaphore` together with the `VkCommandBuffer`, and use the same `VkSemaphore` in the submit info every time the command buffer is submitted. However, there's no mechanism to reset the VkSemaphore to "unsignaled" once it's been signaled. Therefore, as-is the `VkSemaphores` do not serve any purpose after the first inference. The correct approach if we were to use semaphores is to create a new one with every submission, and not have it be attached to a specific command buffer However, after some deeper research I found that `VkSemaphore` is not actually needed to ensure correct execution order between command buffers; the command pipeline barriers that we already insert should be sufficient. My primary source is this [stackoverflow question](the correct approach if we were to use semaphores is to create a new one with every submission, and not have it be attached to a specific command buffer) which references the Vulkan API spec in the answer. Therefore, remove the `VkSemaphore` machinery since it's not required. Differential Revision: D79468286
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/13070
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New Failure, 1 Unrelated FailureAs of commit 1a4b217 with merge base 1d80837 ( NEW FAILURE - The following job has failed:
FLAKY - The following job failed but was likely due to flakiness present on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
This pull request was exported from Phabricator. Differential Revision: D79468286 |
This PR needs a
|
Differential Revision: D79468286 Pull Request resolved: pytorch#13070
Summary:
Changes
Revert some changes to the D78360038 / #12527 stack which enabled support for submitting command buffers with an associated semaphore.
Context
The original intent was to allow command buffers to be correctly ordered when submitting multiple command buffers for model inference. Previously it was thought that the Vulkan API would not be aware of the dependency between two separate command buffer submissions, so a semaphore would be needed to ensure correct execution order between them.
However, I noticed the following validation layer error on Mac:
The reason this happens is because we store the
VkSemaphore
together with theVkCommandBuffer
, and use the sameVkSemaphore
in the submit info every time the command buffer is submitted. However, there's no mechanism to reset the VkSemaphore to "unsignaled" once it's been signaled.Therefore, as-is the
VkSemaphores
do not serve any purpose after the first inference.The correct approach if we were to use semaphores is to create a new one with every submission, and not have it be attached to a specific command buffer
However, after some deeper research I found that
VkSemaphore
is not actually needed to ensure correct execution order between command buffers; the command pipeline barriers that we already insert should be sufficient. My primary source is this stackoverflow question which references the Vulkan API spec in its answer.Therefore, remove the
VkSemaphore
machinery since it's not required.Differential Revision: D79468286