Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delegate to an impl allocate/deallocate method to allow specifying a SpaceHandle for MemorySpaces #3530

Merged

Conversation

DavidPoliakoff
Copy link
Contributor

This is something I need to enable LogicalMemorySpaces, which are memory spaces that are functionally some other memory space (HostSpace, CudaSpace), but differentiable by tools and templates. When we moved all of the Profiling callbacks into the allocate/deallocate calls (in #3084 ), I lost the ability to "lie" to the Profiling system by handing it a SpaceHandle from the LogicalMemorySpace.

This sketch shows the changes I'd need to get that ability back, I implement it with HostSpace but can extend it to any memory space. Basically we add an impl_allocate/impl_deallocate pair of functions which take their SpaceHandles as arguments. All the other allocate/deallocate functions just call that function, passing a SpaceHandle of the current memory space

I need people to say "this is okay" or "this is not" before I implement this throughout Kokkos

core/src/Kokkos_HostSpace.hpp Outdated Show resolved Hide resolved
core/src/Kokkos_HostSpace.hpp Show resolved Hide resolved
@DavidPoliakoff DavidPoliakoff marked this pull request as ready for review October 28, 2020 22:31
@DavidPoliakoff
Copy link
Contributor Author

This is going through CI, but it's promising. I'm opening it up for thorough review

@DavidPoliakoff DavidPoliakoff changed the title Implementation Sketch: MemorySpace impl_allocate for LogicalMemorySpaces Delegate to an impl allocate/deallocate method to allow specifying a SpaceHandle for MemorySpaces Oct 28, 2020
Copy link
Member

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look fine to me although I must say I don't fully understand how you intend to use these at the end.

@DavidPoliakoff
Copy link
Contributor Author

@dalg24 totally fair. I may be overdoing the "make PR's into smaller chunks" thing, here, in the future I'll make sure things happen in comprehensible chunks

@crtrott crtrott merged commit 0f5fd73 into kokkos:develop Nov 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants