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

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added a comment.

OK, cool. If we're open to changing the implementation later this is fine by me. An instance per host thread is likely to be better than all the internal locks. Couple of minor comments above.

There are use cases for allocating device memory within the plugin itself. I think including MemoryManager.h from within the plugin would work for that.



================
Comment at: openmp/libomptarget/src/MemoryManager.h:27
+class MemoryManagerTy {
+  std::shared_ptr<impl::MemoryManagerImplTy> Impl;
+
----------------
Can we drop the shared_ptr here? Better to have the MemoryManager move-only and use unique_ptr


================
Comment at: openmp/libomptarget/src/MemoryManager.h:37
+
+  /// Deallocate memory pointed by \p TgtPtr
+  int free(void *TgtPtr);
----------------
Deallocate taking a size usually allows a faster implementation, but that can be left until said faster implementation is proposed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81054