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

[Pushpinder Singh via Phabricator via Openmp-commits]

pdhaliwal added inline comments.


================
Comment at: openmp/libomptarget/plugins/amdgpu/impl/atmi_interop_hsa.cpp:27
   if (!symbol || !var_addr || !var_size)
     return HSA_STATUS_ERROR;
 
----------------
JonChesterfield wrote:
> 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
I don't understand. How would it be different from just using int deviceId and verifying it before passing to the methods?


================
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"
----------------
JonChesterfield wrote:
> 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
I haven't checked that. I just compiled and ran libomptarget tests (all of which are fine). But, there is some code which is present in corresponding .cpps in lib/Support/. 


================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:394
+  // CPUs
+  std::vector<hsa_agent_t> CPUAgents;
+
----------------
JonChesterfield wrote:
> 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?
Will work on it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103813