[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:594
+    if (Err != HSA_STATUS_SUCCESS) {
+      fprintf(stderr, "Get memory pool info failed: %s\n",
+              get_error_string(Err));
----------------
Can probably return the error without printing here, the caller will know it was addDeviceMemoryPool that failed anyway. Medium term it's probably worth consolidating all the misc print statements into rtl.cpp and routing them through the DP() macro


================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:637
+
+  void setupMemoryPools() {
+    hsa_status_t Err;
----------------
This looks like it should be returning a hsa_status_t, the caller may want to disable offloading if the memory pool query fails


================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:372
+  }
+
+  return {HSA_STATUS_SUCCESS, AllocAllowed};
----------------
pdhaliwal wrote:
> 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. 
that's probably fair, OK


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104695