[Openmp-commits] [PATCH] D86307: [OpenMP] Pack first-private arguments to improve efficiency of data transfer

[Shilei Tian via Phabricator via Openmp-commits]

tianshilei1992 marked an inline comment as done.
tianshilei1992 added inline comments.


================
Comment at: openmp/libomptarget/src/omptarget.cpp:926
       TgtBaseOffset = (intptr_t)HstPtrBase - (intptr_t)HstPtrBegin;
+      const bool MapTo = ArgTypes[I] & OMP_TGT_MAPTYPE_TO;
+      const int64_t ArgSize = ArgSizes[I];
----------------
ye-luo wrote:
> tianshilei1992 wrote:
> > ye-luo wrote:
> > > tianshilei1992 wrote:
> > > > ye-luo wrote:
> > > > > When I read here with a condition on the transfer.
> > > > > I'm wondering is this just first-private? or private is also affected?
> > > > > 
> > > > Just first-private because we don't need data transfer for private arguments.
> > > > I'm wondering is this just first-private? or private is also affected?
> > > > 
> > > I were referring to the whole patch instead of just the transfer here. I guess you patch affect both private and firstprivate.
> > > if I have private(data1, data2, data3), will your change make a single allocation?
> > No. In that case, `MapTo` will be `false`.
> 1. Then rename it IsFirstPrivate. MapTo seems quite misleading.
> 2. In the future, I think it can be nice optimization to allocate all the private in a single shot. Device malloc is still very expensive.
1. Done.
2. I think it depends. Just like I said in the code comments somewhere, we have memory manager now, then the allocation might not go to device directly, especially those small size of memory. If we take all private arguments into account, it will increase more host overhead (more host memory, and extra Boolean variable to tell whether we need to copy from original place to the buffer), and also transfer unnecessary data to target device. In fact, in my first implementation, I did have all private arguments, but later I changed it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86307