[OpenMP][libomptarget] Enable requires flags for target libraries.

[gtbercea]

Summary

Target link variables are currently implemented by creating a copy of the variables on the device side and unified memory never gets exploited.

When the prgram uses the:

#pragma omp requires unified_shared_memory

directive in conjunction with a declare target link, the linked variable is no longer allocated on the device and the host version is used instead.

This behavior is overridden by performing an explicit mapping.

A Clang side patch is required.

[gtbercea]

Remove line.

[gtbercea]

• Fix comment.

[gtbercea]

• Fix comment.

[ABataev]

libomptarget/include/omptarget.h

51

Do you really need this flag? Can you use something different? For example, OMP_TGT_MAPTYPE_LITERAL or OMP_TGT_MAPTYPE_PRIVATE?

libomptarget/plugins/cuda/src/rtl.cpp

444

8->sizeof(void*)

[gtbercea]

• Fix size.

[ABataev]

libomptarget/src/rtl.cpp

181

What should we do instead for other platforms?

[AlexEichenberger]

see comments

libomptarget/include/omptarget.h

51

I don't believe it is needed, as all is handled while registering the device library. If anything, you could just use the literal to naturally move the host pointer address to the device. This is if you want to avoid going through the indirection already set in place for globals mapped with "link" clause

libomptarget/plugins/cuda/src/rtl.cpp

444

Shouldn't this code be guarded by some "if the runtime is in unified memory mode," and if we see TARGET_LINK and the runtime is not in unified memory mode, should't there be some error as things are likely to go seriously wrong?

libomptarget/src/rtl.cpp

180

Same comment here... if the runtime is in non-unified memory mode, should't it be an error to see a link?

[gtbercea]

• Update patch.

[AlexEichenberger]

See suggested changes, should be pretty straightforward to do, let me know if you need help

libomptarget/include/omptarget.h

65

I wonder if it would not be wise to add an "undefined" flag. Initially, the internal state would be in undefined state. Then when the RT receives a "register_requires", one of two actions can happen.

If the RT state is in undefined state, it looks if the new requires are compatible with the current device; if yes, it accepts it, if not it either silently "degrade" the support to something supported, or abort.

If the RT state is not in undefined state, then it looks if the new require is compatible with the prior "register_required" emitted state. If incompatibilities are found, it abort.

Alternatively, one can add an additional bit in the RT state that indicates if one or more register requires have been reported. That second approach is cleaner but need one more variable.

libomptarget/plugins/cuda/src/rtl.cpp

231

See comment above, either set to unitialized, or let the state 0, and add additional variable to state "not initialized yet".

libomptarget/src/rtl.cpp

186

compatibility test required... minimally: accept if not previously initialized; refuse if different for any target related flags

[gtbercea]

• Fix checks and default flag.

[gtbercea]

• Update patch.

[ABataev]

libomptarget/include/omptargetplugin.h

35 ↗

(On Diff #195399)

Maybe it is better to make it target-independent rather than do this only for NVPTX target?

[AlexEichenberger]

LGTM, just made one minor suggestion

[AlexEichenberger]

response to Alexey's comment

libomptarget/include/omptargetplugin.h

35 ↗

(On Diff #195399)

I thought about this too, but say for one device, there is only unified memory... so we would actually not care about the requires unified.

So it's probably more flexible to have it there, but I am not opposed to take a stricter approach "flag must be consistent regardless of implementation"

[ABataev]

Also, it would be good to try to add the tests for this new functionality

[gtbercea]

Update

[ABataev]

libomptarget/plugins/cuda/src/rtl.cpp

115

I think you need to add some (at least basic) support for this in other plugins - generic-elf-64bit.

[gtbercea]

• Add function to generic elf.

[ABataev]

libomptarget/plugins/generic-elf-64bit/src/rtl.cpp

149 ↗

(On Diff #196892)

Where are you going to check that the provided RequiresFlags does not match the previously specified one? In this case, I think, the runtime should terminate execution of the program.

[gtbercea]

• Add basic functionality.

[gtbercea]

• Fix conditions.

[ABataev]

Still, I think you can add some basic tests at least to test error messages and the conditions. See llvm/projects/openmp/runtime/test/worksharing/for/kmp_sch_simd_guided.c as an example.

libomptarget/src/rtl.cpp

191

This can be an assert, I don't think that the compiler should call it with this flag

205

Should it be != here and in other places?

208

Better to emit it in terms of OpenMP pragmas and clauses here and in all other places

[gtbercea]

• Fix messages.

[gtbercea]

• Add test.

[gtbercea]

• Clean test.

[ABataev]

Tests for error messages

[gtbercea]

ping

[grokos]

libomptarget/src/device.h

107 ↗

(On Diff #196924)

0 -> OMP_REQ_UNDEFINED?

[gtbercea]

libomptarget/src/device.h

107 ↗

(On Diff #196924)

OMP_REQ_UNDEFINED is not in scope here and not needed. This 0 never gets used. In rtl.cpp this field will be overwritten with OMP_REQ_UNDEFINED if applicable.

[gtbercea]

• Fix test.

[gtbercea]

• Add comment.

[Hahnfeld]

In this patch __tgt_rtl_init_requires does nothing. Do we really want to add the prototype now? If yes, are we sure that we won't need to pass more information in the future? Once we add it, the runtime library must maintain compatibility.

Moreover, I'm not sure if we should require plugins to implement the function if they don't need any addition checks. I'd propose to make this optional, ie look for the symbol via dlsym and disable the call if it's not found.

libomptarget/test/offloading/requires.c

27–32 ↗

(On Diff #200110)

This will only work if the test compiler already supports requires. What's wrong with calling __tgt_register_requires twice as I suggested offline?

[gtbercea]

• Fix test to make it compatible with previous clang versions.

[gtbercea]

• Make implementation of init requires optional.

[gtbercea]

In D60223#1507576, @Hahnfeld wrote:

In this patch __tgt_rtl_init_requires does nothing. Do we really want to add the prototype now? If yes, are we sure that we won't need to pass more information in the future? Once we add it, the runtime library must maintain compatibility.

Moreover, I'm not sure if we should require plugins to implement the function if they don't need any addition checks. I'd propose to make this optional, ie look for the symbol via dlsym and disable the call if it's not found.

Should be optional now.

[grokos]

I don't have any other comments or suggestions, the patch looks good.

[Hahnfeld]

In D60223#1507576, @Hahnfeld wrote:

In this patch __tgt_rtl_init_requires does nothing. Do we really want to add the prototype now? If yes, are we sure that we won't need to pass more information in the future? Once we add it, the runtime library must maintain compatibility.

Ping. At the moment this patch just stores the RequiresFlags in the plugins and never uses it.

libomptarget/test/offloading/requires.c

25–26 ↗

(On Diff #200344)

Could you please run clang-format on this file?

[gtbercea]

In D60223#1510396, @Hahnfeld wrote:

In D60223#1507576, @Hahnfeld wrote:

In this patch __tgt_rtl_init_requires does nothing. Do we really want to add the prototype now? If yes, are we sure that we won't need to pass more information in the future? Once we add it, the runtime library must maintain compatibility.

Ping. At the moment this patch just stores the RequiresFlags in the plugins and never uses it.

If you check the declare target link patch it will be used there. I can move this part of the code to that patch.

[gtbercea]

• Format test.

[gtbercea]

• Remove requires init on plugin side.

[gtbercea]

@Hahnfeld just updated the patch. Please let me know if you have any more comments.

[Hahnfeld]

LG in case we don't need __tgt_register_requires to return a code in the near future (like for saying "that's not supported")

[Hahnfeld]

In D60223#1510738, @Hahnfeld wrote:

LG in case we don't need __tgt_register_requires to return a code in the near future (like for saying "that's not supported")

Sorry, mixed this up with __tgt_rtl_init_requires which is now gone. LGTM without restrictions :)

libomptarget/src/rtl.h

125–126 ↗

(On Diff #200557)

Nit: This shouldn't be a copy of the comment for RegisterLib

libomptarget/include/omptarget.h

enum tgt_map_type {
   // return base device address of mapped data
   OMP_TGT_MAPTYPE_RETURN_PARAM    = 0x040,
   // private variable - not mapped
   OMP_TGT_MAPTYPE_PRIVATE         = 0x080,
   // copy by value - not mapped
   OMP_TGT_MAPTYPE_LITERAL         = 0x100,
   // mapping is implicit
   OMP_TGT_MAPTYPE_IMPLICIT        = 0x200,
+  /// Target link
+  OMP_TGT_MAPTYPE_USE_HOST_VAR    = 0x400,
   // member of struct, member given by [16 MSBs] - 1
   OMP_TGT_MAPTYPE_MEMBER_OF       = 0xffff000000000000
 };
 
 enum OpenMPOffloadingDeclareTargetFlags {
   /// Mark the entry as having a 'link' attribute.
   OMP_DECLARE_TARGET_LINK = 0x01,
   /// Mark the entry as being a global constructor.
   OMP_DECLARE_TARGET_CTOR = 0x02,
   /// Mark the entry as being a global destructor.
   OMP_DECLARE_TARGET_DTOR = 0x04
 };
 
 /// This struct is a record of an entry point or global. For a function
 /// entry point the size is expected to be zero
 struct __tgt_offload_entry {
   void *addr;   // Pointer to the offload entry info (function or global)
   char *name;   // Name of the function or global
   size_t size;  // Size of the entry info (0 if it is a function)
   int32_t flags; // Flags associated with the entry, e.g. 'link'.
   int32_t reserved; // Reserved, to be used by the runtime library.
 };

libomptarget/plugins/cuda/src/rtl.cpp

public:
   std::vector<int> NumTeams;
   std::vector<int> NumThreads;
 
   // OpenMP Environment properties
   int EnvNumTeams;
   int EnvTeamLimit;
 
   //static int EnvNumThreads;
   static const int HardTeamLimit = 1<<16; // 64k
   static const int HardThreadLimit = 1024;
   static const int DefaultNumTeams = 128;
   static const int DefaultNumThreads = 128;
 
   // Record entry point associated with device
   void addOffloadEntry(int32_t device_id, __tgt_offload_entry entry) {
     assert(device_id < (int32_t)FuncGblEntries.size() &&
            "Unexpected device id!");
#endif // OMPTARGET_DEBUG
     if (envStr) {
       // OMP_NUM_TEAMS has been set
       EnvNumTeams = std::stoi(envStr);
       DP("Parsed OMP_NUM_TEAMS=%d\n", EnvNumTeams);
     } else {
       EnvNumTeams = -1;
     }
   }
 
   ~RTLDeviceInfoTy() {
     // Close modules
     for (auto &module : Modules)
       if (module) {
         CUresult err = cuModuleUnload(module);
         if (err != CUDA_SUCCESS) {
           DP("Error when unloading CUDA module\n");
           CUDA_ERR_STRING(err);
if (e->size) {
         DP("Loading global '%s' - size mismatch (%zd != %zd)\n", e->name,
             cusize, e->size);
         CUDA_ERR_STRING(err);
         return NULL;
       }
 
       DP("Entry point " DPxMOD " maps to global %s (" DPxMOD ")\n",
           DPxPTR(e - HostBegin), e->name, DPxPTR(cuptr));
+
       entry.addr = (void *)cuptr;
+      if (e->flags & OMP_DECLARE_TARGET_LINK) {
+        // By default, we handle declare target link global variables as if
+        // unified memory can be sued. If at target region launch it turns
+        // out that unified memory is not available then this value will
+        // be overwritten.
+        cuMemcpyHtoD(cuptr, e->addr, 8);
+        DP("Copy linked variable host address (" DPxMOD ")"
+           "to device address (" DPxMOD ")\n",
+          DPxPTR(*((void**)e->addr)), DPxPTR(cuptr));
+      }
 
       DeviceInfo.addOffloadEntry(device_id, entry);
 
       continue;
     }
 
     CUfunction fun;
     err = cuModuleGetFunction(&fun, cumod, e->name);

libomptarget/src/device.cpp

DP("WARNING: Pointer is already mapped but section extends beyond mapped "
         "region\n");
   }
 
   return lr;
 }
 
 // Used by target_data_begin
 // Return the target pointer begin (where the data will be moved).
-// Allocate memory if this is the first occurrence if this mapping.
+// Allocate memory if this is the first occurrence of this mapping.
 // Increment the reference counter.
 // If NULL is returned, then either data allocation failed or the user tried
 // to do an illegal mapping.
 void *DeviceTy::getOrAllocTgtPtr(void *HstPtrBegin, void *HstPtrBase,
     int64_t Size, bool &IsNew, bool IsImplicit, bool UpdateRefCount) {
   void *rc = NULL;
   DataMapMtx.lock();
   LookupResult lr = lookupMapping(HstPtrBegin, Size);

libomptarget/src/interface.cpp

 static void HandleTargetOutcome(bool success) {
   switch (TargetOffloadPolicy) {
     case tgt_disabled:
       if (success) {
         FATAL_MESSAGE0(1, "expected no offloading while offloading is disabled");
       }
       break;
     case tgt_default:
-        FATAL_MESSAGE0(1, "default offloading policy must switched to " 
+        FATAL_MESSAGE0(1, "default offloading policy must switched to "
             "mandatory or disabled");
       break;
     case tgt_mandatory:
       if (!success) {
         FATAL_MESSAGE0(1, "failure of target construct while offloading is mandatory");
       }
       break;
   }

libomptarget/src/omptarget.cpp

 
 /// Internal function to do the mapping and transfer the data to the device
 int target_data_begin(DeviceTy &Device, int32_t arg_num,
     void **args_base, void **args, int64_t *arg_sizes, int64_t *arg_types) {
   // process each input.
   for (int32_t i = 0; i < arg_num; ++i) {
     // Ignore private variables and arrays - there is no mapping for them.
     if ((arg_types[i] & OMP_TGT_MAPTYPE_LITERAL) ||
-        (arg_types[i] & OMP_TGT_MAPTYPE_PRIVATE))
+        (arg_types[i] & OMP_TGT_MAPTYPE_PRIVATE) ||
+        (arg_types[i] & OMP_TGT_MAPTYPE_USE_HOST_VAR))
       continue;
 
     void *HstPtrBegin = args[i];
     void *HstPtrBase = args_base[i];
     int64_t data_size = arg_sizes[i];
 
     // Adjust for proper alignment if this is a combined entry (for structs).
     // Look at the next argument - if that is MEMBER_OF this one, then this one

libomptarget/src/rtl.cpp

for (__tgt_offload_entry *entry = img->EntriesBegin;
         Device.PendingCtorsDtors[desc].PendingCtors.push_back(entry->addr);
       } else if (entry->flags & OMP_DECLARE_TARGET_DTOR) {
         // Dtors are pushed in reverse order so they are executed from end
         // to beginning when unregistering the library!
         DP("Adding dtor " DPxMOD " to the pending list.\n",
             DPxPTR(entry->addr));
         Device.PendingCtorsDtors[desc].PendingDtors.push_front(entry->addr);
       }
-
-      if (entry->flags & OMP_DECLARE_TARGET_LINK) {
-        DP("The \"link\" attribute is not yet supported!\n");
-      }
     }
     Device.PendingGlobalsMtx.unlock();
   }
 }
 
 void RTLsTy::RegisterLib(__tgt_bin_desc *desc) {
   // Attempt to load all plugins available in the system.
   std::call_once(initFlag, &RTLsTy::LoadRTLs, this);
 
   RTLsMtx.lock();
   // Register the images with the RTLs that understand them, if any.
   for (int32_t i = 0; i < desc->NumDeviceImages; ++i) {
     // Obtain the image.
     __tgt_device_image *img = &desc->DeviceImages[i];
 
     RTLInfoTy *FoundRTL = NULL;
 
     // Scan the RTLs that have associated images until we find one that supports
     // the current image.
     for (auto &R : RTLs.AllRTLs) {
       if (!R.is_valid_binary(img)) {
         DP("Image " DPxMOD " is NOT compatible with RTL %s!\n",
             DPxPTR(img->ImageStart), R.RTLName.c_str());
         continue;
       }
 
       DP("Image " DPxMOD " is compatible with RTL %s!\n",
           DPxPTR(img->ImageStart), R.RTLName.c_str());
 
       // If this RTL is not already in use, initialize it.
       if (!R.isUsed) {
         // Initialize the device information for the RTL we are about to use.
         DeviceTy device(&R);
         size_t start = Devices.size();
         Devices.resize(start + R.NumberOfDevices, device);
         for (int32_t device_id = 0; device_id < R.NumberOfDevices;
             device_id++) {