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

[Shilei Tian via Phabricator via Openmp-commits]

tianshilei1992 marked 12 inline comments as done.
tianshilei1992 added inline comments.


================
Comment at: openmp/libomptarget/src/MemoryManager.cpp:107
+  /// Nodes are used in a format of \p std::shared_ptr<Node>
+  using NodePtrTy = std::shared_ptr<NodeTy>;
+
----------------
ye-luo wrote:
> Another shared_ptr. See `typedef std::set<HostDataToTargetTy, std::less<>> HostDataToTargetListTy;` as an example. There doesn't seem to need a pointer wrapping NodeTy.
We might have same nodes both in the table and the free list. It is on purpose because the map relation never changes, which could save us an operation on the map.


================
Comment at: openmp/libomptarget/src/MemoryManager.cpp:149
+      FreeListTy &List = FreeLists[I];
+      if (List.empty())
+        continue;
----------------
ye-luo wrote:
> There can be race when you test List.empty().
It is on purpose, and the "race" is not a problem. Think about it. Even we wrap it into a lock and now it is empty, there is still chance that when we move to the next list, one or more nodes are returned to this list. No difference.


================
Comment at: openmp/libomptarget/src/MemoryManager.cpp:273
+    // Insert the node into the map table if it is not there
+    if (PtrToNodeTable.find(NodePtr->Ptr) == PtrToNodeTable.end()) {
+      std::lock_guard<std::mutex> Guard(MapTableLock);
----------------
ye-luo wrote:
> There can be race in PtrToNodeTable when you find()
There is no race because same `NodePtr` will never go to two threads.


================
Comment at: openmp/libomptarget/src/MemoryManager.cpp:327
+MemoryManagerTy::MemoryManagerTy(DeviceTy &D, size_t Threshold)
+    : Impl(new impl::MemoryManagerImplTy(D)) {
+  if (Threshold)
----------------
ye-luo wrote:
> make_unique is better.
Sure.


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


================
Comment at: openmp/libomptarget/src/MemoryManager.h:30
+  /// Constructor
+  MemoryManagerTy(DeviceTy &D, size_t Threshold = 0);
+
----------------
ye-luo wrote:
> Second (Third?) place with a default. Remove or error out if size 0?
Could remove it.


================
Comment at: openmp/libomptarget/src/MemoryManager.h:26
+class MemoryManagerTy {
+  std::unique_ptr<impl::MemoryManagerImplTy> Impl;
+
----------------
ye-luo wrote:
> tianshilei1992 wrote:
> > ye-luo wrote:
> > > tianshilei1992 wrote:
> > > > ye-luo wrote:
> > > > > Why is the pointer needed?
> > > > > What is the design logic behind MemoryManagerTy and MemoryManagerImplTy layers? Can we just have one?
> > > > Pimpl. Like my previous comments mentioned before, this header will be included by others, I don't want unnecessary headers/declarations/definitions to be included to pollute others.
> > > That is the job of header and cpp files.
> > No. You can refer to https://en.cppreference.com/w/cpp/language/pimpl for more details.
> > Pimpl. Like my previous comments mentioned before, this header will be included by others, I don't want unnecessary headers/declarations/definitions to be included to pollute others.
> 
> Where else do you have in mind this header will be included? So far there is only device.cpp.
That attributes to the Pimpl idiom. It is not a good practice to have too much implementation stuffs in the header file.


================
Comment at: openmp/libomptarget/src/device.cpp:31
+      DataMapMtx(), PendingGlobalsMtx(), ShadowMtx(),
+      LoopTripCnt(D.LoopTripCnt), MemoryManager(nullptr) {}
+
----------------
ye-luo wrote:
> Why do you think it is OK here leaving the copy constructor always setting MemoryManager nullptr? This cause surprises. The same question applies to assign operator as well.
`MemoryManager` will be initialized separately later. The only reason we need this is `std::vector<DeviceTy>` requires it. We don't copy or construct those objects afterwards.


================
Comment at: openmp/libomptarget/src/device.cpp:359
+
+  size_t Threshold = 1U << 13;
+
----------------
ye-luo wrote:
> I think this is your real default. The default value of SizeThreshold always gets overwritten.
Yes. The logic is a little weird. I'll refactor this part.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81054