Skip to content
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

Start migrating to Intel oneMKL #1073

Merged
merged 15 commits into from
Jan 15, 2024
Merged

Start migrating to Intel oneMKL #1073

merged 15 commits into from
Jan 15, 2024

Conversation

albestro
Copy link
Collaborator

@albestro albestro commented Jan 9, 2024

Fixes #878
Close #626

This PR starts with introducing logic for using Intel OneAPI MKL (or oneMKL), which now provides an official CMake Config file. From discussions with @rasolca, for the moment, we opted for keeping also the "legacy" custom module.

  • DLAF_WITH_MKL triggers the new logic using official oneMKL CMake config file;
  • DLAF_WITH_MKL_LEGACY triggers the "legacy" logic which uses our custom CMake module.

note: currently, DLAF_WITH_MKL_LEGACY seems to work fine also for oneAPI MKL installation.

Spack

The spack package has been updated the same way. The logic could be simplified by fully relying on spack (have a look here) and just use the "link-line" logic as any other BLAS/LAPACK library would do. This would require a change, since DLAF_WITH_MKL is used not just for linking purpose but also for implementation details like mkl_set_num_threads. Anyway, this is not needed now and we might evaluate in the future (e.g. when MKL legacy branch will be dropped).

note: intel-parallel-studio is not supported (to my knowledge it has been deprecated too and merged into intel-oneapi)

CI

In the CI nothing has changed yet. It still installs intel-mkl as external package, and then spack uses it. The package should™️ deal correctly with it by using DLAF_WITH_MKL_LEGACY. Switching to intel-oneapi-mkl is outside the objectives of this PR (I will open an issue to track this and decide what to do see #1074).

TODO

(currently legacy mkl is able to detect correctly also oneMKL)
it would be possible to rely on spack for this, but for the moment
we opted for keeping both logic. in the future we might change it.
@albestro albestro added this to the V0.4.0 milestone Jan 9, 2024
@albestro albestro requested a review from rasolca January 9, 2024 17:04
@albestro albestro self-assigned this Jan 9, 2024
@albestro
Copy link
Collaborator Author

albestro commented Jan 9, 2024

cscs-ci run

@albestro albestro marked this pull request as draft January 9, 2024 17:35
- add legacy option for mkl
- make openmp dependent to both mkl and mkl_legacy
- export mkl_legacy option in cmake module
@albestro albestro force-pushed the alby/migrate_to_oneapi branch from 64a1967 to 35287cc Compare January 10, 2024 13:01
@albestro
Copy link
Collaborator Author

cscs-ci run

@codecov-commenter
Copy link

codecov-commenter commented Jan 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6ef1381) 94.06% compared to head (bf3a17f) 94.06%.
Report is 5 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1073   +/-   ##
=======================================
  Coverage   94.06%   94.06%           
=======================================
  Files         148      148           
  Lines        9199     9199           
  Branches     1164     1164           
=======================================
  Hits         8653     8653           
  Misses        323      323           
  Partials      223      223           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@albestro albestro marked this pull request as ready for review January 10, 2024 13:45
@albestro albestro requested review from msimberg and RMeli January 10, 2024 13:46
Copy link
Collaborator

@msimberg msimberg left a comment

Choose a reason for hiding this comment

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

Minor nitpicks. Thanks for doing this!

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
cmake/template/DLAFConfig.cmake.in Outdated Show resolved Hide resolved
spack/packages/dla-future/package.py Outdated Show resolved Hide resolved
src/CMakeLists.txt Show resolved Hide resolved
@rasolca rasolca requested a review from msimberg January 11, 2024 16:43
spack/packages/dla-future/package.py Outdated Show resolved Hide resolved
spack/packages/dla-future/package.py Outdated Show resolved Hide resolved
spack/packages/dla-future/package.py Outdated Show resolved Hide resolved
they act as a guard to not end up in the official package
Copy link
Collaborator

@msimberg msimberg left a comment

Choose a reason for hiding this comment

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

Approved modulo @rasolca's comments and one space too many.

CMakeLists.txt Outdated Show resolved Hide resolved
@albestro
Copy link
Collaborator Author

cscs-ci run

@albestro albestro requested a review from rasolca January 12, 2024 11:21
@rasolca rasolca mentioned this pull request Jan 12, 2024
2 tasks
older versions are considered not compatible
@albestro albestro mentioned this pull request Jan 15, 2024
@albestro albestro requested a review from msimberg January 15, 2024 14:44
@rasolca rasolca merged commit 65eda2b into master Jan 15, 2024
3 checks passed
@rasolca rasolca deleted the alby/migrate_to_oneapi branch January 15, 2024 16:40
github-actions bot pushed a commit that referenced this pull request Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Support oneMKL FindMKL improvements
4 participants