Page MenuHomePhabricator

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

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



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

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


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.


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


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


as above


better to preserve the specific error


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.