Page MenuHomePhabricator

[AMDGPU][Libomptarget] Refactor device and host pools setup into separate methods
Needs ReviewPublic

Authored by pdhaliwal on Sep 30 2021, 8:43 PM.

Details

Summary

Mostly NFC, this patch simplifies memory pools setup.

Event Timeline

pdhaliwal created this revision.Sep 30 2021, 8:43 PM
pdhaliwal requested review of this revision.Sep 30 2021, 8:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2021, 8:43 PM
pdhaliwal added inline comments.
openmp/libomptarget/plugins/amdgpu/src/rtl.cpp
633

This check was already done in isValidMemoryPool

Is the only difference between the two blocks calling addDeviceMemoryPool vs addHostMemoryPool? If so passing that in as a parameter seems better than copy&paste, which seems to already exist as collectMemoryPools

Calling once on the device agents and once on the host agents seems an improvement, but doing so by copy/paste/specialise collectMemoryPools doesn't seem to be necessary for that.

err = setupDevicePools(HSAAgents);

would become
err = collectMemoryPools(addDeviceMemoryPool, HSAAgents)
or similar

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

This doesn't look right. I think iteration will stop when the passed function returns success. There's a 'continue' in the enum that we could return if you want to keep looking after isValid fails, which I think is necessary to skip over zero size pools

644

same iteration here

Is this patch still current? Looking at current main I think it's been implemented in 05ba9ff6a6d243a