Enable bulk transfers of sparse arrays #25243
Merged
Conversation
This file contains 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
This uses a new doi routine in a specific place to enable localization of sparse domains when doing my new optimization. Engin has told me the more principled way to do it, but I wanted to get this into a commit in a standalone branch first before refactoring. Interestingly, my test here seems to show that we don't correctly store 'definedConst' for sparse subdomains at present; though it does work for const arrays, which is the case I actually care about at present. So future work here to generalize this. --- Signed-off-by: Brad Chamberlain <bradcray@users.noreply.github.com>
--- Signed-off-by: Brad Chamberlain <bradcray@users.noreply.github.com>
--- Signed-off-by: Brad Chamberlain <bradcray@users.noreply.github.com>
e-kayrakli
approved these changes
Jun 13, 2024
(though he technically didn't request it) --- Signed-off-by: Brad Chamberlain <bradcray@users.noreply.github.com>
…f domain This adds a test Engin requested in his review to make sure that the bulk assignment logic between sparse arrays is comparing domain index sets precisely, and not relying on identity. Though other tests (including my not-yet-checked in sparse matrix-matrix multiplication) arguably do this by virtue of the domain localization optimization, this does it more expicitly by creating two domains with different identities but the same indices, and assigning arrays back and forth between them, modifying the arrays slightly along the way. For a multilocale run, I'm creating the arrays on different locales, but the domains on the same locale, somewhat arbitrarily; this arguably covers a slightly different case than the existing tests of the localization optimization, since in that case, the domains and arrays are on different locales. --- Signed-off-by: Brad Chamberlain <bradcray@users.noreply.github.com>
Forgot that this test existed in the second-to-previous commit --- Signed-off-by: Brad Chamberlain <bradcray@users.noreply.github.com>
e-kayrakli
approved these changes
Jun 13, 2024
bradcray
added a commit
that referenced
this pull request
Jun 14, 2024
This is a code I've been writing, looking at sparse matrix-matrix multiplication in Chapel to start to get more experience with our sparse features, look for gaps, check performance, etc. So far, the effort has resulted in improvements in the forms of PRs #25152, #25244, #24391, and #25243. This is a work-in-progress that almost certainly needs additional improvements in terms of performance, generality, ergonomics, but it's a good start and in a state to start exercising some of the aforementioned PRs. See the README for additional details.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR adds the
doiBulkTransfer[To|From]Known()routines to the two sparse layouts we currently support in order to enable sparse arrays to be initialized using patterns likevar copy = mySparseArray;where, previously, this resulted in a complaint about being unable to zip sparse arrays due to falling into the most general implementation of array-array transfers.Here, I'm adding some tests of this capability using the domain localization PR that I merged today (#24391) to prove that sparse arrays can be localized and accessed without incurring communication. I've turned on comm counts to track how many transfers are required, where I expect we could significantly reduce this over time by adding support for bulk transfer of the sparse domains themselves in the future (?).
Interestingly, one of the two cases that ought to cause my domain localization optimization to fire doesn't currently, suggesting that we don't set up definedConst properly for sparse subdomains; happily, the case I care about (const array, non-const domain) does work properly, so here I've checked in the tests to show that the other case doesn't work—another area for future improvement and a pre-existing issue not related to this effort.