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

Mon Jul 29 11:23:28 PDT 2019

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;
----------------
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?

================
Comment at: libomptarget/test/offloading/requires_unified_shared_memory_local.c:25-26
+
+  host_data = (long long) &data[0];
+  host_alloc = (long long) &alloc[0];
+
----------------
grokos wrote:
> Hahnfeld wrote:
> > gtbercea wrote:
> > > Hahnfeld wrote:
> > > > Why are the pointers cast to `long long`? Can we just compare them as `void *`?
> > > Is there a problem I'm missing if they are cast to long long?
> > Yes, casting to `long long` might truncate, or at least I'm not aware that it guarantees to be of the same size as pointers.
> You can cast them to `uintptr_t`, it is a datatype guaranteed to be long enough to represent a pointer.
Again: Why do we need to cast? We can just compare `void *`!

Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D65001