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

[DFT] Add static_assert to check types passed into compute_foward and compute_backward #502

Conversation

hjabird
Copy link
Contributor

@hjabird hjabird commented May 28, 2024

Description

This adds static_asserts more rigidly check that input/output argument types are as expected.

Eg.

    auto in_usm = sycl::malloc_shared<std::complex<float>>(N, sycl_queue);
    auto out_usm = sycl::malloc_shared<std::complex<float>>(N, sycl_queue);
    oneapi::mkl::dft::descriptor<oneapi::mkl::dft::precision::SINGLE,
                                 oneapi::mkl::dft::domain::COMPLEX>
        desc(static_cast<std::int64_t>(N));
    desc.set_value(oneapi::mkl::dft::config_param::PLACEMENT,
                   oneapi::mkl::dft::config_value::NOT_INPLACE);
    desc.commit(sycl_queue);
    auto compute_event = oneapi::mkl::dft::compute_forward(desc, in_usm, out_usm);

now fails because the user has inadverently used called the compute_forward for in-place real-real DFTs. The error would help the user identify that they ought to be using

  oneapi::mkl::dft::compute_forward<decltype(desc), std::complex<float>, std::complex<float>>(desc, in_usm, out_usm);

Partially resolves #499

Checklist

All Submissions

  • Do all unit tests pass locally? Attach a log.
  • Have you formatted the code using clang-format?

testres.txt

* static assert that the types are real or complex as required
@hjabird hjabird marked this pull request as ready for review May 28, 2024 15:22
@hjabird hjabird requested review from FMarno, Rbiessy and lhuot and removed request for FMarno May 28, 2024 15:22
@lhuot
Copy link
Contributor

lhuot commented May 29, 2024

The case you describe in the PR description seems like one of the most common use-case. Is there any way we can get the correct template being picked up without having to explicitly pass the additional template argument? I think it would be okay to have to explicitly pass the template argument for the REAL_REAL descriptor but this seems really inconvenient for "standard" complex out of place FFTs.

@hjabird
Copy link
Contributor Author

hjabird commented May 29, 2024

The case you describe in the PR description seems like one of the most common use-case. Is there any way we can get the correct template being picked up without having to explicitly pass the additional template argument? I think it would be okay to have to explicitly pass the template argument for the REAL_REAL descriptor but this seems really inconvenient for "standard" complex out of place FFTs.

I think the obvious way to do it would be to use enable_if. However, this would make the function signatures look much less like the spec.

@lhuot
Copy link
Contributor

lhuot commented May 30, 2024

The case you describe in the PR description seems like one of the most common use-case. Is there any way we can get the correct template being picked up without having to explicitly pass the additional template argument? I think it would be okay to have to explicitly pass the template argument for the REAL_REAL descriptor but this seems really inconvenient for "standard" complex out of place FFTs.

I think the obvious way to do it would be to use enable_if. However, this would make the function signatures look much less like the spec.

I think we found a gap in the spec that should be resolved, if implementing the spec is forcing the user to specify the template argument on the most commonly used FFT then there is a problem in my opinion. My suggestion would be to address to the best we can and by staying as close to the spec as possible this issue here and open an issue for the specification. For example, keeping the same entry point but adding a runtime check to re-dispatch the proper function as suggested by @raphael-egan. @Rbiessy @hjabird what do you think?

@hjabird
Copy link
Contributor Author

hjabird commented May 30, 2024

I've created a version using SFINAE at https://github.com/hjabird/oneMKL/tree/dft_overload_resolution_with_enableif. If this is what we're looking for, I'll close this PR and open a new PR for that.

The benefit is that the desired overload correctly gets chosen.

The downside is that if you don't have the correct types as inputs, you'll get long error messages.

@hjabird
Copy link
Contributor Author

hjabird commented May 31, 2024

It is likely that this PR will be closed in favour of #503.

@hjabird
Copy link
Contributor Author

hjabird commented Jun 11, 2024

Closing since #503 is the preferred solution.

@hjabird hjabird closed this Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants