feat(core): add contiguous packing support for Views of arbitrary rank#222
Open
dssgabriel wants to merge 6 commits intokokkos:developfrom
Open
feat(core): add contiguous packing support for Views of arbitrary rank#222dssgabriel wants to merge 6 commits intokokkos:developfrom
dssgabriel wants to merge 6 commits intokokkos:developfrom
Conversation
Signed-off-by: Gabriel Dos Santos <gabriel.dossantos@cea.fr>
Signed-off-by: Gabriel Dos Santos <gabriel.dossantos@cea.fr>
No public docs since these APIs are internal impl details of Kokkos Comm. Helps with linting for developers though. Signed-off-by: Gabriel Dos Santos <gabriel.dossantos@cea.fr>
dssgabriel
added
C-enhancement
Member
cedricchevalier19
left a comment
cedricchevalier19
left a comment
There was a problem hiding this comment.
No blockers but some questions.
src/KokkosComm/mpi/impl/packer.hpp
Outdated
| template <KokkosExecutionSpace ES> | ||
| static auto allocate_packed_for(const ES &space, const std::string &label, const V &src) -> Args { | ||
| auto packed = KokkosComm::Impl::allocate_contiguous_for(space, label, src); | ||
| auto packed = KokkosComm::Impl::allocate_contiguous_for(space, src, label); |
Member
There was a problem hiding this comment.
In Kokkos, label is often the first parameter.
Collaborator
Author
There was a problem hiding this comment.
I am reverting this.
Member
There was a problem hiding this comment.
It means it is before space also.
Collaborator
Author
There was a problem hiding this comment.
Good to know, thanks! The previous interface wasn't even very Kokkos-like then 😅
Collaborator
Author
There was a problem hiding this comment.
I simply reverted it to what it was; I would have had to propagate the change to the backend-specific Packers and to the primitives with non-contiguous support as well, which would have made it an unnecessarily large change.
Signed-off-by: Gabriel Dos Santos <gabriel.dossantos@cea.fr>
Signed-off-by: Gabriel Dos Santos <gabriel.dossantos@cea.fr>
Signed-off-by: Gabriel Dos Santos <gabriel.dossantos@cea.fr>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Refactors the contiguous packing infrastructure to add support for Views of any dimensions using variadic templates, replacing the previous implementation, which limited us to 2D Views at most.
Additional context
Changes
allocate_contiguous_forviastd::index_sequenceand parameter pack expansion refactoring, replacingif constexprbranches for rank 1 and 2resize_contiguous_forwith the same approach for consistencyallocate_contiguous_for(use a default value if omitted). This change does not affect any user-facing interfaces and is just a convenience in preparation for a larger refactoring of non-contiguous handling strategies.allocate_contiguous_forsignatureChecklist