Page MenuHomePhabricator

[AMDGPU][OpenMP] Add memory pool size check to isValidMemoryPool
ClosedPublic

Authored by pdhaliwal on Sep 26 2021, 7:50 PM.

Details

Summary

Keeping all the checks in one place for future simplification.

Diff Detail

Event Timeline

pdhaliwal created this revision.Sep 26 2021, 7:50 PM
pdhaliwal requested review of this revision.Sep 26 2021, 7:50 PM
Herald added a project: Restricted Project. · View Herald Transcript
pdhaliwal added inline comments.Sep 26 2021, 7:51 PM
openmp/libomptarget/plugins/amdgpu/src/rtl.cpp
331

This method was moved above addKernArgPool because I needed it to be visible for the latter. So diff here is bit of messed up.

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

Looks pre-existing but we can't exit here, need to pass the error up. I'll apply the patch and take a look in a better diff tool

pdhaliwal updated this revision to Diff 375182.Sep 27 2021, 2:52 AM

Couple of minor points around loss of specific error information and we might want another DP instance but otherwise looks good. std::pair<hsa_status_t, bool> to hsa_status_t is an improvement imo, we don't need particularly fine grained understanding of what went wrong when the plan on failure is to log something and keep going.

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

return Err here, it might be more informative than HSA_STATUS_ERROR when printed to a log

318–326

i'd probably parenthesize this, (AllocAllowed && Size > 0)

323

as above

334

better to preserve the specific error

361–363

somewhat inconsistent hat we aren't printing here. Maybe we want something like DP("Skipping memory pool: %s\n");

pdhaliwal updated this revision to Diff 375196.Sep 27 2021, 4:19 AM
pdhaliwal marked 5 inline comments as done.
  • review comments
This revision is now accepted and ready to land.Sep 27 2021, 4:47 AM
This revision was landed with ongoing or failed builds.Sep 27 2021, 5:29 AM
This revision was automatically updated to reflect the committed changes.

Unforeseen consequence arises here. Since isValidMemoryPool now returns false for zero size pools, a zero size pool will make addKernArgPool return an error, and that makes findKernargPool return an error, which aborts construction of the plugin. I have a system locally with two non-zero kernarg pools and to zero size ones which runs into this.

Going to patch by letting FindKernargPool keep looking on an error reported by the lower levels since a zero size pool is broadly equivalent to one we can't use for another reason.