Move CUDAEvent to c10 #158219
Move CUDAEvent to c10 #158219
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/158219
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 42fac35 with merge base b1a6027 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
@eqy what do you think of this stack? |
| is_created_ = true; | ||
| } | ||
|
|
||
| void moveHelper(CUDAEvent& other) { |
There was a problem hiding this comment.
Change from (CUDAEvent&& other) since linter error
Error (CLANGTIDY) [cppcoreguidelines-rvalue-reference-param-not-moved,-warnings-as-errors]
rvalue reference parameter 'other' is never moved from inside the function
body| CUDAEvent& operator=(const CUDAEvent&) = delete; | ||
|
|
||
| CUDAEvent(CUDAEvent&& other) noexcept { | ||
| moveHelper(other); |
There was a problem hiding this comment.
change for void moveHelper(CUDAEvent& other), the original code is moveHelper(std::move(other));
| } | ||
| CUDAEvent& operator=(CUDAEvent&& other) noexcept { | ||
| if (this != &other) { | ||
| moveHelper(other); |
There was a problem hiding this comment.
change for void moveHelper(CUDAEvent& other), the original code is moveHelper(std::move(other));
| (*interp)->trace_gpu_event_deletion( | ||
| at::kCUDA, reinterpret_cast<uintptr_t>(event_)); | ||
| } | ||
| C10_CUDA_CHECK_WARN(cudaEventDestroy(event_)); |
There was a problem hiding this comment.
remove try-catch because linter error, use C10_CUDA_CHECK_WARN instead.
Error (CLANGTIDY) [bugprone-empty-catch,-warnings-as-errors]
empty catch statements hide issues; to handle exceptions appropriately,
consider re-throwing, handling, or avoiding catch altogether|
@albanD @eqy @jeffdaily May I know if you have any comments on this stack of PRs? |
|
@albanD @eqy @jeffdaily May I know if you have any comments on this stack of PRs? |
Stack from ghstack (oldest at bottom):
Motivation
When I refactored the caching allocator, I noticed that there are two separate pieces of code of
EventPool: one in aten/cuda/CachingHostAllocator.cpp and another in c10/cuda/CUDACachingAllocator. I would like to refactor these so that they share a single implementation.To achieve this, I have to move
aten/cuda/CUDAEvent.htoc10/cuda, which I understand this is a big change. However, I think it makes sense conceptually -CUDAStreamandCUDAEventare both fundamental CUDA abstractions, and sinceCUDAStreamis already inc10/cuda, placingCUDAEventthere as well seems reasonable for consistency.