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

[Shilei Tian via Phabricator via Openmp-commits]

tianshilei1992 marked an inline comment as done.
tianshilei1992 added inline comments.


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


================
Comment at: openmp/libomptarget/src/MemoryManager.cpp:324
+  if (Threshold)
+    SizeThreshold = Threshold;
+}
----------------
ye-luo wrote:
> tianshilei1992 wrote:
> > ye-luo wrote:
> > > ye-luo wrote:
> > > > tianshilei1992 wrote:
> > > > > ye-luo wrote:
> > > > > > SizeThreshold is global while Threshold is local. The default values is also different. I'm lost in the logic here.
> > > > > Yeah, you're lost. By default, `Threshold` is 0, which means we will not overwrite `SizeThreshold`.
> > > > Q1. Why SizeThreshold is not per device?
> > > > Q2. I was asking for a way to opt-out this optimization. But you ignore LIBOMPTARGET_MEMORY_MANAGER_THRESHOLD=0
> > > Remove Q2. Opt-out has been supported.
> > Then how could you specify the threshold via environment variable for each device? You don't even know how many devices you're gonna have during the compilation time.
> > Then how could you specify the threshold via environment variable for each device? You don't even know how many devices you're gonna have during the compilation time.
> 
> Although your current implementation via environment variable cannot specify the size for each device, we may use configuration file in the future to control this. It will be helpful If you can facilitate this when cleaning up the logic for the default value.
I would prefer to keep current one. If in the future we have request for the per-device threshold, we could change it by the time. This can keep the implementation consistent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81054