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

Jon Chesterfield via Phabricator via Openmp-commits openmp-commits at lists.llvm.org
Fri Oct 1 03:36:41 PDT 2021

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

  rG LLVM Github Monorepo



More information about the Openmp-commits mailing list