[Openmp-commits] [PATCH] D123443: [OpenMP][NFCI] Cleanup APIs and improve object encapsulation

Tue Apr 12 08:05:19 PDT 2022

jdoerfert added inline comments.

================
Comment at: openmp/libomptarget/src/device.cpp:219
       // It might have been allocated with the parent, but it's still new.
-      IsNew = HT.getTotalRefCount() == 1;
+      TPR.setIsNew(HT.getTotalRefCount() == 1);
       RefCountAction = " (update suppressed)";
----------------
ye-luo wrote:
> jdoerfert wrote:
> > ye-luo wrote:
> > > jdoerfert wrote:
> > > > ye-luo wrote:
> > > > > Can we avoid all the setXXX functions?
> > > > > I feel better keep the local IsNew and then return by calling TPR::TPR(Entry, TargetPointer, IsNew...)
> > > > But then I need to do manual locking again and I would like us to avoid that.
> > > Do you have a patch with locking added? Try to understand better your intention.
> > I do not. Every `setEntry` will lock the entry and unlock the lat entry, if any. Every time TPR goes out of scope, e.g., when we give up and return an dummy object, you unlock the entry. If you do manual locking the caller needs to also unlock when TPR is destroyed, various places all across the code base.
> Assuming the lock is in the entry, it makes sense.
Yes. Entry is the one with the lock. TPR is bacically a exclusive accessor with some extra information. @tianshilei1992 asked for more documentation, I will add that before I merge.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123443