[DFT] Initial DFT work & MKLGPU backend #259

hjabird · 2022-12-29T15:33:32Z

Co-authored-by @t4c1 and @anantsrivastava30.

Description

Implements DFT common functionality.
- Motivations: DFT is a core domain in the OneMKL spec.
Implements MKLGPU backend.
Tested against adapted examples from closed-source MKLGPU library and prototype unit tests (see Add unit-tests for the dft interface #261).

Checklist

All Submissions

Tests are still a work in progress. Tested against prototype tests and code adapted from the MKLGPU DFT examples.
[ x ] Have you formatted the code using clang-format?

New interfaces

Targets the OneAPI 1.2-rev-1 spec.
Complete New features checklist

hjabird · 2022-12-29T15:45:46Z

This commit contains @anantsrivastava30 work from, with additional alterations:

anantsrivastava30 · 2022-12-29T19:01:13Z

Please correct me if I am wrong but I don't thing this is the correct git practice, we need to make sure the work is rebased on top of others' commits because this masks the work done by previous authors and as github keeps track of the credit we should make sure thats held intact.

hjabird · 2022-12-30T10:32:34Z

Please correct me if I am wrong but I don't thing this is the correct git practice, we need to make sure the work is rebased on top of others' commits because this masks the work done by previous authors and as github keeps track of the credit we should make sure thats held intact.

I've credited you and Tadej as co-authors in both the commit message (with Co-authored-by:) and at the top of this PR. I've squashed the commits into one to create a clean commit history. Even if the commits were separate now, I think commits from different authors are usually squashed when a PR is merged.

From a review perspective, I think it is best that your work is included in this PR - I had to make quite a few changes to your code. For example, the descriptor class had to be moved from the dft to the detail namespace to avoid ambiguity with the decsriptor class in the Intel MKLGPU closed-source library. I don' t think there is any benefit to reviewers first reviewing the descriptor class, and then its move to the detail namespace when they could just review detail::descriptor.

If you'd like me to separate out your work within this PR I can of course do that. However, as I said, I'd expect the commits to be squashed on merging.

EDIT: It appears that I needed to put Co-authored-by at the bottom of the commit message rather than the top. Sorry for this mistake.

examples/dft/compile_time_dispatching/complex_fwd_buffer_mklgpu.cpp

examples/dft/compile_time_dispatching/CMakeLists.txt

examples/dft/compile_time_dispatching/complex_fwd_buffer_mklgpu.cpp

examples/dft/run_time_dispatching/CMakeLists.txt

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

src/dft/backends/backend_backward_instantiations.cxx

src/dft/backends/backend_forward_instantiations.cxx

src/dft/descriptor_config_helper.hpp

mmeterel · 2023-01-09T20:51:18Z

A quick high-level comment on the organization of this PR and #261. I am not in favor of adding a backend with no actual tests to cover it. Can we either add the tests first (they won't be used until we add the backend, but that is ok) or add the backend and tests in the same PR with proper testing? (with attached tests logs in the PR)

Also, unless I missed it, I don't see any changes to README and other documentation regarding the addition of new backend (along with supported compilers, OS, hardware, etc). Could you please update them as needed?

hjabird · 2023-01-13T11:23:45Z

A quick high-level comment on the organization of this PR and #261. I am not in favor of adding a backend with no actual tests to cover it. Can we either add the tests first (they won't be used until we add the backend, but that is ok) or add the backend and tests in the same PR with proper testing? (with attached tests logs in the PR)

Also, unless I missed it, I don't see any changes to README and other documentation regarding the addition of new backend (along with supported compilers, OS, hardware, etc). Could you please update them as needed?

I've added that MKLGPU is supported in the main README in 6307446.

I very much agree that this should be tested before merging. I think that this PR is already large enough, and that for the purpose review, it would be better to have feedback on the work in the tests PR and this PR separately. I think it would be best if the tests were merged first, and this PR rebased to include them.

mkrainiuk

Thanks for the PR! I have several questions below to these changes.

CMakeLists.txt

examples/dft/compile_time_dispatching/CMakeLists.txt

examples/dft/CMakeLists.txt

src/dft/backends/backend_compute_signature.cxx

src/dft/backends/mklgpu/commit.cpp

tests/unit_tests/dft/source/CMakeLists.txt

src/dft/backends/mklgpu/forward.cpp

* Implements DFT common functionality * Implements MKLGPU backend * Tested against adapted examples from closed-source MKLGPU library and prototype unit tests (to be submitted in another PR). Co-authored-by: Anant Srivastava <[email protected]> Co-authored-by: Tadej Ciglaric <[email protected]>

* It may be desireable to link to only libonemkl_dft_mklgpu without the need to also link to libonemkl_dft. * To allow this, the OneMKL interface must be duplicated within the backend library. * This PR duplicates the interface, allowing direct linking.

tests/unit_tests/CMakeLists.txt

README.md

mmeterel · 2023-02-15T20:57:55Z

Now that #261 is merged, it would be good to see the test logs in this PR.

There is no reason to believe it wouldn't work, but it has not been tested.

Rbiessy · 2023-02-16T13:38:33Z

Here are the logs:
log_mklcpu.txt
log_mklgpu.txt

Note that I ran this with the OpenCL runtime as this is currently easier on my development machine. I am commenting out this skip here.

anantsrivastava30

Approving, given the md test PR #275 passes with these as well

lhuot · 2023-03-02T08:26:03Z

@mmeterel and @mkrainiuk this PR looks good on the DFT side. Do you have any concerns left regarding how this fit to the overall design of the project?

FMarno · 2023-03-06T15:45:58Z

the MKLCPU backend wasn't building, so I've fixed that. Here is the new log:
log_mklcpu.txt

raphael-egan · 2023-03-10T20:20:20Z

src/dft/backends/mklcpu/commit.cpp

+        set_value_item(descHandle, DFTI_INPUT_DISTANCE, config.fwd_dist);
+        set_value_item(descHandle, DFTI_OUTPUT_DISTANCE, config.bwd_dist);


@anantsrivastava30, @t4c1, @Rbiessy, @hjabird @lhuot: I know this PR was merged already but what motivated this usage of DFTI_INPUT_DISTANCE and DFTI_OUTUT_DISTANCE via dpc++ APIs? Those parameters are not listed in the specs and we explicitly recommend to use FWD_DISTANCE and BWD_DISTANCE for the usage of the closed-source oneMKL via DPC++ APIs here.

Hey @raphael-egan the usage of these are mostly to conform to the classical API usage, that still uses these parameters, however I see you point and we can mask them in the cpu backend pr#288, so that on the user end we still use fwd/bwd distance and later on in this part of the code change these to input/output distance, lets move this discussion to #288

Yes, it makes sense to move to the C++ API in the CPU MR. The initial work used the C API for the CPU backend and we didn't update this to avoid conflicts. Thanks.

fabiomestre mentioned this pull request Dec 30, 2022

Add unit-tests for the dft interface #261

Merged

1 task

lhuot reviewed Jan 6, 2023

View reviewed changes

examples/dft/compile_time_dispatching/complex_fwd_buffer_mklgpu.cpp Outdated Show resolved Hide resolved

FMarno reviewed Jan 6, 2023

View reviewed changes

lhuot requested review from mkrainiuk and mmeterel January 9, 2023 16:55

This comment was marked as resolved.

Sign in to view

FMarno approved these changes Jan 26, 2023

View reviewed changes

Rbiessy approved these changes Jan 26, 2023

View reviewed changes

mkrainiuk reviewed Jan 31, 2023

View reviewed changes

hjabird commented Feb 1, 2023

View reviewed changes

tests/unit_tests/dft/source/CMakeLists.txt Outdated Show resolved Hide resolved

anantsrivastava30 reviewed Feb 1, 2023

View reviewed changes

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

anantsrivastava30 reviewed Feb 1, 2023

View reviewed changes

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

FMarno mentioned this pull request Feb 10, 2023

[DFT] Introduce a cuFFT backend for the DFT interface #278

Closed

2 tasks

hjabird and others added 12 commits February 15, 2023 17:22

Revert unnecessary CMake change in tests

dd8a81c

Remove dft::mklgpu:: interface

2ed2cd3

Update Copyright dates

26af793

Replace partial specialization with if constexpr

f37527b

Remove macros from backend_XX_instantiations

ee47ace

Use default copy constructor

44d6278

Make OpenCL required in example

c5977d0

Print banner uses single string

e76f47c

Add DFT & MKLGPU to support tables

9cb2c9f

Add missing header for va_arg

06d6eb9

remove tmp.cpp

c7ecd6e

Rbiessy reviewed Feb 15, 2023

View reviewed changes

tests/unit_tests/CMakeLists.txt Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

README.md Show resolved Hide resolved

FMarno and others added 3 commits February 16, 2023 11:11

Removed windows support from README

dc3aa7c

There is no reason to believe it wouldn't work, but it has not been tested.

Fix deprecated warnings

57979be

Disable example for MKLCPU for now

f7ba088

FMarno mentioned this pull request Feb 20, 2023

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

Merged

4 tasks

FMarno added 5 commits February 20, 2023 16:32

small improvements

0eccafb

small style changes

5eb85b7

format

32df194

improve uniformity in dft length test

f8f4d75

remove implicit conversion

9f0b8a6

Rbiessy mentioned this pull request Feb 21, 2023

Add option to enable tests on OpenCL devices #283

Closed

9 tasks

revert unneeded cmake change

519fde5

FMarno mentioned this pull request Mar 1, 2023

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

Merged

2 tasks

anantsrivastava30 approved these changes Mar 1, 2023

View reviewed changes

mkrainiuk approved these changes Mar 2, 2023

View reviewed changes

lhuot approved these changes Mar 3, 2023

View reviewed changes

fix dft building for mklcpu backend

1ecf9e9

anantsrivastava30 mentioned this pull request Mar 6, 2023

[DFT] Synced mklcpu backend + Real_Real tests #288

Merged

11 tasks

This was referenced Mar 7, 2023

[DFT] Add more warning flags #290

Merged

[DFT] Split descriptor tests that do not need a device #291

Merged

lhuot merged commit e4c3b2e into uxlfoundation:develop Mar 8, 2023

Rbiessy deleted the FFT_MKLGPU_staging branch March 9, 2023 11:18

raphael-egan reviewed Mar 10, 2023

View reviewed changes

Rbiessy mentioned this pull request Aug 26, 2024

Commit #243

Closed

9 tasks

Rbiessy mentioned this pull request Oct 15, 2024

[DOCS]Update new backend guide to support the latest develop #593

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DFT] Initial DFT work & MKLGPU backend #259

[DFT] Initial DFT work & MKLGPU backend #259

hjabird commented Dec 29, 2022 •

edited

Loading

hjabird commented Dec 29, 2022 •

edited

Loading

anantsrivastava30 commented Dec 29, 2022

hjabird commented Dec 30, 2022 •

edited

Loading

mmeterel commented Jan 9, 2023 •

edited

Loading

This comment was marked as resolved.

hjabird commented Jan 13, 2023

mkrainiuk left a comment •

edited

Loading

mmeterel commented Feb 15, 2023

Rbiessy commented Feb 16, 2023

anantsrivastava30 left a comment

lhuot commented Mar 2, 2023

FMarno commented Mar 6, 2023

raphael-egan Mar 10, 2023

anantsrivastava30 Mar 10, 2023

Rbiessy Mar 13, 2023

		set_value_item(descHandle, DFTI_INPUT_DISTANCE, config.fwd_dist);
		set_value_item(descHandle, DFTI_OUTPUT_DISTANCE, config.bwd_dist);

[DFT] Initial DFT work & MKLGPU backend #259

[DFT] Initial DFT work & MKLGPU backend #259

Conversation

hjabird commented Dec 29, 2022 • edited Loading

Description

Checklist

All Submissions

New interfaces

hjabird commented Dec 29, 2022 • edited Loading

anantsrivastava30 commented Dec 29, 2022

hjabird commented Dec 30, 2022 • edited Loading

mmeterel commented Jan 9, 2023 • edited Loading

This comment was marked as resolved.

hjabird commented Jan 13, 2023

mkrainiuk left a comment • edited Loading

Choose a reason for hiding this comment

mmeterel commented Feb 15, 2023

Rbiessy commented Feb 16, 2023

anantsrivastava30 left a comment

Choose a reason for hiding this comment

lhuot commented Mar 2, 2023

FMarno commented Mar 6, 2023

raphael-egan Mar 10, 2023

Choose a reason for hiding this comment

anantsrivastava30 Mar 10, 2023

Choose a reason for hiding this comment

Rbiessy Mar 13, 2023

Choose a reason for hiding this comment

hjabird commented Dec 29, 2022 •

edited

Loading

hjabird commented Dec 29, 2022 •

edited

Loading

hjabird commented Dec 30, 2022 •

edited

Loading

mmeterel commented Jan 9, 2023 •

edited

Loading

mkrainiuk left a comment •

edited

Loading