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

[Ye Luo via Phabricator via Openmp-commits]

ye-luo added a comment.

In addition,

1. the DeviceTy copy constructor and assign operator are imperfect before this patch. I don't think we can fix them this patch. We should just document the imperfection here.
2. Because the memory limit is per allocation, it seems that the MemoryManager can still hold infinite amount of memory and we don't have  way to free them. I'm concerned about having this feature on by default.



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


================
Comment at: openmp/libomptarget/src/MemoryManager.cpp:324
+  if (Threshold)
+    SizeThreshold = Threshold;
+}
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81054