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

[George Rokos via Phabricator via Openmp-commits]

grokos added a comment.

What about other instances where we have access to the `HostDataToTargetMap` followed be data transfers, e.g. when removing a mapping and copy data back to the host? Shouldn't we apply the same logic there?



================
Comment at: openmp/libomptarget/src/device.cpp:252-253
       IsHostPtr = true;
+      // We don't need to copy data in this case
+      MoveData = false;
       TargetPointer = HstPtrBegin;
----------------
Redundant, if we use USM and don't have the `close` modifier, then `MoveData` will be false already.


================
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.
----------------
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;
      }
    }
```



================
Comment at: openmp/libomptarget/src/omptarget.cpp:564-565
       TgtPtrBase = (void *)((uint64_t)TgtPtrBegin - Delta);
       int rt = Device.submitData(PointerTgtPtrBegin, &TgtPtrBase,
                                  sizeof(void *), AsyncInfo);
       if (rt != OFFLOAD_SUCCESS) {
----------------
Here we also need to take care of this data transfer. Copying the pointee object onto the device and updating the device pointer should behave as a single atomic operation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104555