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

[Ye Luo via Phabricator via Openmp-commits]

ye-luo added inline comments.


================
Comment at: openmp/libomptarget/src/MemoryManager.cpp:324
+  if (Threshold)
+    SizeThreshold = Threshold;
+}
----------------
tianshilei1992 wrote:
> 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`.
Q1. Why SizeThreshold is not per device?
Q2. I was asking for a way to opt-out this optimization. But you ignore LIBOMPTARGET_MEMORY_MANAGER_THRESHOLD=0


================
Comment at: openmp/libomptarget/src/MemoryManager.cpp:327
+MemoryManagerTy::MemoryManagerTy(DeviceTy &D, size_t Threshold)
+    : Impl(new impl::MemoryManagerImplTy(D)) {
+  if (Threshold)
----------------
make_unique is better.


================
Comment at: openmp/libomptarget/src/device.cpp:359
+
+  size_t Threshold = 1U << 13;
+
----------------
I think this is your real default. The default value of SizeThreshold always gets overwritten.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81054