[Openmp-commits] [PATCH] D81054: [OpenMP] Introduce target memory manager

[Shilei Tian via Phabricator via Openmp-commits]

tianshilei1992 added inline comments.


================
Comment at: openmp/libomptarget/src/MemoryManager.cpp:130
+      deleteOnDevice(N->Ptr);
+    FreeLists[I].clear();
+  }
----------------
ye-luo wrote:
> tianshilei1992 wrote:
> > ye-luo wrote:
> > > N->Ptr is deleted here. Then the shared_ptr in FreeLists[I] is deleted here but PtrToNodeTable still has the shared_ptr and an address which is no more valid.
> > > If I understand correctly, you want FreeLists holds a subset of PtrToNodeTable memory segments.
> > > I think what you need is
> > > ```
> > > using FreeListTy = std::multiset<std::reference_wrapper<NodeTy>, NodeCmpTy>;
> > > std::unordered_map<void *, NodeTy> PtrToNodeTable;
> > > ```
> > > In this way, PtrToNodeTable is the unique owner of all the memory segments. FreeList only owns a reference.
> > This is a nice catch. Thanks for that. Holding a reference will not solve the problem that the node should also be removed from the map. It is same as holding a `shared_ptr`, but I could in fact use the reference way for the `FreeListTy`.
> > The initial implementation is to remove the node from the map table and then add it to the free lists. Later I want to avoid the unnecessary operation on the map table but forget to update here. I’ll fix it.
> The correct code needs to take care of both PtrToNodeTable and FreeLists regardless.
> 
> Currently in the destructor, you first deal with PtrToNodeTable and then FreeLists with some nullptr check.
> If you switch to reference in FreeLists, only PtrToNodeTable needs to be taken care.
> 
> I still hope you find shared_ptr not needed at all.
One benefit to use pointer is that we could use `nullptr` to tell a state, which is very important to narrow the critical area as much as possible. Reference does not have that quality so that I need to do more things in the critical area which is counter-efficient. I can take the map table as a container of nodes and use the raw pointer in the free lists.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81054