[Openmp-commits] [PATCH] D103813: [AMDGPU][Libomptarget] Drop dead code related to g_atl_machine

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added inline comments.
Herald added a reviewer: jdoerfert.
Herald added a subscriber: sstefan1.


================
Comment at: openmp/libomptarget/plugins/amdgpu/impl/atmi_interop_hsa.cpp:27
   if (!symbol || !var_addr || !var_size)
     return HSA_STATUS_ERROR;
 
----------------
int DeviceID is somewhat error prone, I wonder if it's worth changing to something like
```struct DeviceIDType
{
 // operations
private:
  uint8_t data;
};```
where there is a check at the entry point that it is positive and within the number of available GPUs, and then we pass that pre-validated type instance around


================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:45
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/Frontend/OpenMP/OMPGridValues.h"
----------------
Are these header-only? They're more likely to cause problems if there is a LLVM library that needs to be linked in to use them


================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:249
 
-static std::vector<hsa_agent_t> find_gpu_agents() {
-  std::vector<hsa_agent_t> res;
+typedef std::pair<hsa_device_type_t, hsa_agent_t> DeviceTypeAgentPair;
+
----------------
Not totally sure about this factoring. What do you think of populating one vector for GPU agents and a second one for CPU agents, instead of tagging each element with a dynamic type?


================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:394
+  // CPUs
+  std::vector<hsa_agent_t> CPUAgents;
+
----------------
This seems to answer the above comment. Is the llvm::Optional<std::vector<DeviceTypeAgentPair>> type replaceable by passing a stateful lambda to FindAgents(), with a reference to each vector?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103813