[Openmp-commits] [PATCH] D142514: [OpenMP][libomptarget] Notify the plugins regarding new mapping/unmappings

[Ye Luo via Phabricator via Openmp-commits]

ye-luo added inline comments.


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp:797
+  auto PinnedPtrOrErr = PinnedAllocs.lockHostBuffer(
+      HstPtr, Size, /* check whether already locked */ true);
+  if (!PinnedPtrOrErr)
----------------
jdoerfert wrote:
> ye-luo wrote:
> > jdoerfert wrote:
> > > ye-luo wrote:
> > > > I don't understand this part of code.
> > > > The implementation actually lock the host buffer instead of just notifying.
> > > While it is mapped, we want to keep it (by default) pinned. The escape hatch (env var) is missing though.
> > > This allows us to avoid the pin/unpin on the data transfer back and intermediate updates.
> > Unconditionally pinning mapped host memory should be an option to opt-in rather than default.
> > If I just map 10 scalars, don't pin them, staging can be better.
> > if users want fine-grain control, use the lock API. For really lazy users, give them the env var to opt-in.
> Mapping scalars as firstprivate or as tofrom? The former would not go through this mechanism. The latter makes reasonable sense to pin the page, no?
> 
> I agree we need an env var, default is the question.
I was referring to `map(tofrom)` scalars. firstprivte is not considered a map technically. I don't think you want to pay the cost of pinning 10 pages due to 10 scalars. Pinning is very expensive (interacting with OS).


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

https://reviews.llvm.org/D142514