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

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added inline comments.


================
Comment at: openmp/libomptarget/src/MemoryManager.cpp:149
+      FreeListTy &List = FreeLists[I];
+      if (List.empty())
+        continue;
----------------
tianshilei1992 wrote:
> 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.
Double check requires an atomic read, though relaxed is fine. Or probably some use of barriers. Calling empty() while another thread modifies the list is the race.  Because empty() is not atomic qualified, the race is UB.

Empty probably resolves to loading two pointers and comparing them for equality, so I sympathise with the argument that the race is benign, but it's still prudent to remove the data races we know about. Less UB, and means data race detectors will have a better chance of helping find bugs.


================
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:
> 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.
A crash would be fine as we'd notice that. It's data corruption due to the race which is the hazard. Thanks for adding it to a locked region.


================
Comment at: openmp/libomptarget/src/device.cpp:31
+      DataMapMtx(), PendingGlobalsMtx(), ShadowMtx(),
+      LoopTripCnt(D.LoopTripCnt), MemoryManager(nullptr) {}
+
----------------
tianshilei1992 wrote:
> 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.
`std::mutex` can't be copied either. If a new default-initialised mutex is OK as the result of the copy, it would be OK as the result of a move too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81054