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

[oneMKL][DFT] Copying and moving the dft::descriptor class #487

Closed
hjabird opened this issue May 25, 2023 · 1 comment
Closed

[oneMKL][DFT] Copying and moving the dft::descriptor class #487

hjabird opened this issue May 25, 2023 · 1 comment

Comments

@hjabird
Copy link
Contributor

hjabird commented May 25, 2023

The oneMKL descriptor class has its constructor / destructor / copy / move interface defined as follows:

namespace oneapi::mkl::dft {
   template <oneapi::mkl::dft::precision prec, oneapi::mkl::dft::domain dom>
   class descriptor {
    public:
       descriptor(std::int64_t length);
       descriptor(std::vector<std::int64_t> dimensions);
       ~descriptor();
    };
}

Consequently one might expect:

  • Copy constructor and copy assignment - should be present.
  • Move constructor and assignment - I think these are implicitly deleted due to the user declared destructor.

In the current implementation of oneMKL, the implicit copy assignment / constructors are deleted due to the unique_ptr to the backend-specific descriptor implementation class.

For copying, it isn't clear what the behavour should be:

  • If constructor was copied, except for the backend-specific implementation class, and then recommitted, copying would become a very expensive operation.
  • Throwing on copying an already committed constructor at runtime would be annoying.

I don't see any downsides to move constructors / assignment operators.

What should the behaviour of the copy constructor be, and should there be move constructors?

hjabird added a commit to hjabird/oneAPI-spec that referenced this issue Jul 7, 2023
* Relates to uxlfoundation#487
* Adds move constructor/assignment to descriptor class
  * Otherwise implicitly deleted.
* Add explicit copy constructor/assignment to descriptor class
  * Otherwise ambigiuous
* Notes that copy operation is by value.
mmeterel pushed a commit that referenced this issue Jul 21, 2023
* [oneMKL] dft::descriptor copy and move (#487)

* Relates to #487
* Adds move constructor/assignment to descriptor class
  * Otherwise implicitly deleted.
* Add explicit copy constructor/assignment to descriptor class
  * Otherwise ambigiuous
* Notes that copy operation is by value.

* Correct assignment signatures; Clarify copy by value to deep-copy

* Reword (Suggestion FMarno)

Co-authored-by: Finlay <finlay.marno@codeplay.com>

---------

Co-authored-by: Finlay <finlay.marno@codeplay.com>
@hjabird
Copy link
Contributor Author

hjabird commented Jul 25, 2023

#490 resolves this issue.

@hjabird hjabird closed this as completed Jul 25, 2023
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

No branches or pull requests

1 participant