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

Thu Aug 25 07:25:37 PDT 2022

jhuber6 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) {
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132660