You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by "MasterJH5574 (via GitHub)" <gi...@apache.org> on 2024/03/08 02:29:36 UTC
[PR] [DeviceAPI] Support "GetCurrentStream" [tvm]
MasterJH5574 opened a new pull request, #16689:
URL: https://github.com/apache/tvm/pull/16689
This PR introduces a new function `GetCurrentStream`to device API, which returns the current stream of the given device.
Meanwhile, this PR updates the "CreateStream" of CUDA to creating a non-blocking stream, so that the execution on this stream can overlap with the execution of other streams.
This PR also changes the `GPUCopy` of CUDA device API to always using `cudaMemcpyAsync`.
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [DeviceAPI] Support "GetCurrentStream" [tvm]
Posted by "tqchen (via GitHub)" <gi...@apache.org>.
tqchen commented on PR #16689:
URL: https://github.com/apache/tvm/pull/16689#issuecomment-1995944642
actually it is great this topic get revealed! since the original logics would cause issues for backends like metal/vulkan due to the fact that these copies are async
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [DeviceAPI] Support "GetCurrentStream" [tvm]
Posted by "tqchen (via GitHub)" <gi...@apache.org>.
tqchen commented on PR #16689:
URL: https://github.com/apache/tvm/pull/16689#issuecomment-1995989793
https://github.com/apache/tvm/pull/16716 contains the followup
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [DeviceAPI] Support "GetCurrentStream" [tvm]
Posted by "tqchen (via GitHub)" <gi...@apache.org>.
tqchen merged PR #16689:
URL: https://github.com/apache/tvm/pull/16689
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [DeviceAPI] Support "GetCurrentStream" [tvm]
Posted by "Lunderberg (via GitHub)" <gi...@apache.org>.
Lunderberg commented on PR #16689:
URL: https://github.com/apache/tvm/pull/16689#issuecomment-1998475761
Thank you for the quick turnaround on the fix, and I like it. I agree that most GPU frameworks are asynchronous by design, and by necessity. My concern was mainly that it was a change in the existing
> The default CUDA sync behavior of copyfromto actually was mainly limited to the default stream, so it was some what limited to cuda for a default stream setting.
Ah, I had thought that was intentional. Absent any explicit opt-in, the GPU operations would be synchronized on attempting to read, but all sequences of GPU operations would be asynchronous. With the stream parameter, the transfers to the GPU would also be async.
I like the change, to have the most common API be synchronous, while all internal APIs are asynchronous.
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [DeviceAPI] Support "GetCurrentStream" [tvm]
Posted by "Lunderberg (via GitHub)" <gi...@apache.org>.
Lunderberg commented on PR #16689:
URL: https://github.com/apache/tvm/pull/16689#issuecomment-1995847276
> This PR also changes the `GPUCopy` of CUDA device API to always using `cudaMemcpyAsync`.
I think this portion of the commit needs to be reverted. Prior to this commit, the [`NDArray::CopyTo`](https://github.com/apache/tvm/blob/main/include/tvm/runtime/ndarray.h#L116) function could be called to transfer an array to/from the GPU and return the transferred array. After this commit, there is no synchronization point after the `cudaMemcpyAsync`, before returning control to the caller of `NDArray::CopyTo`.
* The caller may read from the `NDArray` result immediately after it completes. After this commit, this is a read from uninitialized memory.
* The caller may free the backing allocation of the `NDArray` argument immediate after `NDArray::CopyTo` completes. After this commit, this causes CUDA to read from a dangling pointer.
This function is used in many locations which relied on the previous semantics.
* The [`""vm.builtin.to_device"`](https://github.com/apache/tvm/blob/main/src/runtime/relax_vm/builtin.cc#L418) PackedFunc, which is the lowered form of `R.to_device`.
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [DeviceAPI] Support "GetCurrentStream" [tvm]
Posted by "tqchen (via GitHub)" <gi...@apache.org>.
tqchen commented on PR #16689:
URL: https://github.com/apache/tvm/pull/16689#issuecomment-1995926307
Indeed agree that this makes things more relax. On the other hand, from the device api's pov, we don't really guarantee the sync behavior in other DeviceAPIs, e.g. in the case of metal API or vulkan
- In most GPU APIs, both copy from/to host and across are async (e.g. in the case of metal or vulkan)
- The default CUDA sync behavior of copyfromto actually was mainly limited to the default stream.
One possible middleground we could have is to update CopyTo to always enable a StreamSync before CopyTo ends, this would help us preserve original usage of CopyTo, but still allows low level device API to enable async copy behavior that would generally provide more optimizations opportunities.
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org