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

[Ye Luo via Phabricator via Openmp-commits]

ye-luo added inline comments.


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


================
Comment at: openmp/libomptarget/include/device.h:370
+
+  void reset() { *this = TargetPointerResultTy(); }
 };
----------------
jdoerfert wrote:
> 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.
You are right. The new object will be responsible to call the unlock via the destructor. So this is correct. 


================
Comment at: openmp/libomptarget/include/device.h:405
 
-  ShadowPtrListTy ShadowPtrMap;
-
-  std::mutex PendingGlobalsMtx, ShadowMtx;
+  std::mutex PendingGlobalsMtx;
 
----------------
jdoerfert wrote:
> 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.
Then do it separately. Probably also for   PendingCtorsDtorsPerLibrary PendingCtorsDtors;


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123446