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

Ye Luo via Phabricator via Openmp-commits openmp-commits at lists.llvm.org
Wed Aug 12 10:05:32 PDT 2020

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?

  rG LLVM Github Monorepo



More information about the Openmp-commits mailing list