[Openmp-commits] [PATCH] D104555: [OpenMP][Offloading] Fix data race in data mapping by using two locks

[Shilei Tian via Phabricator via Openmp-commits]

tianshilei1992 added inline comments.


================
Comment at: openmp/libomptarget/src/omptarget.cpp:518-530
+        if (arg_types[i] & OMP_TGT_MAPTYPE_ALWAYS)
+          MoveData = 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.
+          // If this is a PTR_AND_OBJ entry, the OBJ is not part of the struct,
+          // so exclude it from this check.
----------------
tianshilei1992 wrote:
> grokos wrote:
> > This part must be updated to match the changes introduced in D105121. Also, why do we set `MoveData = true` only when the `ALWAYS` flag is set? It should also be true if the mapping is new, but the problem here is that we don't know whether or not we have a new mapping because `getTargetPointer` (ex `getOrAllocTgtPtr`) hasn't been called yet. So I think `MoveData` should be a 3-state integer (maybe use an enum for it) which will pass instructions from `targetDataBegin` to `getTargetPointer` about what to do:
> >   - Case 0: `false`, the `TO` flag hasn't been set (or we are using USM without the `close` modifier etc.), so don't move data no matter what
> >   - Case 1: `true`, the `TO` and `ALWAYS` flags have been set, so move data
> >   - Case 2: `undecided`, the `TO` flag has been set but `targetDataBegin` doesn't know whether the mapping is new, so move data if `RefCount==1`
> > So this part of the logic should look something like this:
> > ```
> >     enum MoveState MoveData = MoveState::false;
> >     if (arg_types[i] & OMP_TGT_MAPTYPE_TO) {
> >       if (!(PM->RTLs.RequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY) ||
> >           HasCloseModifier) {
> >         if (arg_types[i] & OMP_TGT_MAPTYPE_ALWAYS)
> >           MoveData = MoveState::true;
> >         else
> >           MoveData = MoveState::undecided;
> >       }
> >     }
> > ```
> > 
> My intention is to cover new entry in `getTargetPointer`, but I did forget to set `MoveData` to `true` in the function if there is a new entry.
I refined the logic but seems like it almost broke all declare mapper tests…Need to investigate further.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104555