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

Joel E. Denny via Phabricator via Openmp-commits openmp-commits at lists.llvm.org
Tue Jun 29 09:50:32 PDT 2021


jdenny added a comment.

@jdoerfert, @jhuber6: Thanks for the pointers on FAROS.  I'll look into it.

In D105121#2847477 <https://reviews.llvm.org/D105121#2847477>, @protze.joachim wrote:

> I think, we need more tests challenging the reference counting for mapping rules to make sure that the code change is valid.

I'm happy to add tests.  My problem is figuring out the relevant use cases.  Normally I'd add/update tests related to the behavior I'm changing, but I'm not expecting behavior to change (except possibly performance due to the elimination of a lock).  Perhaps FAROS will uncover something.

> Also, is the expectation that the implementation respects the 5.2 clarifications/changes regarding reference counting of struct members (probably issue #1909)?

I didn't intend this patch to be related to 5.2.  As far as I can tell, openmp spec issue 1909 and its pr 1990 are about attempts to map a range that includes multiple previous mappings from other directives.  I'm not aware of any way this patch affects that issue.



================
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.
----------------
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.


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

https://reviews.llvm.org/D105121



More information about the Openmp-commits mailing list