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

[Ye Luo via Phabricator via Openmp-commits]

ye-luo added inline comments.


================
Comment at: openmp/libomptarget/src/MemoryManager.cpp:130
+      deleteOnDevice(N->Ptr);
+    FreeLists[I].clear();
+  }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81054