Page MenuHomePhabricator

[OpenMP] Move memory manager to plugin and make it a common interface
ClosedPublic

Authored by tianshilei1992 on Jan 10 2021, 5:12 PM.

Details

Summary

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.

Diff Detail

Event Timeline

tianshilei1992 created this revision.Jan 10 2021, 5:12 PM
tianshilei1992 requested review of this revision.Jan 10 2021, 5:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 10 2021, 5:12 PM

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?

tianshilei1992 added inline comments.Jan 10 2021, 5:22 PM
openmp/libomptarget/plugins/cuda/src/rtl.cpp
455

Oh, yeah. Will fix that.

Use interface class to replace the template

tianshilei1992 marked 2 inline comments as done.Jan 10 2021, 5:48 PM

Still uses LIBOMPTARGET_MEMORY_MANAGER_THRESHOLD. A plugin can extends the function getSizeThresholdFromEnv in the future to make separate control.

JonChesterfield accepted this revision.Jan 11 2021, 12:23 AM

That's much nicer, thank you. I wouldn't have thought of using inheritance here and like how it's turned out.

This revision is now accepted and ready to land.Jan 11 2021, 12:23 AM
jdoerfert added inline comments.Jan 11 2021, 6:53 AM
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.

jdoerfert accepted this revision.Jan 11 2021, 6:53 AM

I forgot to submit some nits yesterday, address what makes sense ^. LGTM

Fixed some comments

tianshilei1992 marked an inline comment as done.Jan 11 2021, 3:27 PM

Fixed header guard

This revision was landed with ongoing or failed builds.Jan 11 2021, 6:33 PM
This revision was automatically updated to reflect the committed changes.