[Openmp-commits] [PATCH] D105121: [OpenMP] Avoid checking parent reference count in targetDataBegin

[Joel E. Denny via Phabricator via Openmp-commits]

jdenny added inline comments.


================
Comment at: openmp/libomptarget/src/omptarget.cpp:541-542
           copy = true;
-        } else if ((arg_types[i] & OMP_TGT_MAPTYPE_MEMBER_OF) &&
-                   !(arg_types[i] & OMP_TGT_MAPTYPE_PTR_AND_OBJ)) {
-          // Copy data only if the "parent" struct has RefCount==1.
----------------
tianshilei1992 wrote:
> jdenny wrote:
> > protze.joachim wrote:
> > > What is the meaning of these flags?
> > > 
> > > In code like the following, the `target` region should not copy any data, because S is already mapped in the `enter data`
> > > ```
> > > struct{int a; int b;}s_s S;
> > > #pragma omp target enter data map(to:S)
> > > #pragma omp target map(tofrom:S.b)
> > > {...}
> > > #pragma omp target exit data map(from:S)
> > > ```
> > I've verified that, with this patch, your example here works as expected.
> > 
> > I could have left in the `else if` here and simply replaced its body with `copy = IsNew`.  That would make this code look more like the corresponding code in `targetDataEnd` in D104924.  However, the `else if` would have had no effect then:  If `IsNew==true`, then the `if` is taken either way.  If `IsNew==false`, then `copy = IsNew = false`, but that doesn't change the value of `copy`.
> > 
> > The general meaning of the flags is documented in `openmp/libomptarget/include/omptarget.h`.  I assume it's not necessary to check `OMP_TGT_MAPTYPE_MEMBER_OF` anymore because the point of this patch is that we no longer check the parent: `IsNew` seems sufficient whether or not there's a parent.  I assume it's not necessary to check `OMP_TGT_MAPTYPE_PTR_AND_OBJ` anymore because 5adb3a6d86ee says the issue there is the object has its own reference count.  Again, that's the point of this patch: we check the the object's reference count not the parent's.
> > 
> > But I really need someone who understands the history of this code to verify my reasoning.
> > What is the meaning of these flags?
> There is a "full" (probably not) list of the cases for data mapping in the function `generateInfoForComponentList` in `clang/lib/CodeGen/CGOpenMPRuntime.cpp`.
Thanks for mentioning that.  At least some of that documentation appears to be out of date.  For example, I just tried the `map(to: s.ps->ps)` case, and clang generated different map flags.  But it's still a good list of cases.


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

https://reviews.llvm.org/D105121