support MKL FFT backend#411
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds support for using Intel MKL as the FFTW backend by detecting when MKL is configured and skipping FFTW wisdom loading/saving operations (which are not applicable to MKL).
Changes:
- Added Preferences package dependency to query FFTW provider configuration
- Modified
loadFFTwisdom()andsaveFFTwisdom()to skip wisdom operations when MKL is the FFT provider - Added logging statements to inform users when MKL is detected and wisdom operations are skipped
- Added logging for FFTW thread configuration
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Utils.jl | Added fft_provider_is_mkl() helper function and modified wisdom loading/saving to skip operations when MKL is detected; added logging for FFTW thread settings and MKL detection |
| src/Luna.jl | Added logging statement when FFTW threads are set via set_fftw_threads() |
| Project.toml | Added Preferences package as a dependency with version constraint 1.5.1 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@chrisbrahms I think this is ready now. It now has a specific MKL test run, which was useful because several tests failed with MKL. These are fixed, but the changes were slightly more invasive than I initially expected. Please have a look. On our workstation, this PR makes quite a big difference. |
|
The mkl-test CI run doesn't yet run on this repository (until this PR is merged), but it does run on my branch and passes: https://github.com/jtravs/Luna.jl/actions/runs/22555466068 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 11 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function _fft_dim(x::AbstractArray, dim::Integer) | ||
| ndims(x) <= 2 && return FFTW.fft(x, dim) | ||
| if dim == 1 | ||
| sz = size(x) | ||
| return reshape(FFTW.fft(reshape(x, sz[1], :), 1), :, sz[2:end]...) | ||
| end | ||
| return mapslices(s -> FFTW.fft(vec(s)), x; dims=dim) | ||
| end |
There was a problem hiding this comment.
These _fft_dim/_ifft_dim/_rfft_dim/_irfft_dim helpers apply the MKL workaround unconditionally for 3D+ arrays. On the normal FFTW provider, FFTW.fft(x, dim) supports partial-dimension transforms for any dim, but the mapslices fallback here (for dim != 1) will be much slower and allocate heavily. To avoid a performance regression for non-MKL users, consider branching on the active provider (e.g. FFTW.fftw_provider != "mkl") and calling the native FFTW.*fft*(x, dim) paths in that case, only using the reshape/mapslices workaround when MKL is actually active.
| # MKL's FFTW compatibility layer cannot create partial-dimension FFT plans for 3D+ arrays. | ||
| # For dim=1, we reshape to 2D (zero-copy), apply the FFT, and reshape back. | ||
| # For other dims, we fall back to mapslices with 1D transforms. |
There was a problem hiding this comment.
The new comment notes MKL can’t create partial-dimension FFT plans for 3D+ arrays, and the helpers below work around this for direct fft/ifft/rfft/irfft calls. However, there are still code paths in this file that build explicit partial-dimension plans (e.g. plan_hilbert! uses FFTW.plan_fft(copy(x), dim, ...)), which will likely still fail under MKL for ndims(x) > 2. Consider guarding those plan-creation paths similarly (or detecting MKL+3D and falling back with a clear error) so MKL support is consistent.
| ``` | ||
| or, equivalently: | ||
| ```julia | ||
| pkg> add Luna#master |
There was a problem hiding this comment.
The development install snippet uses pkg> add Luna#master. If the repository default branch is main (as suggested by this PR’s base branch name), this command will fail for contributors. Consider using pkg> add Luna#main or wording this in a branch-agnostic way (e.g. "add Luna#").
| pkg> add Luna#master | |
| pkg> add Luna#main |
| test-mkl: | ||
| runs-on: ubuntu-latest | ||
| if: github.event.pull_request.draft == false | ||
| steps: |
There was a problem hiding this comment.
This workflow runs on both push and pull_request, but the job condition uses github.event.pull_request.draft. On push events that context is null, so this job will be skipped (and may not behave as intended). If you want the MKL tests to run on pushes too, consider a condition like github.event_name != 'pull_request' || github.event.pull_request.draft == false; if you only want it on PRs, consider restricting the workflow triggers instead of relying on a PR-only context.
As the title says. This skips wisdom loading/saving if the user has configured FFTW to use MKL as the backend (which is a good idea as it boosts performance).