Skip to content

Add CSR merge_path SpMV for OpenMP backend #1810

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

Merged
merged 4 commits into from
May 27, 2025
Merged

Conversation

yhmtsai
Copy link
Member

@yhmtsai yhmtsai commented Mar 19, 2025

This PR picks the changes from #1497 by Luka Stanisic (@stanisic), applies the comments, and makes it available with advanced_spmv

@yhmtsai yhmtsai added the 1:ST:ready-for-review This PR is ready for review label Mar 19, 2025
@yhmtsai yhmtsai requested a review from a team March 19, 2025 11:37
@yhmtsai yhmtsai self-assigned this Mar 19, 2025
@ginkgo-bot ginkgo-bot added mod:openmp This is related to the OpenMP module. type:matrix-format This is related to the Matrix formats labels Mar 19, 2025
@yhmtsai yhmtsai added this to the Ginkgo 1.10.0 milestone Apr 8, 2025
Copy link
Member

@MarcelKoch MarcelKoch left a comment

Choose a reason for hiding this comment

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

LGTM, I have mostly comments left on the documentation.

@@ -42,12 +42,133 @@ namespace omp {
namespace csr {


/**
* Computes the begin offsets into A and B for the specific diagonal
Copy link
Member

Choose a reason for hiding this comment

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

Could you rephrase this? It's unclear what A, and B are, as well as what the specific diagonal is.

* Computes the begin offsets into A and B for the specific diagonal
*
* @param diagonal the diagonal to search
* @param end_row_offsets the ending of row offsets of A
Copy link
Member

Choose a reason for hiding this comment

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

Also unclear. Is this the pointer to the end of the row offsets for A?

const auto nnz = static_cast<IndexType>(a->get_num_stored_elements());
const auto num_threads = static_cast<IndexType>(omp_get_max_threads());
// Merge list A: row end offsets
const IndexType* row_end_offsets = row_ptrs + 1;
Copy link
Member

Choose a reason for hiding this comment

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

is the suffix _offsets necessary? How you define it, I would just call it row_ends or maybe row_end_idxs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think offsets matches the meaning not idxs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I use row_end_ptrs to match the same term we use in CSR

auto row_carry_out_ptr = row_carry_out.get_data();
auto value_carry_out_ptr = value_carry_out.get_data();

// TODO: parallelize with number of cols, too.
Copy link
Member

Choose a reason for hiding this comment

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

If you want to, I think it's pretty simple to move this loop to the innermost loop. You just have to increase the size of value_carry_out to num_threads * c->get_size()[1].

@yhmtsai yhmtsai force-pushed the acmunich_omp_mergespmv branch from 0707e9f to f5f7dfd Compare May 26, 2025 10:06
@yhmtsai yhmtsai force-pushed the acmunich_omp_mergespmv branch from f5f7dfd to 0e09c20 Compare May 26, 2025 10:50
@yhmtsai yhmtsai added 1:ST:ready-to-merge This PR is ready to merge. and removed 1:ST:ready-for-review This PR is ready for review labels May 26, 2025
@yhmtsai yhmtsai merged commit 79f2e0a into develop May 27, 2025
15 of 17 checks passed
@yhmtsai yhmtsai deleted the acmunich_omp_mergespmv branch May 27, 2025 08:01
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1:ST:ready-to-merge This PR is ready to merge. mod:openmp This is related to the OpenMP module. type:matrix-format This is related to the Matrix formats
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants