[Openmp-commits] [PATCH] D109875: [AMDGPU][Libomptarget] Refactor device/host memory pools setup

[Pushpinder Singh via Phabricator via Openmp-commits]

pdhaliwal marked an inline comment as done.
pdhaliwal added inline comments.


================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:636
 
     return HSA_STATUS_SUCCESS;
   }
----------------
JonChesterfield wrote:
> pdhaliwal wrote:
> > JonChesterfield wrote:
> > > 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
> > 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. 
> 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?
> 
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109875/new/

https://reviews.llvm.org/D109875