[AMDGPU][OpenMP] Add memory pool size check to isValidMemoryPool

[pdhaliwal]

Summary

Keeping all the checks in one place for future simplification.

[pdhaliwal]

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

331

This method was moved above addKernArgPool because I needed it to be visible for the latter. So diff here is bit of messed up.

[JonChesterfield]

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

332

Looks pre-existing but we can't exit here, need to pass the error up. I'll apply the patch and take a look in a better diff tool

[pdhaliwal]

• rebase after rG9d0eb440ff40
• change return type of hsa_status_t

[JonChesterfield]

Couple of minor points around loss of specific error information and we might want another DP instance but otherwise looks good. std::pair<hsa_status_t, bool> to hsa_status_t is an improvement imo, we don't need particularly fine grained understanding of what went wrong when the plan on failure is to log something and keep going.

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

315

return Err here, it might be more informative than HSA_STATUS_ERROR when printed to a log

318–326

i'd probably parenthesize this, (AllocAllowed && Size > 0)

323

as above

334

better to preserve the specific error

361–363

somewhat inconsistent hat we aren't printing here. Maybe we want something like DP("Skipping memory pool: %s\n");

[pdhaliwal]

• review comments

[JonChesterfield]

Thanks!

[JonChesterfield]

Unforeseen consequence arises here. Since isValidMemoryPool now returns false for zero size pools, a zero size pool will make addKernArgPool return an error, and that makes findKernargPool return an error, which aborts construction of the plugin. I have a system locally with two non-zero kernarg pools and to zero size ones which runs into this.

Going to patch by letting FindKernargPool keep looking on an error reported by the lower levels since a zero size pool is broadly equivalent to one we can't use for another reason.

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

 
 uint16_t create_header() {
   uint16_t header = HSA_PACKET_TYPE_KERNEL_DISPATCH << HSA_PACKET_HEADER_TYPE;
   header |= HSA_FENCE_SCOPE_SYSTEM << HSA_PACKET_HEADER_ACQUIRE_FENCE_SCOPE;
   header |= HSA_FENCE_SCOPE_SYSTEM << HSA_PACKET_HEADER_RELEASE_FENCE_SCOPE;
   return header;
 }
 
-std::pair<hsa_status_t, bool>
-isValidMemoryPool(hsa_amd_memory_pool_t MemoryPool) {
+hsa_status_t isValidMemoryPool(hsa_amd_memory_pool_t MemoryPool) {
   bool AllocAllowed = false;
   hsa_status_t Err = hsa_amd_memory_pool_get_info(
       MemoryPool, HSA_AMD_MEMORY_POOL_INFO_RUNTIME_ALLOC_ALLOWED,
       &AllocAllowed);
   if (Err != HSA_STATUS_SUCCESS) {
     DP("Alloc allowed in memory pool check failed: %s\n",
        get_error_string(Err));
-    return {Err, false};
+    return Err;
+  }
+
+  size_t Size = 0;
+  Err = hsa_amd_memory_pool_get_info(MemoryPool, HSA_AMD_MEMORY_POOL_INFO_SIZE,
+                                     &Size);
+  if (Err != HSA_STATUS_SUCCESS) {
+    DP("Get memory pool size failed: %s\n", get_error_string(Err));
+    return Err;
   }
 
-  return {HSA_STATUS_SUCCESS, AllocAllowed};
+  return (AllocAllowed && Size > 0) ? HSA_STATUS_SUCCESS : HSA_STATUS_ERROR;
 }
 
 hsa_status_t addKernArgPool(hsa_amd_memory_pool_t MemoryPool, void *Data) {
   std::vector<hsa_amd_memory_pool_t> *Result =
       static_cast<std::vector<hsa_amd_memory_pool_t> *>(Data);
-  bool AllocAllowed = false;
-  hsa_status_t err = hsa_amd_memory_pool_get_info(
-      MemoryPool, HSA_AMD_MEMORY_POOL_INFO_RUNTIME_ALLOC_ALLOWED,
-      &AllocAllowed);
-  if (err != HSA_STATUS_SUCCESS) {
-    DP("Alloc allowed in memory pool check failed: %s\n",
-       get_error_string(err));
-    return err;
-  }
 
-  if (!AllocAllowed) {
-    // nothing needs to be done here.
-    return HSA_STATUS_SUCCESS;
+  hsa_status_t err;
+  if ((err = isValidMemoryPool(MemoryPool)) != HSA_STATUS_SUCCESS) {
+    return err;
   }
 
   uint32_t GlobalFlags = 0;
   err = hsa_amd_memory_pool_get_info(
       MemoryPool, HSA_AMD_MEMORY_POOL_INFO_GLOBAL_FLAGS, &GlobalFlags);
   if (err != HSA_STATUS_SUCCESS) {
     DP("Get memory pool info failed: %s\n", get_error_string(err));
     return err;
   }
 
-  size_t size = 0;
-  err = hsa_amd_memory_pool_get_info(MemoryPool, HSA_AMD_MEMORY_POOL_INFO_SIZE,
-                                     &size);
-  if (err != HSA_STATUS_SUCCESS) {
-    DP("Get memory pool size failed: %s\n", get_error_string(err));
-    return err;
-  }
-
   if ((GlobalFlags & HSA_AMD_MEMORY_POOL_GLOBAL_FLAG_FINE_GRAINED) &&
-      (GlobalFlags & HSA_AMD_MEMORY_POOL_GLOBAL_FLAG_KERNARG_INIT) &&
-      size > 0) {
+      (GlobalFlags & HSA_AMD_MEMORY_POOL_GLOBAL_FLAG_KERNARG_INIT)) {
     Result->push_back(MemoryPool);
   }
 
   return HSA_STATUS_SUCCESS;
 }
 
 template <typename AccumulatorFunc>
 hsa_status_t collectMemoryPools(const std::vector<hsa_agent_t> &Agents,
                                 AccumulatorFunc Func) {
   for (int DeviceId = 0; DeviceId < Agents.size(); DeviceId++) {
     hsa_status_t Err = hsa::amd_agent_iterate_memory_pools(
         Agents[DeviceId], [&](hsa_amd_memory_pool_t MemoryPool) {
           hsa_status_t Err;
-          bool Valid = false;
-          std::tie(Err, Valid) = isValidMemoryPool(MemoryPool);
-          if (Err != HSA_STATUS_SUCCESS) {
-            return Err;
-          }
-          if (Valid)
+          if ((Err = isValidMemoryPool(MemoryPool)) != HSA_STATUS_SUCCESS) {
+            DP("Skipping memory pool: %s\n", get_error_string(Err));
+          } else
             Func(MemoryPool, DeviceId);
           return HSA_STATUS_SUCCESS;
         });
 
     if (Err != HSA_STATUS_SUCCESS) {
       DP("[%s:%d] %s failed: %s\n", __FILE__, __LINE__,
          "Iterate all memory pools", get_error_string(Err));
       return Err;