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

Jon Chesterfield via Phabricator via Openmp-commits openmp-commits at lists.llvm.org
Wed Aug 12 22:14:19 PDT 2020

JonChesterfield added inline comments.

Comment at: openmp/libomptarget/src/MemoryManager.cpp:149
+      FreeListTy &List = FreeLists[I];
+      if (List.empty())
+        continue;
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.

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) {
This seems bad. Perhaps we should call a function to do this work shortly before destroying the target plugin?

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);
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.

Comment at: openmp/libomptarget/src/device.cpp:31
+      DataMapMtx(), PendingGlobalsMtx(), ShadowMtx(),
+      LoopTripCnt(D.LoopTripCnt), MemoryManager(nullptr) {}
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.

  rG LLVM Github Monorepo



More information about the Openmp-commits mailing list