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

[LAPACK][mkl?pu] Align oneMKL backend with const correctness changes in oneMKL 2025.0 #606

Merged
merged 5 commits into from
Nov 5, 2024

Conversation

sknepper
Copy link
Contributor

Description

Intel oneMKL 2025.0 updates LAPACK SYCL USM APIs to be const correct for input arrays. The internal APIs used for the Intel oneMKL backend for LAPACK need to be updated.
With these changes, the oneMKL/oneMath interfaces will work with Intel oneMKL 2025.0 and future versions. Previous versions of Intel oneMKL will have build failures. If needed, we can support previous versions with macros and slightly uglier code.

Checklist

  • Do all unit tests pass locally? Precommit testing on both Windows and Linux, for both CPU and Intel GPU devices, for both shared and static linking, passed 100%. For each of these platforms, 398 tests were run and passed for static, 399 tests were run and passed for shared (this includes the getrs_usm example).
    Even though these changes don't impact NVIDIA or AMD GPU backends, testing was run and all tests passed on these platforms as well (same numbers as above).

@sknepper sknepper requested a review from a team as a code owner October 23, 2024 20:18
@sknepper
Copy link
Contributor Author

@dnhsieh-intel - the clang-format check and provided diff file works perfectly! Thank you!!

The MKL LAPACK CPU tests are expected to fail with these changes because it is using the currently-available oneMKL 2024.2 release, while this PR is addressing upcoming changes in the oneMKL 2025.0 release. This PR should not be merged until the oneMKL 2025.0 release is publicly available.

Copy link

@jadurazo jadurazo left a comment

Choose a reason for hiding this comment

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

LGTM

@sknepper
Copy link
Contributor Author

@Rbiessy - any concerns with this PR? Thanks!

@Rbiessy
Copy link
Contributor

Rbiessy commented Oct 30, 2024

@sknepper, if you're asking if I have concerns with respect to the renaming PR then no. I'll make sure to solve these conflicts after I get some reviews.

I was thinking that it's not a great design that we need this header in oneMKL Interface, this is only duplicating the Intel oneMKL product header. With the renaming PR it will be much easier to include Intel oneMKL product header without conflict so I will be able to remove this header and this kind of PR won't be needed anymore. Similarly for the blas domain.

Other than this we will have a circular dependency issue since this PR is needed to get the CI working with 2025.0 and the merging of this PR is blocked until the CI can use 2025.0. You need to either:

@sknepper
Copy link
Contributor Author

@sknepper, if you're asking if I have concerns with respect to the renaming PR then no. I'll make sure to solve these conflicts after I get some reviews.

Thanks, @Rbiessy ! I was interested both about concerns with renaming and any general concerns as well.

I was thinking that it's not a great design that we need this header in oneMKL Interface, this is only duplicating the Intel oneMKL product header. With the renaming PR it will be much easier to include Intel oneMKL product header without conflict so I will be able to remove this header and this kind of PR won't be needed anymore. Similarly for the blas domain.

If such a change is possible, that would be great! I was under the impression that the header files here and in BLAS were needed because of the run-time dispatching capability in the interfaces.

Other than this we will have a circular dependency issue since this PR is needed to get the CI working with 2025.0 and the merging of this PR is blocked until the CI can use 2025.0. You need to either:

Cool, I didn't know where the CI changes were located. Looks like we're actually a little outdated (using oneMKL 2024.1). Let me try updating that in this PR.

@sknepper sknepper requested a review from a team as a code owner October 31, 2024 00:01
@sknepper
Copy link
Contributor Author

And that failed spectacularly. =)
@rscohn2 - what is needed to update the version of Intel oneMKL/compiler used in CI tests? Thanks!

@sknepper
Copy link
Contributor Author

And that failed spectacularly. =) @rscohn2 - what is needed to update the version of Intel oneMKL/compiler used in CI tests? Thanks!

Per email discussion, @rscohn2 is going to switch the install to remove the dependency on setup-oneapi repository.
So I reverted my changes to that file.

@sknepper sknepper mentioned this pull request Oct 31, 2024
@sknepper
Copy link
Contributor Author

sknepper commented Nov 4, 2024

I pulled in @rscohn2 's changes from #610 (along with the change requested by @dnhsieh-intel ).
All of the CI checks pass!
So if we want to keep a passing (green) CI, we can merge the LAPACK changes together with the CI changes that updated the version (and location) of oneAPI used in CI.

Copy link
Member

@rscohn2 rscohn2 left a comment

Choose a reason for hiding this comment

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

Compiler installs look good! I will close my PR.

@sknepper sknepper merged commit 68f7571 into oneapi-src:develop Nov 5, 2024
7 checks passed
@Rbiessy Rbiessy mentioned this pull request Nov 7, 2024
2 tasks
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.

5 participants