[DFT] Allow the descriptor to be modified and recommitted #282

FMarno · 2023-02-20T13:49:34Z

Description

Allows the descriptor class for DFTs to be modified and recommitted. Some backends will likely need to regenerate the descriptor completely, but this will allow for those that don't.

This builds on top of #259

Checklist

All Submissions

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

New features

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

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

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

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

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

src/dft/backends/mklcpu/commit.cpp

src/dft/descriptor_config_helper.hpp

src/dft/backends/mklgpu/commit.cpp

hjabird

I think the biggest challenge is what to do in situations that the spec doesn't cover - eg. changing queue or backend for the descriptor.

src/dft/descriptor.cxx

src/dft/backends/descriptor.cpp

src/dft/backends/mklgpu/descriptor.cpp

src/dft/backends/descriptor.cpp

src/dft/backends/mklcpu/commit.cpp

FMarno · 2023-03-10T17:10:11Z

Log of the test runs. My machine only supports single precision and through opencl, but I hope this demonstrates the descriptor tests are passing.
multi_commit_result.txt

Rbiessy · 2023-03-13T16:55:16Z

Next time this needs to be rebased you could fix the typo recommited -> recommitted in the commit description.

tests/unit_tests/dft/source/descriptor_tests.cpp

raphael-egan · 2023-03-18T00:11:18Z

src/dft/backends/descriptor.cpp

+        }
+        pimpl_.reset(detail::create_commit(*this, queue));
+    }
+    pimpl_->commit(values_);
 }


Maybe it's worth adding a recommit_values test which would change the queue between commit calls?

I've added a tests to show that there is a wait on the old queue here 088fa08
I was unable to prove that work is submitted to the new queue or not submitted to the old.

I don't believe there is any direct way to query if a queue has not completed tasks

I don't believe there is a way to query if an event is associated with a task.

When an command group function object is submitted to the queue, I don't believe there are any guarantees that is will complete before slow work on other queues.

Because of all this I can't think of how to test that reliably.

Thank you for that. I think you're right for every bullet point you mentioned and I'm sorry if my original suggestion generated complications/misunderstanding on the testing scope. Those changes look fine to me.
To be clearer, my original concern was about a use case like

create a descriptor desc;

create a queue q1 and commit desc to q1;

delete q1;

create a queue q2 and commit desc to q2;

submit a compute task to desc.
I am not 100% sure this works as expected for the closed-source mkl. But that is something we can/should check on our side as that would be an issue for us to fix anyways. We may consider this thread resolved.

Ok, that's interesting. I think this should work because objects like sycl::queue are kept alive by the SYCL runtime with reference counting semantics. I guess I could have a test that follows the steps you've detailed, waits on q2 and then shows that the event returned from the task is complete. It's not perfect but its something. I'm about to finish for the day but I'll do this sometime tomorrow.

I've added a test for that in 8d0a608

tests/unit_tests/dft/source/descriptor_tests.cpp

raphael-egan

Thank you for your work!

tests/unit_tests/dft/source/descriptor_tests.cpp

lhuot · 2023-03-28T07:43:05Z

@FMarno conflicts need to be resolved before I can merge this. Thanks!

Also reported error in cpu backend

FMarno · 2023-03-28T12:04:42Z

In the last rebase I made some changes to compute_inplace.hpp and compute_out_of_place.hpp which meant the same descriptor was used twice with a recommit in the middle to update the strides.

Here is evidence of the passing tests. I still only have access to single precision and the OpenCL backend -
recommit.txt

FMarno force-pushed the finlay/multi_commit branch from aad7522 to 35f0e3b Compare February 20, 2023 13:51

Rbiessy reviewed Feb 20, 2023

View reviewed changes

FMarno commented Feb 20, 2023

View reviewed changes

src/dft/backends/mklgpu/commit.cpp Outdated Show resolved Hide resolved

FMarno force-pushed the finlay/multi_commit branch 3 times, most recently from 461f11c to c7cc568 Compare February 21, 2023 12:12

hjabird reviewed Feb 21, 2023

View reviewed changes

src/dft/descriptor.cxx Outdated Show resolved Hide resolved

src/dft/backends/descriptor.cpp Show resolved Hide resolved

src/dft/backends/mklgpu/descriptor.cpp Show resolved Hide resolved

hjabird reviewed Feb 21, 2023

View reviewed changes

src/dft/backends/descriptor.cpp Show resolved Hide resolved

t4c1 reviewed Feb 22, 2023

View reviewed changes

src/dft/backends/mklcpu/commit.cpp Outdated Show resolved Hide resolved

FMarno changed the title ~~Allow the descriptor to be modified and recommited~~ [DFT] Allow the descriptor to be modified and recommited Feb 22, 2023

Rbiessy approved these changes Feb 22, 2023

View reviewed changes

t4c1 approved these changes Feb 23, 2023

View reviewed changes

FMarno force-pushed the finlay/multi_commit branch from ece0cc4 to 924a08d Compare March 9, 2023 13:55

raphael-egan reviewed Mar 18, 2023

View reviewed changes

FMarno force-pushed the finlay/multi_commit branch from 1d17c5c to 088fa08 Compare March 21, 2023 17:45

FMarno changed the title ~~[DFT] Allow the descriptor to be modified and recommited~~ [DFT] Allow the descriptor to be modified and recommitted Mar 21, 2023

Rbiessy approved these changes Mar 22, 2023

View reviewed changes

tests/unit_tests/dft/source/descriptor_tests.cpp Show resolved Hide resolved

raphael-egan approved these changes Mar 22, 2023

View reviewed changes

Rbiessy reviewed Mar 23, 2023

View reviewed changes

tests/unit_tests/dft/source/descriptor_tests.cpp Outdated Show resolved Hide resolved

Rbiessy approved these changes Mar 23, 2023

View reviewed changes

lhuot approved these changes Mar 28, 2023

View reviewed changes

FMarno added 6 commits March 28, 2023 11:44

Allow descriptors to be recommitted for certain params

2519504

remove unneeded argument from commit_impl::commit

45de231

formatting

5c39e07

allow commit to change the queue used

1691ba2

Added wait queue when it is removed from a commit_impl

351a195

Also reported error in cpu backend

style change

b1404b5

FMarno added 5 commits March 28, 2023 11:44

modified descriptor recommit tests

447dc7d

made recommit test out-of-place for simplicity

d2bade9

Add test for queue that goes out of scope

843c41f

use the same device for any new queues in tests

e70414c

update output strides for backward stage of out-of-place tests

4259829

FMarno force-pushed the finlay/multi_commit branch from d6a5430 to 4259829 Compare March 28, 2023 11:49

Rbiessy approved these changes Mar 28, 2023

View reviewed changes

lhuot merged commit f58b93f into uxlfoundation:develop Mar 29, 2023

FMarno deleted the finlay/multi_commit branch March 29, 2023 10:43

FMarno mentioned this pull request Mar 29, 2023

[DFT] Introduce the cuFFT backend for the DFT interface. #284

Merged

2 tasks

HaoweiZhangIntel mentioned this pull request Mar 30, 2023

[cuSolver] Avoid repeated ctxCreate/Destroy for all Lapack API calls. #298

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DFT] Allow the descriptor to be modified and recommitted #282

[DFT] Allow the descriptor to be modified and recommitted #282

FMarno commented Feb 20, 2023 •

edited

Loading

hjabird left a comment

FMarno commented Mar 10, 2023

Rbiessy commented Mar 13, 2023

raphael-egan Mar 18, 2023

FMarno Mar 21, 2023 •

edited

Loading

raphael-egan Mar 22, 2023

FMarno Mar 22, 2023

FMarno Mar 23, 2023

raphael-egan left a comment

lhuot commented Mar 28, 2023

FMarno commented Mar 28, 2023

[DFT] Allow the descriptor to be modified and recommitted #282

[DFT] Allow the descriptor to be modified and recommitted #282

Conversation

FMarno commented Feb 20, 2023 • edited Loading

Description

Checklist

All Submissions

New features

hjabird left a comment

Choose a reason for hiding this comment

FMarno commented Mar 10, 2023

Rbiessy commented Mar 13, 2023

raphael-egan Mar 18, 2023

Choose a reason for hiding this comment

FMarno Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

raphael-egan Mar 22, 2023

Choose a reason for hiding this comment

FMarno Mar 22, 2023

Choose a reason for hiding this comment

FMarno Mar 23, 2023

Choose a reason for hiding this comment

raphael-egan left a comment

Choose a reason for hiding this comment

lhuot commented Mar 28, 2023

FMarno commented Mar 28, 2023

FMarno commented Feb 20, 2023 •

edited

Loading

FMarno Mar 21, 2023 •

edited

Loading