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

[Jon Chesterfield via Phabricator via Openmp-commits]

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
        get_error_string(Err));
-    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");`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110513