Improve type inference in plan_r2r

[jishnub]

Updated PR

Instead of storing the value of kinds as a type parameter in r2r transforms, we may store it as a struct field instead, as this improves type-stability. This doesn't rely on the value being constant propagated.

Older version of this PR

This is a somewhat hacky approach to improve type-inference in plan_r2r, in the case where kinds and region are compile-time constants. This is a common use case, so, for example, the following is type-stable after this:

julia> @inferred (() -> FFTW.plan_r2r(rand(4), FFTW.REDFT10))()
FFTW r2r REDFT10 plan for 4-element array of Float64
(redft10e-r2hc-4
  (rdft-r2hc-direct-r2c-4 "r2cf_4"))

The idea is that, in this case, the second type parameter of r2rFFTWPlan may be evaluated at compile time instead of a runtime call converting a Vector to a Tuple. This requires aggressive constant propagation through Base.@constprop :aggressive, which is not a part of the public API, but will hopefully be stable.

[codecov]

Codecov Report

Base: 61.28% // Head: 64.50% // Increases project coverage by +3.21% 🎉

Coverage data is based on head (6f834b7) compared to base (17bc81a).
Patch coverage: 85.71% of modified lines in pull request are covered.

❗ Current head 6f834b7 differs from pull request most recent head 9255357. Consider uploading reports for the commit 9255357 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #253      +/-   ##
==========================================
+ Coverage   61.28%   64.50%   +3.21%     
==========================================
  Files           5        5              
  Lines         483      493      +10     
==========================================
+ Hits          296      318      +22     
+ Misses        187      175      -12     

Impacted Files	Coverage Δ
src/fft.jl	64.12% <85.71%> (+2.76%)	⬆️
src/providers.jl	55.55% <0.00%> (+18.51%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[jishnub]

Test failure in "CI / Julia nightly - macOS-latest - x64 - provider mkl - 1 thread(s)" is unrelated, and is also seen on other recent PRs.

[jishnub]

Gentle bump

[jishnub]

Gentle bump

[stevengj]

The fix_kinds implementation is old and might also be re-thought. Especially if region is a scalar or tuple we should be able to make this allocation-free and type-stable (even without constant propagation).

[jishnub]

@stevengj how would you recommend proceeding? I don't know enough about this package to make large changes

[jishnub]

Gentle bump @stevengj

[jishnub]

Gentle bump

[jishnub]

I've simplified the implementation considerably by storing kinds as a struct field instead of a type parameter. This PR doesn't require constant propagation anymore. Given that the types of the plans are considered to be internal to the package, this should be non-breaking (although perhaps a minor version bump is warranted)

[stevengj]

Should we parameterize this? ScaledPlan is an abstract type, no?

[jishnub]

Yes, it is, but it's a bit tricky to do this properly, as the struct is initialized incompletely. Maybe I'll look into this in a separate PR

[stevengj]

LGTM.

[stevengj]

Unfortunately, this broke Hadamard.jl (which relies on internals of FFTW.jl); I'll fix it.