[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/omptarget.cpp:247-249
+    // TODO: Check if this is correct
+    bool IsInUseDevicePtrClause = arg_types[i] & OMP_TGT_MAPTYPE_TARGET_PARAM &&
+        arg_types[i] & OMP_TGT_MAPTYPE_RETURN_PARAM;
----------------
gtbercea wrote:
> Hahnfeld wrote:
> > grokos wrote:
> > > This is correct, with one little exception. Although the OpenMP standard does not mandate it, upstream clang supports `use_device_ptr` on pointers which are struct members. Because they are struct members, they are not marked with `TARGET_PARAM` (only the combined entry is considered a target parameter, not the individual members). On the other hand, they are marked with `PTR_AND_OBJ` and have some value in the `MEMBER_OF` bits.
> > > 
> > > Once again, it's a non-standard extension so we are free to decide whether to support it or not in the unified shared memory scenario.
> > Can we please first answer my question why we need to care about the existence of `use_device_ptr`? Why does it make a difference for unified shared memory?
> Because use_device_ptr implies the use of a true device pointer and again that needs to be respected even unified memory is used.
Where is that in the spec?


================
Comment at: libomptarget/test/api/api.c:98
+
+  // CHECK: Inside target data: A is not present
+  // CHECK: Inside target data: B is present
----------------
gtbercea wrote:
> Hahnfeld wrote:
> > gtbercea wrote:
> > > Hahnfeld wrote:
> > > > gtbercea wrote:
> > > > > Hahnfeld wrote:
> > > > > > because this sounds really odd: `A` can be accessed on the device, but is not "present"?
> > > > > This is just a presence test using the OpenMP API. If A is not associated to a device instance then it won't be considered present. Again, if the user wants to handle all of this manually the option is there and unified memory can't make this not an option for the user.
> > > > > If A is not associated to a device instance then it won't be considered present.
> > > > 
> > > > Can you link me to a paragraph in the spec that mandates this implication? I only know of the following from the `Effect` of `omp_target_is_present`:
> > > > 
> > > > > 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.
> > > > 
> > > > I think there's nothing that prevents a non-zero return value because in fact all pointers would be found present by a `map` clause under unified shared memory.
> > > You would be right to think so if this was not in combination with omp_target_alloc. The fact that it is it means that the pointer was not officially mapped (explicitely or implicitly).
> > So what would be a full motivating example where libomptarget can't return true for any pointer passed to `omp_target_is_present`? Is it in this test?
> Wait, this is not about a motivating example, it's about correctness. The function should return false if A is not "present" on the device. So I think the question is what is the definition of present in this case? As stated before I think in this case A is not actually mapped, perhaps the pointer to A may be implicitly mapped but the actual data belonging to A has not been associated with the pointer.
But the data of `A` will not copied to the device by a `map` clause, right? So it's already present from that perspective.


Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D65001