-
Notifications
You must be signed in to change notification settings - Fork 160
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
Rename oneMKL Interface to oneMath #602
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are a lot of changes for one PR!. Mostly they look fine with what was discussed in the RFC and spce change. However I have a few question in my review.
The conflicts are mostly due to the recent PR for cuSPARSE support. I will work on fixing them but the other domains can still be reviewed. |
Thanks for doing this huge work! |
Yes I have used |
ok. Thanks. Just wanted to make sure that we don't accidentally miss important information |
@oneapi-src/onemkl-sparse-write I have updated the PR to fix the conflicts with the cuSPARSE PR. It would be useful if you can review the sparse related changes in this PR before the rocSPARSE PR #544! |
I fixed some more recent conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BLAS changes look good to me.
/intelci: run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving on behalf of DFT
/intelci: run |
Thanks for all your work here - there are so many changes, so I know you've spent a lot of effort on this! [1/155] C:\cache\stash\oneapi-compiler\20240719_rls\win\package\compiler\latest\bin\icx.exe -fsycl /nologo -IC:\temp\ec\mkltest\auto-efa034fe-1902-f165-9fcf-a4bf010d0e2d\sources\src -IC:\temp\ec\mkltest\auto-efa034fe-1902-f165-9fcf-a4bf010d0e2d\sources\src\include -IC:\temp\ec\mkltest\auto-efa034fe-1902-f165-9fcf-a4bf010d0e2d\build\bin -IC:\cache\stash\onemkl\regular\2024.2.1\20240722\build__release_win\include /EHsc /DWIN32 /D_WINDOWS /W3 /GR /EHsc -Wno-unused-function -w /O2 /Ob2 /DNDEBUG -iquote C:/temp/ec/mkltest/auto-efa034fe-1902-f165-9fcf-a4bf010d0e2d/sources/include -DMKL_ILP64 -fsycl -QMD -QMT bin\blas\backends\mklcpu\CMakeFiles\onemath_blas_mklcpu_obj.dir\mklcpu_level2.cpp.obj -QMF bin\blas\backends\mklcpu\CMakeFiles\onemath_blas_mklcpu_obj.dir\mklcpu_level2.cpp.obj.d /Fobin\blas\backends\mklcpu\CMakeFiles\onemath_blas_mklcpu_obj.dir\mklcpu_level2.cpp.obj -c C:\temp\ec\mkltest\auto-efa034fe-1902-f165-9fcf-a4bf010d0e2d\sources\src\blas\backends\mklcpu\mklcpu_level2.cpp And somewhat similar for LAPACK (and RNG); e.g., |
Thanks @sknepper I have been able to identify that the issue is due to using In terms of review it is still possible to review most of the changes. I expect the changes needed will only affect CMake files. |
@toxicscum @sknepper I have found a decent solution which should fix the compilation issue on Windows in a2de0e5. |
/intelci: run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @Rbiessy - what an impressive set of changes!
LAPACK looks good - thanks for removing the duplicated headers and documentation - that will streamline things in the future.
The USE_MKLREF in the tests was just a convenience feature to avoid a dependency on Netlib; in the future, if we create standalone testing, the same goal will have been achieved. So that's fine to remove this undocumented macro.
The latest round of precommit testing is still running, but it does indeed look like the Windows build issues were resolved - much thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me, thank you! I have two minor comments.
|
||
You can also join the mailing lists for the [UXL Foundation](https://lists.uxlfoundation.org/g/main/subgroups) to be informed of when meetings are happening and receive the latest information and discussions. | ||
|
||
--- | ||
|
||
## Contributing | ||
|
||
You can contribute to this project and also contribute to [the specification for this project](https://oneapi-spec.uxlfoundation.org/specifications/oneapi/latest/elements/onemkl/source/). Please read the [CONTRIBUTING](CONTRIBUTING.md) page for more information. You can also contact oneMKL developers and maintainers via [UXL Foundation Slack](https://slack-invite.uxlfoundation.org/) using [#onemkl](https://uxlfoundation.slack.com/archives/onemkl) channel. | ||
You can contribute to this project and also contribute to [the specification for this project](https://oneapi-spec.uxlfoundation.org/specifications/oneapi/latest/elements/onemath/source/). Please read the [CONTRIBUTING](CONTRIBUTING.md) page for more information. You can also contact oneMath developers and maintainers via [UXL Foundation Slack](https://slack-invite.uxlfoundation.org/) using [#onemath](https://uxlfoundation.slack.com/archives/onemath) channel. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know who can update the slack channel name? Does it make sense to introduce new channel and close the old one once these changes will be merged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know who can update the slack channel name? Does it make sense to introduce new channel and close the old one once these changes will be merged?
I can rename the channel or create a new one. It might make sense to retain the existing channel though I am open to whatever the consensus is. Just drop me a message when you are ready.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @rodburns! Renaming'd be perfect option if possible.
Co-authored-by: Maria Kraynyuk <[email protected]>
@spencerpatty gentle ping since you have approved the PR for the specification renaming but not the implementation. |
return oneapi::mkl::sparse::trmv(queue, A_view.uplo_view, opA, A_view.diag_view, | ||
host_alpha, backend_handle, x_usm, host_beta, y_usm, | ||
dependencies); | ||
RETHROW_ONEMKL_EXCEPTIONS_RET( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice macro for rethrowing exceptions and returning sycl events :) handy that!
@@ -45,16 +43,16 @@ sycl::event release_spsv_descr(sycl::queue& queue, spsv_descr_t spsv_descr, | |||
return detail::submit_release(queue, spsv_descr, dependencies); | |||
} | |||
|
|||
void check_valid_spsv(const std::string& function_name, oneapi::mkl::transpose opA, | |||
matrix_view A_view, matrix_handle_t A_handle, dense_vector_handle_t x_handle, | |||
void check_valid_spsv(const std::string& function_name, transpose opA, matrix_view A_view, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
much simpler now to distinguish between oneMKL product types and the oneMATH types :) good changes for this interfaces project
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have reviewed all Sparse BLAS related files and I approve of these changes. I love that it is much simpler to distinguish whether we are using oneMATH variables or the backend variables in the files now. This is a good thing!
@Rbiessy I have reviewed the cuSPARSE changes and the general changes for Sparse BLAS and approved them, but in the intel mklcpu/gpu logs it seems like all the SPARSE BLAS tests are skipping. Is that intended? cuSPARSE tests appear to run the first 4 or 5 then skip the rest as well ... is that intended ? |
Description
Related RFC: #564 and related spec PR: uxlfoundation/oneAPI-spec#596
As a reminder the plan is to have this PR and the spec PR approved before we move this repository to https://github.com/uxlfoundation and merge the 2 PRs.
src/include/common_onemkl_conversion.hpp
to convert oneMath common types to oneMKL types. This wasn't needed before as the backends relied on the 2 projects to use the same namespace. The file also provide helper macros to catch oneMKL exceptions and rethrow them as oneMath exceptions.oneapi::mkl
exceptions when committing a descriptor. We discussed this issue by email recently. With the changes needed by the renaming, we now catch and rethrow all the exceptions asoneapi::math
exceptions and I was able to remove a related workaround in the tests (FYI @oneapi-src/onemkl-dft-write). Also see related discussion below: Rename oneMKL Interface to oneMath #602 (comment)onemkl_distribution_conversion.hpp
to convert the RNG types and call the backend's free functions such asoneapi::mkl::rng::generate
,oneapi::mkl::rng::skip_ahead
. @oneapi-src/onemkl-rng-write you may want to carefully reviewsrc/rng/backends/mklgpu/
as I am not familiar with the RNG domain.@oneapi-src/onemkl-*
will be renamed (or duplicated) to@uxlfoundation/onemath-*
in the new repository (@rscohn2 FYI)onemkl
Slack channel will be renamed toonemath
(@rodburns FYI)USE_MKLREF
which was only used in Lapack tests and was not documented. This macro wouldn't work easily with the namespace change anymore (@oneapi-src/onemkl-lapack-write FYI)MKLCPU
andMKLGPU
.oneapi::mkl
inside the Intel backends.cmake/mkl/MKLConfig.cmake
(could probably be removed IMO).include/oneapi/mkl
for deprecated headers and deprecatedoneapi::mkl
C++ namespace.MKLCPU
andMKLGPU
backends use "mkl" in their name. They could be updated to onemkl if needed.Checklist
All Submissions