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

[Ye Luo via Phabricator via Openmp-commits]

ye-luo added a comment.

1. Please mention LIBOMPTARGET_MEMORY_MANAGER_THRESHOLD, default value and unit in the patch summary.
2. Is it possible to have a unit test testing the manager class behaviors?
3. Can we offload to host and run address sanitizer or valgrind?

I'm not sure if I'm asking for too much here.



================
Comment at: openmp/libomptarget/src/MemoryManager.cpp:324
+  if (Threshold)
+    SizeThreshold = Threshold;
+}
----------------
SizeThreshold is global while Threshold is local. The default values is also different. I'm lost in the logic here.


================
Comment at: openmp/libomptarget/src/MemoryManager.h:26
+class MemoryManagerTy {
+  std::unique_ptr<impl::MemoryManagerImplTy> Impl;
+
----------------
Why is the pointer needed?
What is the design logic behind MemoryManagerTy and MemoryManagerImplTy layers? Can we just have one?


================
Comment at: openmp/libomptarget/src/device.h:150
+  /// Memory manager
+  std::shared_ptr<memory::MemoryManagerTy> MemoryManager;
+
----------------
Could you explain why shared_ptr is needed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81054