[OpenMP] Add more pass-through functions in DeviceTy

[ye-luo]

Summary

1. Add DeviceTy::data_alloc, DeviceTy::data_delete, DeviceTy::data_alloc, DeviceTy::synchronize pass-through functions. Avoid directly accessing Device.RTL
2. Fix the type of the first argument of synchronize_ty in rth.h, device id is int32_t which is consistent with other functions.

[jdoerfert]

Thx for the cleanup, minor comments below.

openmp/libomptarget/src/device.cpp

357

I guess the HostPtr is never used by any RTL?

362

No function comments here then.

439

same as above.

openmp/libomptarget/src/device.h

197

While we are at it, comments for both, full sentences, explain the result of delete.

229

Same as above.

[ye-luo]

openmp/libomptarget/src/device.cpp

357

It seems never used.

362

Where you asking for comments of "DeviceTy::data_delete" or "return RTL->data_delete(RTLDeviceID, TgtPtrBegin);"?
For the former, if we have it in the header, do we still need to repeat the same comment at function definition?

[ye-luo]

openmp/libomptarget/src/device.cpp

362

https://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments
Don’t duplicate the documentation comment in the header file and in the implementation file.
I'm not sure I understand what you were asking for here.

[tianshilei1992]

openmp/libomptarget/src/device.h

198

Although HstPtr is not used here, it is still part of the plugin interface. In order to be compatible with the plugin interface, I think either HstPtr is kept here, or remove the HstPtr from the plugin interface.

[ye-luo]

openmp/libomptarget/src/device.h

198

My intention is to remove it fully. Due to compatibility consideration, it has not been removed in the plugin interface. I insist in not exposing HstPtr here.

[tianshilei1992]

openmp/libomptarget/src/device.h

198

I think it doesn't make any sense. The so-called compatibility is, we may use it somewhere or sometime, especially in the future. If the plugin interface leaves it, considering maybe in the future we may use it to assist the allocation of device memory, or something else, then there is no reason to remove it from data_alloc because data_alloc is also an interface.

[ye-luo]

openmp/libomptarget/src/device.h

198

Here I'm not intended to change __tgt_rtl_data_alloc in omptargetplugin.h. Any changes there requires a different discussion.
So far the added DeviceTy::data_alloc/data_delete are intended to allocate/deallocate device memory only so HstPtr is not part of this logic.

[tianshilei1992]

openmp/libomptarget/src/device.h

198

I know. But data_alloc calls __tgt_rtl_data_alloc and it does have an argument for host pointer, right? Your intention is to take data_alloc as the only user of __tgt_rtl_data_alloc, and hope others to use data_alloc to allocate device memory in the future instead of using the function pointer directly. If __tgt_rtl_data_alloc has an argument for host pointer, why does data_alloc not have the argument?

[ye-luo]

openmp/libomptarget/src/device.h

198

Maybe my initial thoughts were not accurate.
DeviceTy::data_alloc is not a direct replacement of __tgt_rtl_data_alloc. It is intended for replacing all the calls __tgt_rtl_data_alloc currently used by libomptarget.so.

As long as __tgt_rtl_data_alloc interface stays as it is now. DeviceTy::data_alloc cannot fully cover it. Any codes which are not visible to us (downstream compilers) can still used them directly just like any other __tgt_rtl functions.
DeviceTy::data_alloc is intended to allocate device memory only and thus HstPtr is not part of the logic.

So far all the calls to __tgt_rtl_data_alloc in libomptarget.so are only intended for allocating device memory and HstBegin mapping to TgtBegin is done outside __tgt_rtl_data_alloc call.
For this reason, they can be all replaced by DeviceTy::alloc.

My intention of only using DeviceTy::data_alloc is not because of its covering all the __tgt_rtl_data_alloc feature but the real use we have seen so far only needs DeviceTy::data_alloc.

[tianshilei1992]

openmp/libomptarget/src/device.h

198

Given that case, if you really don't want to pass a nullptr to it when using data_alloc, it would be better to define data_alloc as any of following:

1. void *data_alloc(int64_t Size, void *HstPtr = nullptr);
2. Add a void *data_alloc(int64_t Size, void *HstPtr) and let void *data_alloc(int64_t Size) call data_alloc(Size, nullptr).

But I think the first one is less confusing since we only have one plugin interface and it does not have such distinction.
What's more, RTL is a member variable of DeviceTy. we almost have all member functions in DeviceTy corresponding to those in RTL. We should keep "users" away from RTL, especially when they have to deal with DeviceID and RTLDeviceID. Otherwise it is confusing. When should I use member function of DeviceTy, and when should I use RTL directly and pass the right ID to it?

[ye-luo]

openmp/libomptarget/src/device.h

198

I will adopt 1.
A lot data member of DeviceTy should be private including RTL, DeviceID, RTLDeviceID. I was actually burnt by DeviceID and motivated this patch.
Is DeviceID actually get used?

[tianshilei1992]

openmp/libomptarget/src/device.h

198

Yes, it is, but just in one case for now. :-)

[ye-luo]

openmp/libomptarget/src/device.h

198

Done with adopting 1.

[tianshilei1992]

openmp/libomptarget/src/omptarget.cpp

848

Please also add HstPtrBegin here.

[ye-luo]

openmp/libomptarget/src/omptarget.cpp

848

done

[tianshilei1992]

LGTM

openmp/libomptarget/src/api.cpp

if (device_num == omp_get_initial_device()) {
     return rc;
   }
 
   if (!device_is_ready(device_num)) {
     DP("omp_target_alloc returns NULL ptr\n");
     return NULL;
   }
 
-  DeviceTy &Device = Devices[device_num];
-  rc = Device.RTL->data_alloc(Device.RTLDeviceID, size, NULL);
+  rc = Devices[device_num].data_alloc(size);
   DP("omp_target_alloc returns device ptr " DPxMOD "\n", DPxPTR(rc));
   return rc;
 }
 
 EXTERN void omp_target_free(void *device_ptr, int device_num) {
   DP("Call to omp_target_free for device %d and address " DPxMOD "\n",
       device_num, DPxPTR(device_ptr));
 
   if (!device_ptr) {
     DP("Call to omp_target_free with NULL ptr\n");
     return;
   }
 
   if (device_num == omp_get_initial_device()) {
     free(device_ptr);
     DP("omp_target_free deallocated host ptr\n");
     return;
   }
 
   if (!device_is_ready(device_num)) {
     DP("omp_target_free returns, nothing to do\n");
     return;
   }
 
-  DeviceTy &Device = Devices[device_num];
-  Device.RTL->data_delete(Device.RTLDeviceID, (void *)device_ptr);
+  Devices[device_num].data_delete(device_ptr);
   DP("omp_target_free deallocated device ptr\n");
 }
 
 EXTERN int omp_target_is_present(void *ptr, int device_num) {
   DP("Call to omp_target_is_present for device %d and address " DPxMOD "\n",
       device_num, DPxPTR(ptr));
 
   if (!ptr) {

openmp/libomptarget/src/device.h

int deallocTgtPtr(void *TgtPtrBegin, int64_t Size, bool ForceDelete,
                     bool HasCloseModifier = false);
   int associatePtr(void *HstPtrBegin, void *TgtPtrBegin, int64_t Size);
   int disassociatePtr(void *HstPtrBegin);
 
   // calls to RTL
   int32_t initOnce();
   __tgt_target_table *load_binary(void *Img);
 
+  // device memory allocation/deallocation routines
+  /// Allocates \p Size bytes on the device and returns the address/nullptr when
+  /// succeeds/fails. \p HstPtr is an address of the host data which the
+  /// allocated target data will be associated with. If it is unknown, the
+  /// default value of \p HstPtr is nullptr. Note: this function doesn't do
+  /// pointer association. Actually, all the __tgt_rtl_data_alloc
+  /// implementations ignore \p HstPtr.
+  void *data_alloc(int64_t Size, void *HstPtr = nullptr);
+  /// Deallocates memory which \p TgtPtrBegin points at and returns
+  /// OFFLOAD_SUCCESS/OFFLOAD_FAIL when succeeds/fails.
+  int32_t data_delete(void *TgtPtrBegin);
+
   // Data transfer. When AsyncInfoPtr is nullptr, the transfer will be
   // synchronous.
   // Copy data from host to device
   int32_t data_submit(void *TgtPtrBegin, void *HstPtrBegin, int64_t Size,
                       __tgt_async_info *AsyncInfoPtr);
   // Copy data from device back to host
   int32_t data_retrieve(void *HstPtrBegin, void *TgtPtrBegin, int64_t Size,
                         __tgt_async_info *AsyncInfoPtr);
   // Copy data from current device to destination device directly
   int32_t data_exchange(void *SrcPtr, DeviceTy DstDev, void *DstPtr,
                         int64_t Size, __tgt_async_info *AsyncInfoPtr);
 
   int32_t run_region(void *TgtEntryPtr, void **TgtVarsPtr,
                      ptrdiff_t *TgtOffsets, int32_t TgtVarsSize,
                      __tgt_async_info *AsyncInfoPtr);
   int32_t run_team_region(void *TgtEntryPtr, void **TgtVarsPtr,
                           ptrdiff_t *TgtOffsets, int32_t TgtVarsSize,
                           int32_t NumTeams, int32_t ThreadLimit,
                           uint64_t LoopTripCount,
                           __tgt_async_info *AsyncInfoPtr);
 
+  /// Synchronize device/queue/event based on \p AsyncInfoPtr and return
+  /// OFFLOAD_SUCCESS/OFFLOAD_FAIL when succeeds/fails.
+  int32_t synchronize(__tgt_async_info *AsyncInfoPtr);
+
 private:
   // Call to RTL
   void init(); // To be called only via DeviceTy::initOnce()
 };
 
 /// Map between Device ID (i.e. openmp device id) and its DeviceTy.
 typedef std::vector<DeviceTy> DevicesTy;
 extern DevicesTy Devices;
 
 extern bool device_is_ready(int device_num);
 
 #endif

openmp/libomptarget/src/device.cpp

DP("Mapping required by 'present' map type modifier does not exist for "
        "HstPtrBegin=" DPxMOD ", Size=%ld\n",
        DPxPTR(HstPtrBegin), Size);
     MESSAGE("device mapping required by 'present' map type modifier does not "
             "exist for host address " DPxMOD " (%ld bytes)",
             DPxPTR(HstPtrBegin), Size);
   } else if (Size) {
     // If it is not contained and Size > 0, we should create a new entry for it.
     IsNew = true;
-    uintptr_t tp = (uintptr_t)RTL->data_alloc(RTLDeviceID, Size, HstPtrBegin);
+    uintptr_t tp = (uintptr_t)data_alloc(Size, HstPtrBegin);
     DP("Creating new map entry: HstBase=" DPxMOD ", HstBegin=" DPxMOD ", "
        "HstEnd=" DPxMOD ", TgtBegin=" DPxMOD "\n",
        DPxPTR(HstPtrBase), DPxPTR(HstPtrBegin),
        DPxPTR((uintptr_t)HstPtrBegin + Size), DPxPTR(tp));
     HostDataToTargetMap.emplace(
         HostDataToTargetTy((uintptr_t)HstPtrBase, (uintptr_t)HstPtrBegin,
                            (uintptr_t)HstPtrBegin + Size, tp));
     rc = (void *)tp;
int DeviceTy::deallocTgtPtr(void *HstPtrBegin, int64_t Size, bool ForceDelete,
   LookupResult lr = lookupMapping(HstPtrBegin, Size);
   if (lr.Flags.IsContained || lr.Flags.ExtendsBefore || lr.Flags.ExtendsAfter) {
     auto &HT = *lr.Entry;
     if (ForceDelete)
       HT.resetRefCount();
     if (HT.decRefCount() == 0) {
       DP("Deleting tgt data " DPxMOD " of size %ld\n",
           DPxPTR(HT.TgtPtrBegin), Size);
-      RTL->data_delete(RTLDeviceID, (void *)HT.TgtPtrBegin);
+      data_delete((void *)HT.TgtPtrBegin);
       DP("Removing%s mapping with HstPtrBegin=" DPxMOD ", TgtPtrBegin=" DPxMOD
           ", Size=%ld\n", (ForceDelete ? " (forced)" : ""),
           DPxPTR(HT.HstPtrBegin), DPxPTR(HT.TgtPtrBegin), Size);
       HostDataToTargetMap.erase(lr.Entry);
     }
     rc = OFFLOAD_SUCCESS;
   } else {
     DP("Section to delete (hst addr " DPxMOD ") does not exist in the allocated"

 // Load binary to device.
 __tgt_target_table *DeviceTy::load_binary(void *Img) {
   RTL->Mtx.lock();
   __tgt_target_table *rc = RTL->load_binary(RTLDeviceID, Img);
   RTL->Mtx.unlock();
   return rc;
 }
 
+void *DeviceTy::data_alloc(int64_t Size, void *HstPtr) {
+  return RTL->data_alloc(RTLDeviceID, Size, HstPtr);
+}
+
+int32_t DeviceTy::data_delete(void *TgtPtrBegin) {
+  return RTL->data_delete(RTLDeviceID, TgtPtrBegin);
+}
+
 // Submit data to device
 int32_t DeviceTy::data_submit(void *TgtPtrBegin, void *HstPtrBegin,
                               int64_t Size, __tgt_async_info *AsyncInfoPtr) {
   if (!AsyncInfoPtr || !RTL->data_submit_async || !RTL->synchronize)
     return RTL->data_submit(RTLDeviceID, TgtPtrBegin, HstPtrBegin, Size);
   else
     return RTL->data_submit_async(RTLDeviceID, TgtPtrBegin, HstPtrBegin, Size,
                                   AsyncInfoPtr);
 }
bool DeviceTy::isDataExchangable(const DeviceTy &DstDevice) {
 
   if (RTL->is_data_exchangable(RTLDeviceID, DstDevice.RTLDeviceID))
     return (RTL->data_exchange != nullptr) ||
            (RTL->data_exchange_async != nullptr);
 
   return false;
 }
 
+int32_t DeviceTy::synchronize(__tgt_async_info *AsyncInfoPtr) {
+  if (RTL->synchronize)
+    return RTL->synchronize(RTLDeviceID, AsyncInfoPtr);
+  return OFFLOAD_SUCCESS;
+}
+
 /// Check whether a device has an associated RTL and initialize it if it's not
 /// already initialized.
 bool device_is_ready(int device_num) {
   DP("Checking whether device %d is ready.\n", device_num);
   // Devices.size() can only change while registering a new
   // library, so try to acquire the lock of RTLs' mutex.
   RTLsMtx->lock();
   size_t Devices_size = Devices.size();

openmp/libomptarget/src/omptarget.cpp

for (int32_t i = 0; i < arg_num; ++i) {
     bool IsLast, IsHostPtr; // unused.
     if (arg_types[i] & OMP_TGT_MAPTYPE_LITERAL) {
       DP("Forwarding first-private value " DPxMOD " to the target construct\n",
           DPxPTR(HstPtrBase));
       TgtPtrBegin = HstPtrBase;
       TgtBaseOffset = 0;
     } else if (arg_types[i] & OMP_TGT_MAPTYPE_PRIVATE) {
       // Allocate memory for (first-)private array
-      TgtPtrBegin = Device.RTL->data_alloc(Device.RTLDeviceID,
-          arg_sizes[i], HstPtrBegin);
+      TgtPtrBegin = Device.data_alloc(arg_sizes[i], HstPtrBegin);
       if (!TgtPtrBegin) {
         DP ("Data allocation for %sprivate array " DPxMOD " failed, "
             "abort target.\n",
             (arg_types[i] & OMP_TGT_MAPTYPE_TO ? "first-" : ""),
             DPxPTR(HstPtrBegin));
         return OFFLOAD_FAIL;
       }
       fpArrays.push_back(TgtPtrBegin);
#endif
   }
   if (rc != OFFLOAD_SUCCESS) {
     DP ("Executing target region abort target.\n");
     return OFFLOAD_FAIL;
   }
 
   // Deallocate (first-)private arrays
   for (auto it : fpArrays) {
-    int rt = Device.RTL->data_delete(Device.RTLDeviceID, it);
+    int rt = Device.data_delete(it);
     if (rt != OFFLOAD_SUCCESS) {
       DP("Deallocation of (first-)private arrays failed.\n");
       return OFFLOAD_FAIL;
     }
   }
 
   // Move data from device.
   int rt = target_data_end(Device, arg_num, args_base, args, arg_sizes,
                            arg_types, arg_mappers, &AsyncInfo);
   if (rt != OFFLOAD_SUCCESS) {
     DP("Call to target_data_end failed, abort targe.\n");
     return OFFLOAD_FAIL;
   }
 
-  if (Device.RTL->synchronize)
-    return Device.RTL->synchronize(device_id, &AsyncInfo);
-
-  return OFFLOAD_SUCCESS;
+  return Device.synchronize(&AsyncInfo);
 }

openmp/libomptarget/src/rtl.h

typedef int32_t(run_region_async_ty)(int32_t, void *, void **, ptrdiff_t *,
                                        int32_t, __tgt_async_info *);
   typedef int32_t(run_team_region_ty)(int32_t, void *, void **, ptrdiff_t *,
                                       int32_t, int32_t, int32_t, uint64_t);
   typedef int32_t(run_team_region_async_ty)(int32_t, void *, void **,
                                             ptrdiff_t *, int32_t, int32_t,
                                             int32_t, uint64_t,
                                             __tgt_async_info *);
   typedef int64_t(init_requires_ty)(int64_t);
-  typedef int64_t(synchronize_ty)(int64_t, __tgt_async_info *);
+  typedef int64_t(synchronize_ty)(int32_t, __tgt_async_info *);
 
   int32_t Idx = -1;             // RTL index, index is the number of devices
                                 // of other RTLs that were registered before,
                                 // i.e. the OpenMP index of the first device
                                 // to be registered with this RTL.
   int32_t NumberOfDevices = -1; // Number of devices this RTL deals with.
 
   void *LibraryHandler = nullptr;