[Openmp-commits] [PATCH] D104691: [AMDGPU][Libomptarget] Move allow_access_to_all_gpu_agents to rtl.cpp

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added a comment.

Moving the function is good, nice that we don't have to re-assemble the list of agents. The change to Malloc is (probably, I haven't checked all the call sites) correct but will make it easy to forget the register call in a later change, so I think we want to route all calls through HostMalloc and DeviceMalloc and make Runtime::Malloc harder to call by accident, e.g. scope it to the file and rename it MallocImpl or similar



================
Comment at: openmp/libomptarget/plugins/amdgpu/impl/data.cpp:66
-
-  if (err == HSA_STATUS_SUCCESS) {
-    err = register_allocation(*ptr, size, DeviceType);
----------------
it's not obvious this is dead - if Runtime::Malloc is called with DEVTYPE_CPU, but not from HostMalloc (where the follow on handling is added), then we miss the allow_access call. Can this Runtime::Malloc be made local to this TU, so that we can't accidentally call it instead of one of the DeviceMalloc or HostMalloc wrappers?


================
Comment at: openmp/libomptarget/plugins/amdgpu/impl/system.cpp:940
                 info.size);
-    err = register_allocation(reinterpret_cast<void *>(info.addr),
-                              (size_t)info.size, ATMI_DEVTYPE_GPU);
----------------
this was dead because it passes DEVTYPE_GPU which register_allocation ignores


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104691