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

[Shilei Tian via Phabricator via Openmp-commits]

tianshilei1992 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.
----------------
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`.


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

https://reviews.llvm.org/D105121