Page MenuHomePhabricator

[libomptarget][amdgpu] Destruct HSA queues
ClosedPublic

Authored by JonChesterfield on Sep 9 2021, 7:53 AM.

Details

Summary

Store queues in unique_ptr so they are destroyed when the global DeviceInfo is. Currently they leak which raises an assert in debug builds of hsa.

Diff Detail

Event Timeline

JonChesterfield created this revision.Sep 9 2021, 7:53 AM
JonChesterfield requested review of this revision.Sep 9 2021, 7:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2021, 7:54 AM
JonChesterfield added inline comments.Sep 9 2021, 8:00 AM
openmp/libomptarget/plugins/amdgpu/src/rtl.cpp
856–857

Storing (most) state in a global variable that closes hsa in the destructor make it difficult to use raii classes to manage hsa resources.

We could change to using multiple calls to hsa_init/destroy, since it's internally reference counted, and that would give us the last one to destroy closes hsa.

Better is probably to nest the lifetime - would like hsa_init to occur before any other objects are constructed and hsa_shut_down after they've all been torn down. Will think about how best to represent that (separate to this patch)

JonChesterfield planned changes to this revision.Sep 9 2021, 8:34 AM

D109512 rearranges rtl.cpp so that hsa is reliably constructed before other member variables in the big global object. If we land that, this signal pool can be refactored to free the signals in the destructor again. Still some care needed to ensure it doesn't try to create any without checking hsa is available, i.e. don't prepopulate the pool in the constructor, but at least cleanup can be implicit.

Likewise could put queue in a unique_ptr or similar to reliably cleanup.

  • roll back most of the patch
  • put queues in unique_ptr
JonChesterfield retitled this revision from [libomptarget][amdgpu] Clean up destruction of hsa queue, signals to [libomptarget][amdgpu] Destruct HSA queues on exit.Sep 9 2021, 11:58 AM
JonChesterfield edited the summary of this revision. (Show Details)
JonChesterfield retitled this revision from [libomptarget][amdgpu] Destruct HSA queues on exit to [libomptarget][amdgpu] Destruct HSA queues.
pdhaliwal accepted this revision.Sep 20 2021, 5:42 AM

Looks good.

This revision is now accepted and ready to land.Sep 20 2021, 5:42 AM
This revision was landed with ongoing or failed builds.Sep 26 2021, 7:34 AM
This revision was automatically updated to reflect the committed changes.