[AMDGPU][Libomptarget] Move allow_access_to_all_gpu_agents to rtl.cpp

[pdhaliwal]

Summary

Moving this method helps eliminate a use of g_atl_machine.

[JonChesterfield]

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

openmp/libomptarget/plugins/amdgpu/impl/data.cpp

66

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?

openmp/libomptarget/plugins/amdgpu/impl/system.cpp

940

this was dead because it passes DEVTYPE_GPU which register_allocation ignores

[pdhaliwal]

openmp/libomptarget/plugins/amdgpu/impl/data.cpp

66

Malloc is already declared inside Runtime class as private static method.

[JonChesterfield]

Ah yes, so it is. Not sure we're getting much value from the static member functions in rt.h, relative to the free functions found elsewhere, but that's unrelated to this change.

openmp/libomptarget/plugins/amdgpu/impl/data.cpp

 
 hsa_amd_memory_pool_t get_memory_pool_by_mem_place(int DeviceId,
                                                    atmi_devtype_t DeviceType) {
   ATLProcessor &proc = get_processor_by_mem_place(DeviceId, DeviceType);
   return get_memory_pool(proc, 0 /*Memory Type (always zero) */);
 }
 } // namespace
 
-hsa_status_t register_allocation(void *ptr, size_t size,
-                                 atmi_devtype_t DeviceType) {
-  if (DeviceType == ATMI_DEVTYPE_CPU)
-    return allow_access_to_all_gpu_agents(ptr);
-  else
-    return HSA_STATUS_SUCCESS;
-}
-
 hsa_status_t Runtime::DeviceMalloc(void **ptr, size_t size, int DeviceId) {
   return Runtime::Malloc(ptr, size, DeviceId, ATMI_DEVTYPE_GPU);
 }
 
 hsa_status_t Runtime::HostMalloc(void **ptr, size_t size) {
-  return Runtime::Malloc(ptr, size, 0, ATMI_DEVTYPE_CPU);
+  hsa_status_t Err = Runtime::Malloc(ptr, size, 0, ATMI_DEVTYPE_CPU);
+  if (Err == HSA_STATUS_SUCCESS) {
+    Err = core::allow_access_to_all_gpu_agents(*ptr);
+  }
+  return Err;
 }
 
 hsa_status_t Runtime::Malloc(void **ptr, size_t size, int DeviceId,
                              atmi_devtype_t DeviceType) {
   hsa_amd_memory_pool_t pool =
       get_memory_pool_by_mem_place(DeviceId, DeviceType);
   hsa_status_t err = hsa_amd_memory_pool_allocate(pool, size, 0, ptr);
   DEBUG_PRINT("Malloced [%s %d] %p\n",
               DeviceType == ATMI_DEVTYPE_CPU ? "CPU" : "GPU", DeviceId, *ptr);
-
-  if (err == HSA_STATUS_SUCCESS) {
-    err = register_allocation(*ptr, size, DeviceType);
-  }
-
   return (err == HSA_STATUS_SUCCESS) ? HSA_STATUS_SUCCESS : HSA_STATUS_ERROR;
 }
 
 hsa_status_t Runtime::Memfree(void *ptr) {
   hsa_status_t err = hsa_amd_memory_pool_free(ptr);
   DEBUG_PRINT("Freed %p\n", ptr);
 
   return (err == HSA_STATUS_SUCCESS) ? HSA_STATUS_SUCCESS : HSA_STATUS_ERROR;
 }
 
 } // namespace core

openmp/libomptarget/plugins/amdgpu/impl/internal.h

template <typename T> inline T alignUp(T value, size_t alignment) {
   return alignDown((T)(value + alignment - 1), alignment);
 }
 
 template <typename T> inline T *alignUp(T *value, size_t alignment) {
   return reinterpret_cast<T *>(
       alignDown((intptr_t)(value + alignment - 1), alignment));
 }
 
-hsa_status_t register_allocation(void *addr, size_t size,
-                                 atmi_devtype_t DeviceType);
-
 extern bool atl_is_atmi_initialized();
 
 bool handle_group_signal(hsa_signal_value_t value, void *arg);
 
 hsa_status_t allow_access_to_all_gpu_agents(void *ptr);
 } // namespace core
 
 const char *get_error_string(hsa_status_t err);
 const char *get_atmi_error_string(hsa_status_t err);
 
 #endif // SRC_RUNTIME_INCLUDE_INTERNAL_H_

openmp/libomptarget/plugins/amdgpu/impl/system.cpp

static const std::map<std::string, KernelArgMD::ValueKind> ArgValueKind = {
      KernelArgMD::ValueKind::HiddenMultiGridSyncArg},
     {"hidden_hostcall_buffer", KernelArgMD::ValueKind::HiddenHostcallBuffer},
 };
 
 ATLMachine g_atl_machine;
 
 namespace core {
 
-hsa_status_t allow_access_to_all_gpu_agents(void *ptr) {
-  std::vector<ATLGPUProcessor> &gpu_procs =
-      g_atl_machine.processors<ATLGPUProcessor>();
-  std::vector<hsa_agent_t> agents;
-  for (uint32_t i = 0; i < gpu_procs.size(); i++) {
-    agents.push_back(gpu_procs[i].agent());
-  }
-  return hsa_amd_agents_allow_access(agents.size(), &agents[0], NULL, ptr);
-}
-
 // Implement memory_pool iteration function
 static hsa_status_t get_memory_pool_info(hsa_amd_memory_pool_t memory_pool,
                                          void *data) {
   ATLProcessor *proc = reinterpret_cast<ATLProcessor *>(data);
   hsa_status_t err = HSA_STATUS_SUCCESS;
   // Check if the memory_pool is allowed to allocate, i.e. do not return group
   // memory
   bool alloc_allowed = false;
if (type == HSA_SYMBOL_KIND_KERNEL) {
     if (err != HSA_STATUS_SUCCESS) {
       printf("[%s:%d] %s failed: %s\n", __FILE__, __LINE__,
              "Symbol info size extraction", get_error_string(err));
       return err;
     }
 
     DEBUG_PRINT("Symbol %s = %p (%u bytes)\n", name, (void *)info.addr,
                 info.size);
-    err = register_allocation(reinterpret_cast<void *>(info.addr),
-                              (size_t)info.size, ATMI_DEVTYPE_GPU);
-    if (err != HSA_STATUS_SUCCESS) {
-      return err;
-    }
     SymbolInfoTable[std::string(name)] = info;
     free(name);
   } else {
     DEBUG_PRINT("Symbol is an indirect function\n");
   }
   return HSA_STATUS_SUCCESS;
 }
 

openmp/libomptarget/plugins/amdgpu/src/rtl.cpp

int32_t __tgt_rtl_synchronize(int32_t device_id, __tgt_async_info *AsyncInfo) {
   // Cuda asserts that AsyncInfo->Queue is non-null, but this invariant
   // is not ensured by devices.cpp for amdgcn
   // assert(AsyncInfo->Queue && "AsyncInfo->Queue is nullptr");
   if (AsyncInfo->Queue) {
     finiAsyncInfo(AsyncInfo);
   }
   return OFFLOAD_SUCCESS;
 }
+
+namespace core {
+hsa_status_t allow_access_to_all_gpu_agents(void *ptr) {
+  return hsa_amd_agents_allow_access(DeviceInfo.HSAAgents.size(),
+                                     &DeviceInfo.HSAAgents[0], NULL, ptr);
+}
+
+} // namespace core
 No newline at end of file