Skip to content

[ONEMKL_SYCL] Rearrange MKL backend and add logging#22

Merged
BenBrock merged 8 commits intomainfrom
dev/spatty/rearrange_mkl
Feb 25, 2025
Merged

[ONEMKL_SYCL] Rearrange MKL backend and add logging#22
BenBrock merged 8 commits intomainfrom
dev/spatty/rearrange_mkl

Conversation

@spencerpatty
Copy link
Copy Markdown
Contributor

@spencerpatty spencerpatty commented Feb 21, 2025

Changes:

  1. Add offset_t = index_t to distinguish between types for reference/armpl/mkl
  2. Improve comments in Examples
  3. break MKL's vendor multiply_impl.hpp into 3 files: spmv_impl.hpp, spmm_impl.hpp and spgemm_impl.hpp
  4. Change MKL index_t to std::int32_t and fix spgemm_impl.hpp to work with nnz being fixed as int64_t
  5. Add include/spblas/backend/log.hpp as logging system for debugging implementations and add log_trace("") to all existing multiply* functions in reference/armpl/onemkl_sycl

Testing:

Builds locally with Intel oneAPI compiler and oneMKL from oneAPI 2025.0 release.

@spencerpatty spencerpatty added the enhancement New feature or request label Feb 21, 2025
@spencerpatty spencerpatty self-assigned this Feb 21, 2025
Comment thread examples/simple_spmv.cpp Outdated
@spencerpatty spencerpatty marked this pull request as draft February 21, 2025 05:17
…work with nnz as int64_t

  * [Examples] Add comments to simple examples
  * [LOG][ONEMKL_SYCL] Add log system to repository and shift mkl->onemkl_sycl with some separations out into individual files for implementations
@spencerpatty spencerpatty force-pushed the dev/spatty/rearrange_mkl branch from fd31457 to cecb695 Compare February 24, 2025 17:58
@spencerpatty spencerpatty marked this pull request as ready for review February 24, 2025 18:22
@spencerpatty spencerpatty changed the title Rearrange MKL backend [ONEMKL_SYCL] Rearrange MKL backend and add logging Feb 24, 2025
Copy link
Copy Markdown
Collaborator

@BenBrock BenBrock left a comment

Choose a reason for hiding this comment

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

Looks mostly good! As you mentioned over chat, the IESPBLAS stuff needs to be removed from this PR. There's one necessary change and two requested changes:

And nice-to-haves:

  • I'd prefer T instead of DATA_TYPE in examples. If you strongly desire to have a multi-character type alias, please use value_type to be consistent with the rest of the codebase. (And index_type and offset_type for indices and offsets.)
  • I'd also prefer to stick to fmt in the examples instead of using iostream, given that we're already including it. Should be an easy change.

Comment thread examples/simple_spmv.cpp Outdated
Comment thread examples/simple_spgemm.cpp Outdated
Comment thread CMakeLists.txt
@spencerpatty
Copy link
Copy Markdown
Contributor Author

@BenBrock I have made all suggested changes, and it is ready for check again.

@BenBrock BenBrock self-requested a review February 25, 2025 00:42
Copy link
Copy Markdown
Collaborator

@BenBrock BenBrock left a comment

Choose a reason for hiding this comment

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

Looks good. I just moved log.hpp from backend to detail and added a missing header that was preventing compilation on OS X.

We eventually need to go back to log.hpp and prevent it from putting anything in the global namespace, as that's a no-no for C++. Fine for now though.

@BenBrock BenBrock merged commit b6b5abe into main Feb 25, 2025
@BenBrock BenBrock deleted the dev/spatty/rearrange_mkl branch February 25, 2025 01:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants