The lifetime of libomptarget and its opened plugins are not aligned
and it's hard for libomptarget to determine when the plugins are destroyed.
As a result, some issues (see D94256 for details) occur on some platforms.
Actually, if we take target memory as target resources, same as other resources,
such as CUDA streams, in each plugin, then the memory manager should also be in
the plugin. Also considering some platforms may want to opt out the feature, it
makes sense to move the memory manager to plugin, make it a common interface, and
let plguin developers determine whether they need it. This is what this patch does.
CUDA plugin is taken as example to show how to integrate it. In this way, we can
also get a bonus that different thresholds can be set for different platforms.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
This looks better.
We could make an interface, let memory manager and DeviceAllocatorTy implement it, and simply fill the "allocator list" with the appropriate ones. Unsure if that is preferable over the if(UseMemoryManager) conditional but it might be nicer to read.
Update the docs wrt. environment vars, and also look for the generic LIBOMPTARGET_MEMORY_MANAGER_THRESHOLD. Or only look for that one.
openmp/libomptarget/plugins/cuda/src/rtl.cpp | ||
---|---|---|
453 | No need for the if. | |
455 | Don't we always need to create the device allocators? |
openmp/libomptarget/plugins/cuda/src/rtl.cpp | ||
---|---|---|
455 | Oh, yeah. Will fix that. |
Still uses LIBOMPTARGET_MEMORY_MANAGER_THRESHOLD. A plugin can extends the function getSizeThresholdFromEnv in the future to make separate control.
That's much nicer, thank you. I wouldn't have thought of using inheritance here and like how it's turned out.
openmp/libomptarget/plugins/common/MemoryManager.h | ||
---|---|---|
96 ↗ | (On Diff #315697) | As clang tidy tells you, this can be problematic. Easy way out of this is to move the helpers as static functions into the memory manager, especially given that they look at MEMORY_MANAGER env vars. |
114 ↗ | (On Diff #315697) | Given tat you have the DeviceAllocatorTy already, why not inherit here and in the cuda class? |
279 ↗ | (On Diff #315697) | Nit: No need for the ifdef, I think. Given all the other DP macros around anyway. also other places. |
openmp/libomptarget/plugins/cuda/src/rtl.cpp | ||
28 | common should be in the include path via cmake. |
clang-tidy: warning: header guard does not follow preferred style [llvm-header-guard]
not useful