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

[Ye Luo via Phabricator via Openmp-commits]

ye-luo accepted this revision.
ye-luo added inline comments.
This revision is now accepted and ready to land.


================
Comment at: openmp/libomptarget/include/device.h:285
+    /// If the pointer is present in the mapping table.
+    unsigned IsPresent : 1;
+  } Flags = {0, 0, 0};
----------------
jhuber6 wrote:
> 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.
> 
> 
OK. I was thinking of IsHostPointer for zero-length array. Actually your are right they are for USM. There is still some interaction between isNew and isPresent. I think isNew only make sense when isPresent is true.

Maybe std::unque_ptr is not a good option because it involves memory alloc/dealloc, std::optional is better but it requires c++17. Probably still need to stay in C++14 now. So the current way is acceptable.


================
Comment at: openmp/libomptarget/src/device.cpp:428
+    IsHostPtr = false;
+    TargetPointer = HstPtrBegin;
   }
----------------
I'm wondering if the USM case and this case should be fused in a more compact way.
The action is the same, when a pointer is not present in the map table. Keep the original value.

Even the logic of USM is fishy https://github.com/llvm/llvm-project/blob/178554f3c8f983bd192818b6d46e4d95560c4964/openmp/libomptarget/src/api.cpp#L133
But I would consider doing any related change in a separate patch.


================
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());
+
----------------
jhuber6 wrote:
> ye-luo wrote:
> > Doesn't compile. Need `(int *) omp_target_alloc`
> This is C code, the cast shouldn't be necessary.
You are right, I was compiling as C++.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133447