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

[Pushpinder Singh via Phabricator via Openmp-commits]

pdhaliwal added inline comments.


================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:372
+  }
+
+  return {HSA_STATUS_SUCCESS, AllocAllowed};
----------------
JonChesterfield wrote:
> 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;
I was thinking that it would be a good idea to stop early if there any kind of error in hsa API. 


================
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;
----------------
JonChesterfield wrote:
> 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
It is per-cpu-agent. I haven't added any zero size check following the existing logic. We always use only one memory pool so using single scalar should not cause any issues.


================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:723
+    HostCoarseGrainedMemoryPools.resize(NumCPUs);
+    HostFineGrainedMemoryPools.resize(NumCPUs);
+    DeviceCoarseGrainedMemoryPools.resize(NumberOfDevices);
----------------
JonChesterfield wrote:
> 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?
There is no size >0 check so it would populate all the slots.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104695