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

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert accepted this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.

Some comments and nits you should take under consideration.

I'm not 100% sold on the list design, that we look for the exact size, and that we traverse the list while we look. 
However, this is improving a lot over the status quo and we can revisit this with more profiling information later.

LGTM

Nits:
I'd rename `memory.h` into `MemoryManager.h` if we don't expect anything else to go in there that is not "a memory manager" at the end of the day. Same with the cpp.
I'm not sure we need the `memory` namespace, or the `impl` namespace for that matter.



================
Comment at: openmp/libomptarget/src/memory.cpp:24
+// also set such that if the device already supports this, memory manager will
+// be disabled automatically.
+//
----------------
The last sentence is now obsolete. I'd just state that there is an environment variable to set the threshold for which we will manage allocations. Please actually put the name of the variable here ;).


================
Comment at: openmp/libomptarget/src/memory.cpp:68
+  return Num >> 1;
+}
+
----------------
Can we rename `flp2` into `findPreviousPowerOfTwo` or something similarly descriptive?


================
Comment at: openmp/libomptarget/src/memory.cpp:157
+        deleteOnDevice(N->Ptr);
+      FreeLists[I].clear();
+    }
----------------



================
Comment at: openmp/libomptarget/src/memory.cpp:249
+        TgtPtr = freeAndAllocate(Size, HstPtr);
+      }
+
----------------
This pattern occurs at least twice, might be worth to put it in a helper method, e.g., `allocateOrFreeAndAllocate` for the lack of a better name ;)


================
Comment at: openmp/libomptarget/src/memory.cpp:253
+      if (TgtPtr == nullptr) {
+        DP("Still cannot get memory on device. Return nullptr.\n");
+        return nullptr;
----------------
This message should be more descriptive I guess. "Return nullptr" is not helpful. Maybe spell out that we failed to allocate the requested memory, the device might be OOM. I guess this is also a good spot for some debugger events eventually...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81054