[Openmp-commits] [PATCH] D104695: [AMDGPU][Libomptarget] Collect allocatable memory pools using HSA

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added inline comments.


================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:372
+  }
+
+  return {HSA_STATUS_SUCCESS, AllocAllowed};
----------------
This looks like it can return a hsa_status_t directly

hsa_status_t Err = get_info();
if (Err != HSA_SUCCESS) return Err;
if (!AllocAllowed) return HSA_ERROR; // or a more specific enum
return HSA_SUCCESS;


================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:383
+          hsa_status_t Err;
+          bool Valid = false;
+          std::tie(Err, Valid) = isValidMemoryPool(MemoryPool);
----------------
control flow here is simpler if the bool Valid is dropped by isValidMemoryPool returning a hsa_status_t


================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:477
 
+  // fine and coarse-grained memory pools per CPU
+  std::vector<hsa_amd_memory_pool_t> HostFineGrainedMemoryPools;
----------------
Is this per-cpu-agent, or per-cpu-agent-with-nonzero-memory-pool? I wonder if we'd be better off with a scalar `hsa_amd_memory_pool_t HostFineGrainedMemoryPool;` for now


================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:723
+    HostCoarseGrainedMemoryPools.resize(NumCPUs);
+    HostFineGrainedMemoryPools.resize(NumCPUs);
+    DeviceCoarseGrainedMemoryPools.resize(NumberOfDevices);
----------------
Ah, it includes the agents with zero available memory. Not sure distinguishing between CPU agents is helpful at present.

Also, doesn't this plus the (size > 0) check mean that a threadripper would have a vector of length 4 of the host pools, but only index 0 and 1 would be populated, and not correspond to the iteration order of CPU agents?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104695