Page MenuHomePhabricator

[AMDGPU][Libomptarget] Refactor device/host memory pools setup
AbandonedPublic

Authored by pdhaliwal on Sep 16 2021, 4:03 AM.

Details

Summary
  • Simplifies logic for device memory pool lookup
  • Merges kernarg host pool lookup into setupHostMemoryPools
  • Removes redundant code.

Diff Detail

Event Timeline

pdhaliwal created this revision.Sep 16 2021, 4:03 AM
pdhaliwal requested review of this revision.Sep 16 2021, 4:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2021, 4:03 AM
pdhaliwal updated this revision to Diff 372895.Sep 16 2021, 4:11 AM

run clang-format

We've lost some error checking in the refactor but otherwise it looks like an improvement

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

This is going to return success if it finds a pool for either kernarg or fine grain. I think we should only be claiming success if we've found (at least?) one for each. We could handle the control flow for that by initialising both pools to {0} and checking neither is {0} before returning, or by keeping track of whether both have been initialized

pdhaliwal updated this revision to Diff 373169.Sep 17 2021, 1:51 AM

Check if both values are set properly.

pdhaliwal marked an inline comment as done.Sep 17 2021, 1:52 AM
pdhaliwal added inline comments.
openmp/libomptarget/plugins/amdgpu/src/rtl.cpp
651

Thanks for pointing this out. It actually found an issue when only one memory pool is available and it is both kernarg and fine grained.

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

That's interesting. If we have only one pool for both then we must use it for both.

What if we have one pool for both and one for only fine grain? Presumably we should then use different ones as they may be backed by different allocators.

Iteration order is probably difficult to predict. Maybe first kernarg pool found is used, and first non-kernarg pool is used for fine grain, but before erroring out we also use the kernarg pool for fine grain iff it hasn't been assigned yet?

pdhaliwal marked an inline comment as done.Sep 19 2021, 8:50 PM
pdhaliwal added inline comments.
openmp/libomptarget/plugins/amdgpu/src/rtl.cpp
651

That makes sense. Using "if-else" instead of "only if" can make sure that in each iteration only one of the kernarg and fine grained pool is assigned. After the for loop, kernarg should be always assigned (I guess?). If FineGrainedMemoryPool is not set then kernarg pool can be reused. Issues/Suggestions for this approach?

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

Sounds good to me

pdhaliwal updated this revision to Diff 373558.EditedSep 20 2021, 5:18 AM

update logic for fine-grained/kernarg pools

JonChesterfield accepted this revision.Sep 21 2021, 9:28 AM

LG, thanks!

This revision is now accepted and ready to land.Sep 21 2021, 9:28 AM
openmp/libomptarget/plugins/amdgpu/src/rtl.cpp
18

functional may now be unused (think it was only here for placeholders)

pdhaliwal abandoned this revision.Sep 30 2021, 8:45 PM

Abandoning this in favor of D110902 due to dependency issues.