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

[Shilei Tian via Phabricator via Openmp-commits]

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


================
Comment at: openmp/libomptarget/src/MemoryManager.cpp:149
+      FreeListTy &List = FreeLists[I];
+      if (List.empty())
+        continue;
----------------
JonChesterfield wrote:
> tianshilei1992 wrote:
> > 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.
> That seems to assume list.empty() is an atomic operation. It isn't - calling list.empty() from one thread while another can be inserting into the list is a data race.
> 
> We could do something involving a relaxed read followed by a lock followed by another read, in the double checked locking fashion. Uncontended locks are cheap though so it's probably not worthwhile.
Double check does not work either. If the `empty` function might crash because of the data race, that is a problem. Otherwise, it is not a problem. Like I said, another thread could still insert node into the list after we check empty using a lock.


================
Comment at: openmp/libomptarget/src/MemoryManager.cpp:193
+    // object, therefore the memory free will not succeed.
+    // Deallocate all memory in map
+    for (std::pair<void *, NodePtrTy> P : PtrToNodeTable) {
----------------
JonChesterfield wrote:
> This seems bad. Perhaps we should call a function to do this work shortly before destroying the target plugin?
That is a potential problem, and actually it might not be a problem. Only when we're going to exit the process can this function be invoked. Even the deallocation will not succeed, GPU memory will be free once the process exits anyway.


================
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);
----------------
JonChesterfield wrote:
> tianshilei1992 wrote:
> > 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.
> It looks like PtrToNodeTable can be modified by other threads while this is running. Doesn't matter that NodePtr itself is unique - can't call .find() on the structure while another thread is mutating it.
The iterators will not be invalidated on `multiset` in insert operation, but anyway, I'm not sure whether it will crash in some middle status, so I'll wrap them into the guard lock.


================
Comment at: openmp/libomptarget/src/device.cpp:31
+      DataMapMtx(), PendingGlobalsMtx(), ShadowMtx(),
+      LoopTripCnt(D.LoopTripCnt), MemoryManager(nullptr) {}
+
----------------
JonChesterfield wrote:
> tianshilei1992 wrote:
> > 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.
> std::vector<DeviceTy> should be content with a move constructor. Then the copy constructor can be = delete.
`std::mutex` cannot be moved. That is the only reason we have the copy constructor.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81054