[Openmp-commits] [PATCH] D132660: [openmp][amdgpu] Implement target_alloc_host as fine grain HSA memory

Thu Aug 25 07:35:27 PDT 2022

JonChesterfield added inline comments.

================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:2693
   DP("Tgt free data (tgt:%016llx).\n", (long long unsigned)(Elf64_Addr)TgtPtr);
   Err = core::Runtime::Memfree(TgtPtr);
   if (Err != HSA_STATUS_SUCCESS) {
----------------
jhuber6 wrote:
> JonChesterfield wrote:
> > Curious aside - cuda rtl.cpp has a hashtable of pointers to determine what sort of allocation was involved, which is ugly, and also broken if a given address can have different kind across multiple allocations (maybe? cuda implementation property) as it grows without bound on multiple allocations. I was postponing implementing this patch until that could be removed in favour of a dedicated free call, but it turns out HSA does the same sort of map-pointer-to-deallocator under the hood anyway.
> I believe this is born from the [omp_target_free](https://www.openmp.org/spec-html/5.0/openmpsu164.html) function not taking the allocator type. This means if the user wishes to free the memory we can't know what it is unless we record the pointer beforehand. This seems like a major oversight considering that the basic `omp_alloc`/`omp_free` allows this and is in fact what we use in the tests. I'm not sure what the expected semantics are here.
It's a convenience feature for applications that imposes a performance overhead on nvptx. Memory allocated by llvm_omp_target_alloc_host is returned to omp_target_free. If we introduce llvm_omp_target_free_host etc then the hash table can be removed. That the hashtable never has elements removed is a bug, whether it's a memory leak or a correctness error depends on how the address space is partitioned by cuda.

However, it turns out HSA doesn't need the hashtable anyway, so that would make the cuda plugin slightly simpler/faster without changing the amdgpu one. So this patch is standalone.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132660