[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/device.cpp:208-211
+    } else if (HasPresentModifier) {
+      DP("Mapping required but does not exist%s for HstPtrBegin=" DPxMOD
+         ", Size=%ld\n",
+         (IsImplicit ? " (implicit)" : ""), DPxPTR(HstPtrBegin), Size);
----------------
I think this else-if should be moved right after the else-if that checks for explicit extension of mapping (i.e. after line 194) and outside the `else if (Size)` branch. If we have the present modifier then the data must be mapped already no matter whether a size is specified.


================
Comment at: openmp/libomptarget/src/omptarget.cpp:266-269
+      // TODO: Do we actually need to check the present modifier here?  I
+      // haven't found an example where the getOrAllocTgtPtr call here fails
+      // because, so far in my attempts, it's always already present due to an
+      // earlier map entry for the object holding the pointer.
----------------
Correct. 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. This means that whenever we have something like:
```
#pragma omp target map(s.p[0:N])
```
clang will first map the struct/array and then we'll visit the `PTR_AND_OBJ` entry, so the space for the pointer itself has been allocated already. I think it's safe to skip this check here (or you can leave it as a form of defensive programming).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83062