[Openmp-commits] [PATCH] D133447: [Libomptarget] Implement OpenMP 5.2 semantics for device pointers

[Joseph Huber via Phabricator via Openmp-commits]

jhuber6 added inline comments.


================
Comment at: openmp/libomptarget/include/device.h:285
+    /// If the pointer is present in the mapping table.
+    unsigned IsPresent : 1;
+  } Flags = {0, 0, 0};
----------------
ye-luo wrote:
> IsPresent is not orthogonal to IsHostPointer. This unamed struct design is flawed.
> 
> Also directly access TargetPointerResultTy member is also bad. There is lack of read only protection. accessing members should via a function.
> 
> something like.
> struct TargetPointerResultTy {
>   std::unique_ptr<EntryFlags> flags# null is the entry doesn't exist.
> 
>   inline bool isPresent() const { return bool(flags); }
> }
> IsPresent is not orthogonal to IsHostPointer. This unamed struct design is flawed.
Could you elaborate on this? They should mean different things, with the current semantics we pass the pointer first-private, but do not assume it is a valid host pointer. That's why we use both `IsPresent` and `IsHostPointer`. For USM we will have `IsPresent` be false and `IsHostPointer` be true, for this implicit mapping `IsHostPointer` will be false.

> Also directly access TargetPointerResultTy member is also bad. There is lack of read only protection. accessing members should via a function.
Sure, I'll add it. I'm not exactly sure what the utility of this struct is anyway, considering that these are just bools.




================
Comment at: openmp/libomptarget/test/mapping/implicit_device_ptr.c:12
+int main(void) {
+  int *A = omp_target_alloc(sizeof(int), omp_get_default_device());
+
----------------
ye-luo wrote:
> Doesn't compile. Need `(int *) omp_target_alloc`
This is C code, the cast shouldn't be necessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133447