[Openmp-commits] [PATCH] D65001: [OpenMP][libomptarget] Add support for unified memory for regular maps

[Gheorghe-Teodor Bercea via Phabricator via Openmp-commits]

gtbercea marked 3 inline comments as done.
gtbercea added inline comments.


================
Comment at: libomptarget/src/api.cpp:118-123
+  // Under unified memory the host pointer can be returned by the
+  // getTgtPtrBegin() function which means that there is no device
+  // corresponding point for ptr. This function should return false
+  // in that situation.
+  if (Device.RTLRequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY)
+    rc = (TgtPtr != ptr);
----------------
Hahnfeld wrote:
> The spec says `This routine returns non-zero if the specified pointer would be found present on device device_num by a map clause; otherwise, it returns zero.` Programs with `unified_shared_memory` could imply that all pointers are present on all devices, so I would have expected the routine to always return true. What's the reasoning for checking that the pointers differ?
I'm checking if the pointer is on the actual device. If unified memory is used then the pointers will match and the device present test will return false.
I have now refactored this check to make it more precise: if the host pointer is used then we have a flag for that.


================
Comment at: libomptarget/src/device.cpp:290-291
 int DeviceTy::deallocTgtPtr(void *HstPtrBegin, int64_t Size, bool ForceDelete) {
+  if (RTLRequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY)
+    return OFFLOAD_SUCCESS;
   // Check if the pointer is contained in any sub-nodes.
----------------
Hahnfeld wrote:
> If we can still alloc memory in `getOrAllocTgtPtr`, then this is wrong.
Fixed.


Repository:
  rOMP OpenMP

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65001/new/

https://reviews.llvm.org/D65001