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

[Shilei Tian via Phabricator via Openmp-commits]

tianshilei1992 added inline comments.


================
Comment at: openmp/libomptarget/src/memory.cpp:32
+
+// Find the previous number that is power of 2 given a number
+inline size_t flp2(size_t Num) {
----------------
JonChesterfield wrote:
> This maps:
> 0 -> 0
> 1 -> 1
> 2 -> 2
> 3 -> 2
> 4 -> 4
> which is not the previous power of two. Round down to a power of two could be:
> `x < 2 ? x : 1 << (31 - __builtin_clz(x - 1))`
> 
That is actually what my expectation. This function is for a number that is not a power of 2. The comment is not accurate, and I'll update it.
The intention here is to distribute different buffers to different buckets based on its previous power of two. For example, 1024, 1025, 1100, 2000 will all go to the bucket with size 1024.


================
Comment at: openmp/libomptarget/src/memory.cpp:71
+class MemoryManagerTy {
+  std::list<Node *> FreeList[NumBuckets];
+  std::list<Node> NodeList;
----------------
JonChesterfield wrote:
> Why list over smallvector? I can't see a need for iterator stability here
Any good suggestion? I also think this style is a little weird, but cannot find a better one.


================
Comment at: openmp/libomptarget/src/memory.cpp:94
+    for (int I = NumBuckets; I >= 0; --I) {
+      std::list<Node *> &List = FreeList[I];
+      std::mutex &Lock = FreeListLocks[I];
----------------
JonChesterfield wrote:
> This is quadratic - each pass around the loop walks through each node of the list
In the worse case, yes. The worse case is equivalent to release all free buffers. That's why this procedure starts from the bucket with largest size. Each time we release one buffer, we will try allocation once, until the allocation succeeds.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81054