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.

Sign up for GitHub

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

Jump to bottom

[DFT][MKLGPU] Use FWD/BWD_STRIDES #514

Merged
merged 3 commits into from
Jun 24, 2024
Merged

[DFT][MKLGPU] Use FWD/BWD_STRIDES #514

merged 3 commits into from
Jun 24, 2024

Conversation

hjabird
Copy link
Contributor

@hjabird hjabird commented Jun 13, 2024

Description

  • Intel(R) oneMKL 2024.1 deprecate the INPUT/OUTPUT_STRIDES API
  • Using the old API causes error messages to be printed.
  • This PR uses the new FWD/BWD_STRIDES API, but maintains support for older Intel(R) oneMKL.
  • Fixes a warning
    • sources/src/dft/backends/mklgpu/descriptor.cpp:30:29: warning: a declarative nested name specifier cannot name an alias template [-Walias-template-in-declaration-name]
    • This warning seems to need nightly clang rather than icpx.
  • It additionally fixes one-line bug in cuFFT (albeit one that doesn't seem to cause any issues).

Fixes #487

Checklist

All Submissions

  • Do all unit tests pass locally? Attach a log.
    • testres.txt
    • Tests only attached for MKLGPU. Other domains have the minor modification to stop the compiler warning. Ctest has been run and passes.
  • Have you formatted the code using clang-format?

Rbiessy
Rbiessy reviewed Jun 14, 2024
View reviewed changes
src/dft/backends/mklgpu/commit.cpp Outdated Show resolved Hide resolved
src/dft/backends/mklgpu/commit.cpp Outdated Show resolved Hide resolved
src/dft/backends/mklgpu/commit.cpp Show resolved Hide resolved
@hjabird
Copy link
Contributor Author

hjabird commented Jun 14, 2024

Note that we could just hide the deprecation warnings with pragma like here if we just want a quick fix. It would be better to find a long term solution.

MKL prints an warning to std::cerr at runtime, so I don't think this is the fix we're looking for.

@hjabird
[DFT][MKLGPU] Use MKLGPU's FWD/BWD_STRIDES API to avoid deprecation m…
ac0c63e 
…essage

* 2024.1's MKL prints a deprecation message when the deprecated INPUT/OUTPUT API is used.
* This checks for 2024.1 and uses the new FWD/BWD stride API instead.
* Also minor bugfix for CuFFT workspace size.
@hjabird
Initialize workspaceSizeBwd
9ae3a61
@hjabird hjabird force-pushed the hjab/mklgpu_202401_deprecation_warning branch from eba8191 to 9ae3a61 Compare June 17, 2024 14:27
@lhuot
Copy link
Contributor

lhuot commented Jun 18, 2024

Right, I am still wondering if we should completely remove support for INPUT/OUTPUT STRIDES and require oneAPI 2024.1 once oneMKL Interface supports FWD/BWD STRIDES. I think it would allow to have just one descriptor for MKLGPU and portFFT backends but I don't know the impact on the other backends. I understand this would be a breaking change. I'm curious to know the opinion of @lhuot on the matter.

We should support the latest release as documented in the README so imo it is okay to require oneMKL 2024.1 and cleanup the code to use only fwd/bwd_stride when using the SYCL API from the Intel oneMKL library.

@hjabird
Copy link
Contributor Author

hjabird commented Jun 18, 2024

I'll remove the support for 2024.0.

In the meantime, it looks like I've somehow broken my implementation, so please hold off reviewing!

@hjabird
Copy link
Contributor Author

hjabird commented Jun 18, 2024

I've updated the PR to remove support for earlier than 2024.1, and also to sort out an issue with exception handling. Test results are here: testres.txt

Q: How come it was working earlier and then you said it was broken?
A: I previously fixed this issue, giving the results earlier, but failed push the relavent commit. Then I deleted it locally and and had to redo the fix all over.

@hjabird
Copy link
Contributor Author

hjabird commented Jun 24, 2024

@lhuot are you happy with this PR in its revised form?

lhuot
lhuot approved these changes Jun 24, 2024
View reviewed changes
Copy link
Contributor

@lhuot lhuot left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the patch!

@hjabird hjabird merged commit 222f0d6 into oneapi-src:develop Jun 24, 2024
6 checks passed
normallytangent pushed a commit to normallytangent/oneMKL that referenced this pull request Aug 6, 2024
@hjabird @normallytangent
[DFT][MKLGPU] Use FWD/BWD_STRIDES (oneapi-src#514)
c248bc2 
* INPUT/OUTPUT_STRIDES are deprecated in the oneMKL spec and Intel(R) oneMLK 2024.1 (the latest release)
* Using them causes a warning message to be printed
* This PR uses the new API: FWD/BWD_STRIDES
@hjabird hjabird mentioned this pull request Aug 7, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lhuot lhuot lhuot approved these changes

@Rbiessy Rbiessy Rbiessy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[DFT][MKLGPU] Use FWD_ and BWD_STRIDES for MKLGPU 2024.1
3 participants
@hjabird @lhuot @Rbiessy