This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Enable map checks under unified_shared_memory mode
Needs ReviewPublic

Authored by carlo.bertolli on Aug 23 2021, 11:22 AM.

Details

Summary

Enable correctness checks in runtime for map clauses under unified_shared_memory mode.
Add OPENMP_DISABLE_MAPS environment variable input to disable this behavior.

As an example, before this patch is applied, this example would not fail:

#pragma omp requires unified_shared_memory

void foo() {

int a[10];

#pragma omp target enter data map(to:a[0:5])
#pragma omp target enter data map(to:a[0:10]) // extending already mapped memory, illegal!

}

This is illegal in OpenMP, and presumably also under unified_shared_memory mode, but the current implementation bails out on map checks under this mode.
By specifications, under unified_shared_memory mode, the user is allowed not to use maps and can expect the program to work on an system that supports the mode. However, for portability to systems that do not support unified_shared_memory, map clauses should always be correct. Not applying checks in the runtime should be a conscious request by the user.

Diff Detail

Event Timeline

carlo.bertolli requested review of this revision.Aug 23 2021, 11:22 AM

Update environment variable name to reflect comments and intended name

jdenny added a subscriber: jdenny.Aug 25 2021, 7:09 AM

As discussed, we should put this under an environment variable switch and do some measurements.

openmp/libomptarget/src/rtl.h
119

This is potentially uninitialized.

grokos added inline comments.Aug 31 2021, 10:29 AM
openmp/libomptarget/src/device.cpp
236–238

We will have a problem here after D107925, D107927 and D107928 land. These patches assume that USM mappings do not have an entry in HostDataToTargetMap. That's how they figure out whether we have an allocation in shared memory or in GPU memory, i.e. the value of IsHostPtr is determined by the presence or absence of an entry in the map. If we want to move on with this change here, we'll need to introduce an extra member in struct HostDataToTargetTy, e.g. bool IsUSMAlloc = false;, and this emplacement here will have to be extended with ..., HstPtrName, /*IsUSMAlloc*/ true). In general, I think that implementing desired support involves much more work than this patch. Basically, we need to treat USM data the exact same way as device-side mappings, i.e. remove all specialized code paths for USM allocations and only check whether we have USM data when we are about to alloc memory / perform data transfers.

370–372

Rebase this patch after D107925 lands, as these lines will be removed.

387–390

This check can go away after D107925 lands because the caller of this function has already performed this check.

openmp/libomptarget/src/rtl.cpp
82

It's better to rename this env var to something like LIBOMPTARGET_DISABLE_USM_MAPS. All env vars begin either with OMP_ if they are part of the OpenMP standard or with LIBOMPTARGET_ if they are libomptarget-specific extensions. We have no env vars starting with OPENMP_, so let's keep it consistent with the existing codebase. Also, including "USM" in the name makes it more descriptive of exactly what it does.

openmp/libomptarget/src/rtl.h
119

Need to initialize, I guess we want the default value to be false?

Also, can you add a comment about what this variable does? And maybe a more meaningful name would be something like "NoUSMMapEntries"?

Updated patch based on comments: add new map table entry field to track USM maps; use it to determine behavior in getTargetPointer and deallocTgtPtr; update var name and add explaining comments.

carlo.bertolli marked 5 inline comments as done.Sep 20 2021, 8:48 AM