[Openmp-commits] [PATCH] D94379: [OpenMP] Move memory manager to plugin and make it a common interface

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added inline comments.


================
Comment at: openmp/libomptarget/plugins/common/MemoryManager.h:96
+}
+} // namespace
+
----------------
As clang tidy tells you, this can be problematic. Easy way out of this is to move the helpers as static functions into the memory manager, especially given that they look at MEMORY_MANAGER env vars.


================
Comment at: openmp/libomptarget/plugins/common/MemoryManager.h:114
+/// have an allocator for each device.
+class MemoryManagerTy {
+  /// A structure stores the meta data of a target pointer
----------------
Given tat you have the DeviceAllocatorTy already, why not inherit here and in the cuda class?


================
Comment at: openmp/libomptarget/plugins/common/MemoryManager.h:279
+      DP("Find one node " DPxMOD " in the bucket.\n", DPxPTR(NodePtr));
+#endif
+
----------------
Nit: No need for the ifdef, I think. Given all the other DP macros around anyway. also other places.


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:28
 
+#include "../../common/MemoryManager.h"
+
----------------
common should be in the include path via cmake.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94379