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

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert 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};
----------------
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.
> 
> 
1) IsPresent and IsHostPointer are orthogonal, as elaborated this morning in the call.
2) All the rest is unrelated to this patch. Potentially fair but unrelated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133447