[Openmp-commits] [PATCH] D110513: [AMDGPU][OpenMP] Add memory pool size check to isValidMemoryPool

Jon Chesterfield via Phabricator via Openmp-commits openmp-commits at lists.llvm.org
Mon Sep 27 03:01:45 PDT 2021

JonChesterfield added a comment.

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.

Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:315
-    return {Err, false};
+    return HSA_STATUS_ERROR;
+  }
return Err here, it might be more informative than HSA_STATUS_ERROR when printed to a log

Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:323
+    DP("Get memory pool size failed: %s\n", get_error_string(Err));
+    return HSA_STATUS_ERROR;
as above

Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:326
-  return {HSA_STATUS_SUCCESS, AllocAllowed};
+  return AllocAllowed && Size > 0 ? HSA_STATUS_SUCCESS : HSA_STATUS_ERROR;
i'd probably parenthesize this, (AllocAllowed && Size > 0)

Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:334
+  if (isValidMemoryPool(MemoryPool) != HSA_STATUS_SUCCESS) {
+    return HSA_STATUS_ERROR;
better to preserve the specific error

Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:359
         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 (isValidMemoryPool(MemoryPool) == HSA_STATUS_SUCCESS)
             Func(MemoryPool, DeviceId);
somewhat inconsistent hat we aren't printing here. Maybe we want something like `DP("Skipping memory pool: %s\n");`

  rG LLVM Github Monorepo



More information about the Openmp-commits mailing list