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

[Shilei Tian via Phabricator via Openmp-commits]

tianshilei1992 added a comment.

In D81054#2208730 <https://reviews.llvm.org/D81054#2208730>, @jdoerfert wrote:

> 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.

The "list" is not a real list. It is a `std::multiset` here. So basically its look up complexity is `O(logn)` on average. If we don't have such a thing, what would be a better way to organize those free nodes with different sizes?

> 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.

Done.

> I'm not sure we need the `memory` namespace, or the `impl` namespace for that matter.

I prefer to leave the namespace. Current implementation of `libomptarget` has really poor code style. This is a totally new file. I hope to make it right from it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81054