[Openmp-commits] [PATCH] D83062: [OpenMP] Implement TR8 `present` map type modifier in runtime (2/2)

[George Rokos via Phabricator via Openmp-commits]

grokos added inline comments.


================
Comment at: openmp/libomptarget/src/omptarget.cpp:275
+      // afterward, so the pointer is already allocated by this time.  Also,
+      // "declare target" can produce a PTR_AND_OBJ entry for a global, whose
+      // pointer should already be allocated by this time.  As a result,
----------------
It's `declare target link`, not plain `declare target`.


================
Comment at: openmp/libomptarget/src/omptarget.cpp:268
+      // The only case we can have a PTR_AND_OBJ entry is if we have a pointer
+      // which is member of a struct or element of an array.  For example:
+      //
----------------
jdenny wrote:
> grokos wrote:
> > jdenny wrote:
> > > grokos wrote:
> > > > jdenny wrote:
> > > > > This comment is incorrect.  We can also have PTR_AND_OBJ in the case of `omp declare target link` as discussed at <http://lists.llvm.org/pipermail/openmp-dev/2020-July/003586.html>.
> > > > Right, the comment is not relevant anymore. But I think the conveyed message is still correct. Apart from structs/arrays, if we have a `PTR_AND_OBJ` for a global pointer then by definition the pointer itself has been mapped (since it's a global). Maybe update the comment? The logic is still correct I think.
> > > Yes, I believe the code is right.  I'll try to generalize the comment.
> > I was just reminded that globals marked with `declare target link` are also PTR_AND_OBJ entries. `declare target link` already works in the exact same way as the proposed change we discussed at the July 15 telecon.
> If we're so sure it's impossible to have `!Pointer_TgtPtrBegin` here, should we have an assert?
> 
> That way, if the situation evolves, testing will hopefully reveal when our assumptions here are wrong.  Based on my understanding now, it would probably indicate a bug somewhere.  But I guess it might just mean the assert then needs to be removed and the comment needs to be updated.  Either way, it will be good to know about the change.
> 
> My understanding is that assert would only be active for debug builds.  For release builds, we could still have the `if` and return `OFFLOAD_FAIL` for the sake of defensive programming.
Yes, we can include an assertion here.


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

https://reviews.llvm.org/D83062