Skip to content

Conversation

@BenBrock
Copy link
Collaborator

@BenBrock BenBrock commented Mar 20, 2025

Summary:
Short summary of key additions or changes or fixes, including public facing issue
or bug being address if it exists

Details:

  • list of key changes to aide present and future reviewers in understanding what
  • is happening in this PR

Merge Checklist:

  • Passing CI
  • Update documentation or README.md
  • Additional Test/example added (if applicable) and passing
  • At least one reviewer approval
  • (optional) Clang sanitizer scan run and triaged
  • Clang formatter applied (verified as part of passing CI)

@BenBrock BenBrock force-pushed the dev/brock/aocl-ci branch from 6f305ec to 9c67934 Compare March 21, 2025 01:41
@BenBrock
Copy link
Collaborator Author

@mshanthagit @spencerpatty This adds AOCL-Sparse to the CI. The CI is failing, as the AOCL-Sparse tests currently do not compile (since CSC is not handled).

To fix this, we need to implement two new functions: (1) to create an AOCL-Sparse matrix handle for both CSR/CSC matrices (just like this function in oneMKL) and (2) to return whether or not the matrix is considered transposed (CSR is not transposed and CSC is transposed, just like this function in oneMKL).

@BenBrock BenBrock changed the title Try to run CMake with AOCL. Add AOCL-Sparse to CI. Mar 21, 2025
@BenBrock BenBrock changed the title Add AOCL-Sparse to CI. Add AOCL-Sparse to CI Mar 21, 2025
@BenBrock
Copy link
Collaborator Author

@mshanthagit @spencerpatty What are the rules around AOCL descriptors? I assume aoclsparse_create_mat_descr allocates memory, and this later needs to be deallocated with a call to aoclsparse_destroy_mat_descr? (Currently that's not happening—descriptors are created and never destroyed.) Perhaps more importantly, do we need to keep the descriptor alive in between multiply_compute and multiply_fill? Currently, we create a descriptor in multiply_compute and then another in multiply_fill (neither are deallocated). Do we need to store this in the operation state?

@BenBrock BenBrock marked this pull request as ready for review April 17, 2025 18:47
@BenBrock
Copy link
Collaborator Author

@mshanthagit @spencerpatty I'm ready to merge this PR if you guys are happy with it. I'm currently leave the descriptors (mentioned in my previous comment) alone. The current changes are:

  • Add AOCL-Sparse to CI.
  • Add create_matrix_handle and get_transpose functions to handle both CSR and CSC, identically to how it's done in the oneMKL backend.
  • Modify SpMV, SpMM, and SpGEMM to use these functions and accept both CSR/CSC.
  • Removed a few unused declarations.

AOCL-Sparse now runs as part of the CI, and it passes. 🎉

@mshanthagit
Copy link
Contributor

Hi @BenBrock Thank you for handling CSC, will go over the patch soon. You are right w.r.t matrix descriptors, we need to destroy them. I will review and update the patch by EOD tomorrow.

Copy link
Contributor

@mshanthagit mshanthagit left a comment

Choose a reason for hiding this comment

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

I have updated the patch with destroy_mat_descr. Looks good to me.

@BenBrock
Copy link
Collaborator Author

@mshanthagit Sounds like you're happy with the PR—please go ahead and submit a review approving.

@BenBrock BenBrock merged commit 96485f1 into main Apr 24, 2025
11 checks passed
@BenBrock BenBrock deleted the dev/brock/aocl-ci branch April 24, 2025 18:32
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.

4 participants