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

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

gtbercea marked 2 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:
> gtbercea wrote:
> > 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.
> Okay, I think I get what you're saying: You want to check that this particular pointer has been `map`ped to that specific `device_num`, right?
> 
> But with unified shared memory, this shouldn't matter, right? Because all pointers can be accessed from all devices, no?
Employing the API functions allows the user to bypass the unified memory behaviour and these functions allow the user to manage device pointers explicitly.


================
Comment at: libomptarget/src/device.cpp:207-215
+      uintptr_t tp;
+      // If unified memory is active AND a use_device_ptr clause was used,
+      // we will force the allocation of a device variable. It means that the
+      // user really wants to have this variable replicate on the device.
+      if (RTLRequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY &&
+          IsInUseDevicePtrClause)
+        tp = (uintptr_t)RTL->data_alloc(RTLDeviceID, Size, NULL);
----------------
Hahnfeld wrote:
> Hahnfeld wrote:
> > I don't understand this comment and behavior: `use_device_ptr` is a clause for `target data` and IIRC it means that the pointer is replaced by the device pointer in the data region. Why does this imply we have to allocate memory? I'd expect to just keep the host pointer because all memory of all devices is accessible from the host.
> > 
> > Can you maybe add a test for the expected behavior?
> I still don't understand this.
When use_device_ptr is employed the pointer is a true device pointer.


Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D65001