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

Shilei Tian via Phabricator via Openmp-commits openmp-commits at lists.llvm.org
Wed Aug 12 10:11:34 PDT 2020

tianshilei1992 marked 3 inline comments as done.
tianshilei1992 added inline comments.

Comment at: openmp/libomptarget/src/MemoryManager.cpp:324
+  if (Threshold)
+    SizeThreshold = Threshold;
ye-luo wrote:
> SizeThreshold is global while Threshold is local. The default values is also different. I'm lost in the logic here.
Yeah, you're lost. By default, `Threshold` is 0, which means we will not overwrite `SizeThreshold`.

Comment at: openmp/libomptarget/src/MemoryManager.h:26
+class MemoryManagerTy {
+  std::unique_ptr<impl::MemoryManagerImplTy> Impl;
ye-luo wrote:
> Why is the pointer needed?
> What is the design logic behind MemoryManagerTy and MemoryManagerImplTy layers? Can we just have one?
Pimpl. Like my previous comments mentioned before, this header will be included by others, I don't want unnecessary headers/declarations/definitions to be included to pollute others.

Comment at: openmp/libomptarget/src/device.h:150
+  /// Memory manager
+  std::shared_ptr<memory::MemoryManagerTy> MemoryManager;
ye-luo wrote:
> Could you explain why shared_ptr is needed?
Such that I don't need to include `MemoryManager.h` in the header, and it doesn't hurt anything.

  rG LLVM Github Monorepo



More information about the Openmp-commits mailing list