[Openmp-commits] [PATCH] D123446: [OpenMP][FIX] Remove shadow pointer map and introduce consistent locking

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added inline comments.


================
Comment at: openmp/libomptarget/include/device.h:343
+  }
+
+  ~TargetPointerResultTy() {
----------------
ye-luo wrote:
> Please delete copy constructor and copy assign operator.
Aren't they deleted anyway? I can make it explicit, sure.


================
Comment at: openmp/libomptarget/include/device.h:367
+    if (Entry && Lock)
+      Entry->lock();
+  }
----------------
ye-luo wrote:
>   void lock() const { States->UpdateMtx.lock(); }
>   void unlock() const { States->UpdateMtx.unlock(); }
> 
> the mutex is only intended to protect data transfer submission.
The mutex is now used to protect the entire entry. I will update the comment.
While the data transfer is running the entry should not be looked at, similarly, while anything might modify the entry, the entry should not be looked at. Hence, one can only get the entry locked.


================
Comment at: openmp/libomptarget/include/device.h:370
+
+  void reset() { *this = TargetPointerResultTy(); }
 };
----------------
ye-luo wrote:
> I don't get the intention of this function. Need documentation. It also seems buggy. Don't you need to unlock Entry before assign?
It's not buggy, IMHO. Assignment will swap the entry, which may or may not be locked. The new object goes out of scope immediately which will trigger the destructor. If there is an entry it will be unlocked, if there was none, all is already good.

This function is needed to give up ownership of an entry early, that is while the TPR is still live but effectively not needed anymore.


================
Comment at: openmp/libomptarget/include/device.h:405
 
-  ShadowPtrListTy ShadowPtrMap;
-
-  std::mutex PendingGlobalsMtx, ShadowMtx;
+  std::mutex PendingGlobalsMtx;
 
----------------
ye-luo wrote:
> Do you know what this is? Add some documentation?
We do know what this is. And we should add some documentation. We should do so in a separate patch though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123446