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

[Jonas Hahnfeld via Phabricator via Openmp-commits]

Hahnfeld 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);
----------------
gtbercea wrote:
> Hahnfeld wrote:
> > gtbercea wrote:
> > > 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.
> > I'm still not sure about this, what do others think?
> See Alex's answer below!
This doesn't even mention `omp_target_is_present`, so this is no answer.


Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D65001