Rewrite the cuhook using cupti #332
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #332 +/- ##
========================================
Coverage 27.15% 27.15%
========================================
Files 190 190
Lines 39174 39174
Branches 14289 14359 +70
========================================
Hits 10638 10638
- Misses 27681 28144 +463
+ Partials 855 392 -463 ☔ View full report in Codecov by Sentry. |
src/realm/cuda/cuda_module.h
Outdated
| atomic<size_t> cudaipc_responses_received{0}; | ||
| int cuda_api_version = 0; | ||
|
|
||
| CudaHook *cuda_hook{nullptr}; |
There was a problem hiding this comment.
Please do not add CudaHook to the cuda module, since everything here is public and accessible to applications. We actually need to move some of these things in here to some internal structure.
There was a problem hiding this comment.
I agree with you. Before we have an internal module structure, we will either put it here or move it into the .cc and declare it as a static variable.
There was a problem hiding this comment.
I declared it as static variable inside cuda_module.cc
src/realm/cuda/cuda_module.cc
Outdated
| cupti_api_initialized = true; | ||
| } | ||
| if (config->cfg_enable_cuhook && CUPTI_HAS_FNPTR(cuptiSubscribe)) { | ||
| cuda_hook = new CudaHook(); |
There was a problem hiding this comment.
Why does this have to be a separate object?
There was a problem hiding this comment.
I think it will be cleaner to pack all cu hook related variables and functions into a class.
There was a problem hiding this comment.
I just do not like floating functions and variables, but you prefer not put them into a class, I am OK removing it.
| @@ -1,3 +1,4 @@ | |||
|
|
|||
There was a problem hiding this comment.
Can we instead just delete this file entirely? Why does this need to be separate from cuda_module or cuda_internal.cc?
There was a problem hiding this comment.
The cuda hook is a separate feature, so I think it is better to put it into a different file. The cuda_module and cuda_internal are already over 2000 lines. IMHO, we should divided them into something like cuda_memory.cc, cuda_channel.cc and etc.
2 participants
No description provided.