[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.


================
Comment at: openmp/libomptarget/plugins/amdgpu/impl/atmi_interop_hsa.cpp:27
   if (!symbol || !var_addr || !var_size)
     return HSA_STATUS_ERROR;
 
----------------
pdhaliwal wrote:
> 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?
Would move some programming mistake detection from assert time to compile time


================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:528
 
+  void addGPUAgent(hsa_agent_t Agent) { HSAAgents.push_back(Agent); }
+
----------------
These appear to be unused


================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:573
     }
-
-    std::tie(err, KernArgPool) = core::FindKernargPool(HSAAgents);
+    std::tie(err, KernArgPool) = core::FindKernargPool(AllAgents);
     if (err != HSA_STATUS_SUCCESS) {
----------------
I'm not sure this needs AllAgents - shouldn't one of the subsets (iirc it would be HSAAgents) suffice?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103813