[Openmp-commits] [PATCH] D138389: [OpenMP][libomptarget] Add AMDGPU NextGen plugin with asynchronous behavior

[Kevin Sala Penadés via Phabricator via Openmp-commits]

kevinsala added inline comments.


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:181
+  /// Getter of the HSA memory pool.
+  hsa_amd_memory_pool_t get() const { return MemoryPool; }
+
----------------
jdoerfert wrote:
> JonChesterfield wrote:
> > this is pointless, just make the member public if we have to leak it. but again, existing code...
> It's not pointless, it gives a common name for abstracted resources. Often this would be `operator*` or similar but `get` is as good as anything for now. Let's not go down rabbit holes that are indeed pointless.
I don't think it's pointless; we limit the access to the HSA handle and only expose it when it's really needed. And we can also detect easily where these handles are being accessed.


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:1931
+    bool Found = false;
+    HostAllocationsMutex.lock_shared();
+    if (!HostAllocations.empty()) {
----------------
jdoerfert wrote:
> kevinsala wrote:
> > JonChesterfield wrote:
> > > I can't work out what's going on here. The corresponding logic for erase looks up the pointer directly, should found not be the same? Also can't tell why we're recording the size of the allocation next to the pointer, as opposed to a DenseSet<void*>
> > The `__tgt_rtl_data_delete` operation should pass the same pointer provided by the `__tgt_rtl_data_alloc`. As far as I know, it's not valid to make a partial deletion of an allocated buffer. But in the case of `__tgt_rtl_data_submit/retrieve`, the pointer can come with an applied offset. Thus, we should check whether the provided pointer is inside any host allocation, considering the sizes of the allocations.
> We might want to make our lives easier here and not do a search.
> Later we want two changes that will help:
> 1) Have a pre-allocated pinned buffer for arguments.
> 2) Use the hsa lookup function if it's not a pointer to pinned memory allocated by us.
> @kevinsala You think we need the search for the current use cases?
We can replace the search by a call to `hsa_amd_pointer_info` and let the HSA runtime do the work. If the buffer is explicitly locked by the user (malloc + HSA lock), it should return `HSA_EXT_POINTER_TYPE_LOCKED`. If the buffer was allocated using the HSA allocator functions, I guess it will return `HSA_EXT_POINTER_TYPE_HSA`. This latter does not mean that the buffer is host pinned memory because it may also be any other kind of memory allocated through HSA API. But we can assume the user won't pass such invalid buffer types.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138389