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

Ye Luo via Phabricator via Openmp-commits openmp-commits at lists.llvm.org
Tue Aug 18 20:27:03 PDT 2020

ye-luo added inline comments.

Comment at: openmp/libomptarget/src/MemoryManager.cpp:130
+      deleteOnDevice(N->Ptr);
+    FreeLists[I].clear();
+  }
tianshilei1992 wrote:
> 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.
Please don’t use raw pointers.  If you look at reference_wrapper it has the same cost as taking the address and store the address. C++ guru invented that for us in a safe way.

  rG LLVM Github Monorepo



More information about the Openmp-commits mailing list