This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][Libomptarget] Drop dead code related to g_atl_machine
ClosedPublic

Authored by pdhaliwal on Jun 7 2021, 7:25 AM.

Details

Summary

This patch includes some changes which deletes the code accessing
g_atl_machine global. Some accesses related to memory_pools are
still remaining.

Diff Detail

Event Timeline

pdhaliwal created this revision.Jun 7 2021, 7:25 AM
pdhaliwal requested review of this revision.Jun 7 2021, 7:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 7 2021, 7:25 AM
JonChesterfield added inline comments.Jun 7 2021, 7:37 AM
openmp/libomptarget/plugins/amdgpu/impl/atmi_interop_hsa.cpp
27

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

openmp/libomptarget/plugins/amdgpu/src/rtl.cpp
45

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

249

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?

394

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?

pdhaliwal added inline comments.Jun 7 2021, 8:11 AM
openmp/libomptarget/plugins/amdgpu/impl/atmi_interop_hsa.cpp
27

I don't understand. How would it be different from just using int deviceId and verifying it before passing to the methods?

openmp/libomptarget/plugins/amdgpu/src/rtl.cpp
45

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

394

Will work on it.

pdhaliwal updated this revision to Diff 350510.Jun 7 2021, 11:51 PM

Use lambdas to accumulate agents.

pdhaliwal marked an inline comment as done.Jun 11 2021, 4:05 AM

Ping!

openmp/libomptarget/plugins/amdgpu/impl/atmi_interop_hsa.cpp
27

Would move some programming mistake detection from assert time to compile time

openmp/libomptarget/plugins/amdgpu/src/rtl.cpp
529

These appear to be unused

572

I'm not sure this needs AllAgents - shouldn't one of the subsets (iirc it would be HSAAgents) suffice?

pdhaliwal updated this revision to Diff 351418.Jun 11 2021, 5:35 AM

Remove unused code.

pdhaliwal marked an inline comment as done.Jun 11 2021, 5:35 AM
pdhaliwal added inline comments.
openmp/libomptarget/plugins/amdgpu/src/rtl.cpp
572

Normally kernarg is found in CPUAgents, so I included both in case HSAAgents also have kernarg pool.

JonChesterfield accepted this revision.Jun 11 2021, 1:03 PM

LG. My preference would be as commented above, but the vector containing the contents of the other two works too.

openmp/libomptarget/plugins/amdgpu/src/rtl.cpp
572

I think we should go with passing the list we currently see the kernarg segment in, partly to alert us if a future system changes that association, and partly because it's fractionally faster code that way. I think that also drops the AllAgents vector so there's less redundant state lying around.

This revision is now accepted and ready to land.Jun 11 2021, 1:03 PM

Remove AllAgents vector.

This revision was landed with ongoing or failed builds.Jun 14 2021, 10:21 PM
This revision was automatically updated to reflect the committed changes.