Page MenuHomePhabricator

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

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



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.Mon, Jun 7, 7:25 AM
pdhaliwal requested review of this revision.Mon, Jun 7, 7:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Jun 7, 7:25 AM
JonChesterfield added inline comments.Mon, Jun 7, 7:37 AM

int DeviceID is somewhat error prone, I wonder if it's worth changing to something like

struct DeviceIDType
 // operations
  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


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


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?


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.Mon, Jun 7, 8:11 AM

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


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


Will work on it.

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

Use lambdas to accumulate agents.

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



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


These appear to be unused


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.Fri, Jun 11, 5:35 AM

Remove unused code.

pdhaliwal marked an inline comment as done.Fri, Jun 11, 5:35 AM
pdhaliwal added inline comments.

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

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

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


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.Fri, Jun 11, 1:03 PM

Remove AllAgents vector.

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