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

[Shilei Tian via Phabricator via Openmp-commits]

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


================
Comment at: openmp/libomptarget/src/device.cpp:204
+      uintptr_t tp =
+          (uintptr_t)MemoryManager.allocate(Size, HstPtrBegin, DeviceID);
       DP("Creating new map entry: HstBase=" DPxMOD ", HstBegin=" DPxMOD ", "
----------------
jdoerfert wrote:
> Nit: make `tp` a `void *` and cast the one use of it as `uintptr_t` instead.
Unrelated to this patch so mark it as Done.


================
Comment at: openmp/libomptarget/src/device.cpp:285
           DPxPTR(HT.TgtPtrBegin), Size);
-      RTL->data_delete(RTLDeviceID, (void *)HT.TgtPtrBegin);
+      MemoryManager.free((void *)HT.TgtPtrBegin, DeviceID);
       DP("Removing%s mapping with HstPtrBegin=" DPxMOD ", TgtPtrBegin=" DPxMOD
----------------
jdoerfert wrote:
> Nit: Remove the cast.
Unrelated to this patch so mark it as Done.


================
Comment at: openmp/libomptarget/src/memory.cpp:70
+
+class MemoryManagerTy {
+  std::list<Node *> FreeList[NumBuckets];
----------------
jdoerfert wrote:
> Comment explaining this thing. I'm also very confused by the duplicated class declaration. Let's not do that.
Not duplicate. I just don't want to put too many things into a header that will be included by others. Use PImpl could make things better.


================
Comment at: openmp/libomptarget/src/memory.cpp:150
+        deleteFromDevice(N.Ptr);
+  }
+
----------------
jdoerfert wrote:
> If this is part of the device, the place we tear down the context, this issue should go away, I think.
Comments seems out of date.


================
Comment at: openmp/libomptarget/src/memory.cpp:153
+  void *allocate(size_t Size, void *HstPtr) {
+    assert(Size && "Size is zero");
+
----------------
jdoerfert wrote:
> At least for malloc and friends that is totally fine btw. If we filter this earlier we can leave the assert though.
Comments seems out of date.


================
Comment at: openmp/libomptarget/src/memory.cpp:172
+      // Allocate one from device
+      DevPtr = allocateFromDevice(Size, HstPtr);
+
----------------
jdoerfert wrote:
> We should round the size up to increase reuse. Also makes all blocks in a bucket the same size.
Comments seems out of date.


================
Comment at: openmp/libomptarget/src/memory.cpp:197
+        P = &NodeList.back();
+      }
+    } else {
----------------
jdoerfert wrote:
> As the lock is released, this can/should go into a helper function.
Comments seems out of date.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81054