Andrew Stubbs <[email protected]> writes: > I can't see any great problems with this. Obviously it would be nice > if the "ephemeral memories" and kernargs caches shared the same > implementation, instead of being almost-but-not-quite the same, but > that's more work.
Yeah, I initially tried to fit the kernargs into the ephemeral memory code, but it'd have been a bigger change because the ephemeral memory allocation code depends on the kernel arguments already having been partially populated, or something of such nature (it's been a while since I looked at it so I've forgotten the details - sorry). The data structure here is probably the same as the ephemeral memories one, except with a different size-checking mechanism, but that can probably be remedied. > Again, it's not specifically this patch, but did you consider finding > a way to avoid the "shadow" allocation? That's been there since we > first copied the code from some HSA example somewhere, but I don't > know why we need it. We don't, really. It's very superfluous, I could allot some time to removing it and cleaning that up, but it is a larger change than this patch would've needed. It could at least be turned into an automatic storage variable, really, since its lifetime is entirely constrained to run_kernel. Maybe GCC already does that transform? ;) > I'm happy with this patch for mainline, as long as it's properly > tested with kernels that have a lot of mappings. I've ran it through OpenMP VV, and omptests, neither of which regressed. I'm not aware of any especially good real-world testcases that I could use, so I'm open to suggestions on that front. I could also just synthesize some trivial test with, like, 80 mappings to make sure that works. That should hit the concerning test case and could fit in the testsuite, but really it depends on what size of stack people run their OpenMP/ACC programs with and how deep their call stacks are, so I'm not sure it'd be very representative. -- Arsen Arsenović
signature.asc
Description: PGP signature
