[Openmp-commits] [PATCH] D110902: [AMDGPU][Libomptarget] Refactor device and host pools setup into separate methods

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added a comment.

Is the only difference between the two blocks calling addDeviceMemoryPool vs addHostMemoryPool? If so passing that in as a parameter seems better than copy&paste, which seems to already exist as collectMemoryPools

Calling once on the device agents and once on the host agents seems an improvement, but doing so by copy/paste/specialise collectMemoryPools doesn't seem to be necessary for that.

> err = setupDevicePools(HSAAgents);

would become
err = collectMemoryPools(addDeviceMemoryPool, HSAAgents)
or similar



================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:624
+                 get_error_string(ValidStatus));
+              return HSA_STATUS_SUCCESS;
+            }
----------------
This doesn't look right. I think iteration will stop when the passed function returns success. There's a 'continue' in the enum that we could return if you want to keep looking after isValid fails, which I think is necessary to skip over zero size pools


================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:644
+          DP("isValidMemoryPool returned: %s\n", get_error_string(Err));
+          return HSA_STATUS_SUCCESS;
+        }
----------------
same iteration here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110902