Sync fft interfaces by anantsrivastava30 · Pull Request #235 · uxlfoundation/oneMath

anantsrivastava30 · 2022-10-10T07:31:53Z

Description

Please include a summary of the change. Please also include relevant
motivation and context. See
contribution guidelines
for more details. If the change fixes an issue not documented in the project's
Github issue tracker, please document all steps necessary to reproduce it.

Fixes # (GitHub issue)

Checklist

All Submissions

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

New interfaces

Have you provided motivation for adding a new feature as part of RFC and
it was accepted? # (RFC)
What version of oneAPI spec the interface is targeted?
Complete New features checklist

New features

Have you provided motivation for adding a new feature?
Have you added relevant tests?

Bug fixes

Have you added relevant regression tests?
Have you included information on how to reproduce the issue (either in a
GitHub issue or in this PR)?

…ync_fft_interfaces

…and dom

… changes

t4c1 · 2022-10-12T09:39:56Z

include/oneapi/mkl/dft/detail/descriptor_impl.hpp

+namespace dft {
+namespace detail {
+
+class descriptor_impl {


Everything in this impl class is general (not backend specific). So, it does not need to be in descriptor_impl, but can be in the descriptor class. We only need pimpl for backend-specific parts (factorizations, twiddle tables etc) and these can only be initialized in the commit call, not in descriptor constructor.

Implementations will need to be backend specific (so one in every backend) and for now you can leave them empty. They will only be initialized in commit implementation for each backend.

Maybe we dont even need pimpl, just a pointer that each backend can set to point to its internal data it needs to store in the descriptor (just thinking out loud).

Hey Tadej, thank you for the review, I have a few options to move forward

impliment the constructor and the set_value + get_value in the descriptor.hpp, use pimpl only for commit

if we prefer to separate implimentation and declaration then I would have to explicitly specialize consturctors + pre-commit member functinos, which would bloat the implimentation code (e.g. src/dft/backend/descriptor.cxx)

could use detail::create_descriptor (pointer to class impl) for general implimentation, and detail::create_commit(pointer to different commit class ?) along withmklcpu::create_commit(...mklgpu::create_commit`), this would avoid some bloating as create_descriptor can be templated and passed on to the impl class.

for now I can work on option 1 as the prefered way, let me know your thoughts on this.

Yeah, I like 1 the best as well. Other options sound like unnecessary complications.

710a4e1, should enable you to work on gpu commit.

t4c1 · 2022-10-12T09:42:07Z

include/oneapi/mkl/dft/detail/dft_loader.hpp

+template <oneapi::mkl::dft::precision prec, oneapi::mkl::dft::domain dom>
+oneapi::mkl::dft::detail::descriptor_impl* create_descriptor(std::size_t length);
+
+template <oneapi::mkl::dft::precision prec, oneapi::mkl::dft::domain dom>
+oneapi::mkl::dft::detail::descriptor_impl* create_descriptor(std::vector<std::int64_t> dimensions);


We won't need these after everything from descriptor_impl is moved to descriptor.

…ation

t4c1

I added multiple comments, but I think only the first one actually blocks any further implementations.

t4c1 · 2022-10-14T06:59:15Z

include/oneapi/mkl/dft/descriptor.hpp

+#ifdef ENABLE_MKLCPU_BACKEND
+        : pimpl_(mklcpu::create_descriptor(length)) {}
+#endif
+#ifdef ENABLE_MKLGPU_BACKEND
+        : pimpl_(mklgpu::create_descriptor(length)) {}
+#endif


This will prevent us from compiling for multiple backends. pimpl_ should be initialized in commit, where we know which backend is used (from the queue).

t4c1 · 2022-10-14T06:59:40Z

include/oneapi/mkl/dft/descriptor.hpp

    descriptor(std::vector<std::int64_t> dimensions);

-    ~descriptor();
+    // ~descriptor();


Why comment out destructor? I think this should be reverted.

Also, destructor should free pimpl if commit was called before descriptor is destroyed.

pimpl is casted as unique_ptr, it will free itself as it goes out of scope.

Makes sense. This is fine as it is.

t4c1 · 2022-10-14T07:01:50Z

include/oneapi/mkl/dft/detail/descriptor_impl.hpp

+    }
+
+protected:
+    sycl::queue queue_;


I think we don't need queue in the descriptor_impl, as it is part of the descriptor.

t4c1 · 2022-10-14T07:03:38Z

include/oneapi/mkl/dft/detail/dft_loader.hpp

+
+namespace mklcpu {
+
+ONEMKL_EXPORT oneapi::mkl::dft::detail::descriptor_impl* create_descriptor(std::size_t length);


We do not need those anymore. In fact, this whole file can be removed.

anantsrivastava30 · 2022-10-14T19:15:46Z

I added multiple comments, but I think only the first one actually blocks any further implementations.

I think you reviewed old code, there have been a few commits after the one you reviewed.

t4c1 · 2022-10-17T09:24:04Z

Right. Nothing in this should block us now, but the comment about not needing queue in descriptor_impl still stands.

KumudhaN · 2022-10-19T14:30:13Z

@anantsrivastava30 Can you modify the PR to target the develop branch in your fork and then we should be able to work on top of that.
Thanks!

anantsrivastava30 · 2022-10-19T16:33:44Z

@anantsrivastava30 Can you modify the PR to target the develop branch in your fork and then we should be able to work on top of that. Thanks!

@KumudhaN thanks for pointing out opend a local PR! and merging it to local anantsrivastava30#1

Rbiessy · 2023-05-09T12:22:50Z

@anantsrivastava30 I think this can be closed right?

t4c1 and others added 7 commits September 27, 2022 08:28

added dft interface and implementation stubs for mklgpu backend

6dbd3d4

format

32ddffe

addressed comments from internal review

e481041

Merge branch 'dft_interface' of https://github.com/t4c1/oneMKL into s…

bccc24c

…ync_fft_interfaces

pimpl descriptor class skeleton

a48b583

push descriptor outside of device specific impl + desc class w/ prec …

47af09e

…and dom

initial implementation of set_value and corresponding desc_impl class…

0c62927

… changes

t4c1 requested changes Oct 12, 2022

View reviewed changes

anantsrivastava30 added 3 commits October 12, 2022 11:43

before refactoring the desc_impl class

d5f824b

refactor and move desc class outside of pimpl & constructor generaliz…

710a4e1

…ation

enable setting strides

3f9d0b8

t4c1 requested changes Oct 14, 2022

View reviewed changes

hjabird mentioned this pull request Dec 29, 2022

[DFT] Initial DFT work & MKLGPU backend #259

Merged

1 task

anantsrivastava30 closed this May 9, 2023


		namespace mklcpu {

		ONEMKL_EXPORT oneapi::mkl::dft::detail::descriptor_impl* create_descriptor(std::size_t length);

Comments

Conversation

anantsrivastava30 commented Oct 10, 2022

Description

Checklist

All Submissions

New interfaces

New features

Bug fixes

Uh oh!

t4c1 Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anantsrivastava30 Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t4c1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anantsrivastava30 commented Oct 14, 2022

Uh oh!

t4c1 commented Oct 17, 2022

Uh oh!

KumudhaN commented Oct 19, 2022

Uh oh!

anantsrivastava30 commented Oct 19, 2022

Uh oh!

Rbiessy commented May 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

t4c1 Oct 12, 2022 •

edited

Loading

anantsrivastava30 Oct 13, 2022 •

edited

Loading