This is an archive of the discontinued LLVM Phabricator instance.

[libomptarget] Add allocator support for target memory
ClosedPublic

Authored by grokos on Mar 3 2021, 12:33 PM.

Details

Summary

This patch adds the infrastructure for allocator support for target memory. Three allocators are introduced for device, host and shared memory. The corresponding API functions have the llvm_ prefix temporarily, until they become part of the OpenMP standard:

llvm_omp_target_alloc_device(size_t size, int device_num);

  • Returns memory owned by device.
  • Memory is only accessible by device.

llvm_omp_target_alloc_host(size_t size, int device_num);

  • Returns non-migratable memory owned by host.
  • Memory is accessible by host and device(s).

llvm_omp_target_alloc_shared(size_t size, int device_num);

  • Returns migratable memory owned by host and device.
  • Memory is accessible by host and device.

The corresponding host OpenMP runtime patch is D96669.

Ultimately, libomptarget needs support from each plugin to allocate memory using the desired method. __tgt_rtl_data_alloc has been extended with an extra kind argument which dictates the allocator to be used. The patch provides a trivial sample implementation using the generic-elf-64 plugin. For all other plugins, calling __tgt_rtl_data_alloc with a specific allocator (other than TARGET_ALLOC_DEFAULT) results in NULL being returned until said allocator has been implemented in the plugin.

Diff Detail

Event Timeline

grokos requested review of this revision.Mar 3 2021, 12:33 PM
grokos created this revision.

LG from my end, someone else?

tianshilei1992 added inline comments.
openmp/libomptarget/plugins/generic-elf-64bit/src/rtl.cpp
275

I think we can extend __tgt_rtl_data_alloc here by adding a default argument kind such that it has the same semantic as __tgt_rtl_data_alloc. __tgt_rtl_data_alloc_explicit sounds weird...After all, it is internal interface so it will not break anything.

grokos updated this revision to Diff 329415.Mar 9 2021, 11:33 AM

Removed __tgt_rtl_data_alloc_explicit; instead we pass an extra kind argument to __tgt_rtl_data_alloc.

grokos edited the summary of this revision. (Show Details)Mar 9 2021, 11:36 AM
grokos edited the summary of this revision. (Show Details)
tianshilei1992 added inline comments.Mar 9 2021, 3:58 PM
openmp/libomptarget/src/api.cpp
40

maybe it's better to move this function to omptarget.cpp?

jdoerfert accepted this revision.Mar 10 2021, 2:04 PM

LGTM, let's move functions around separatly. @tianshilei1992 any other comment?

This revision is now accepted and ready to land.Mar 10 2021, 2:04 PM

LGTM, let's move functions around separatly. @tianshilei1992 any other comment?

Sure, but why? Others LGTM.

grokos updated this revision to Diff 330312.Mar 12 2021, 11:21 AM

Rabsed and moved targetAllocExplicit() under omptarget.cpp.

This revision was landed with ongoing or failed builds.Mar 13 2021, 3:53 AM
This revision was automatically updated to reflect the committed changes.

Apologies for missing this. Looks reasonable to me. I'll wire up the amdgcn plugin when I find some time.

openmp/libomptarget/include/omptarget.h
203

Is alloc_host expected to ignore the device_num argument?

grokos added inline comments.Mar 15 2021, 11:59 AM
openmp/libomptarget/include/omptarget.h
203

That's a good question. On the surface of it I would say yes, it makes sense to ignore the device ID when allocating on the host. However, bear in mind that these functions are not standardized yet, so the exact behavior may change to something that makes use of the device ID.

Reasonable to drop the device number for alloc_host as all devices should be able to access host memory.