[DFT] Synced mklcpu backend + Real_Real tests by anantsrivastava30 · Pull Request #288 · uxlfoundation/oneMath

anantsrivastava30 · 2023-03-06T16:42:34Z

Description

This PR enables the mkl_cpu backend for the FFT domain to be introduced in #259. This enables the DFT domain to support CPU device conforming to the Spec OneAPI.
Supports COMPLEX_STORAGE=COMPLEX_COMPLEX, REAL_REAL for COMPLEX transforms;

REAL STORAGE is not used anymore and instead CONJUGATE_EVEN_STORAGE is used for REAL transforms,

Tested with:

1D {complex/real} {sp/dp} {split complex} Add unit-tests for the dft interface #261
mD, many-batched (WIP) [DFT] Multi-Dimension Many-Batch DFT tests #275

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)?

anantsrivastava30 · 2023-03-06T17:09:12Z

CPU descriptor and compute tests :
CPUTest.log

src/dft/backends/mklcpu/commit.cpp

README.md

examples/dft/compile_time_dispatching/complex_fwd_buffer_mklcpu.cpp

src/dft/backends/mklcpu/mklcpu_helpers.hpp

src/dft/backends/mklcpu/backward.cpp

hjabird · 2023-03-14T10:31:08Z

You're missing something similar to compute_signatures.cpp in the MKLGPU backend.

src/dft/backends/mklcpu/commit.cpp

tests/unit_tests/dft/include/compute_inplace_real_real.hpp

tests/unit_tests/dft/include/compute_out_of_place_real_real.hpp

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

src/dft/backends/mklcpu/commit.cpp

…ynced_mklcpu_backend

src/dft/backends/mklcpu/commit.cpp

src/dft/backends/mklcpu/mklcpu_helpers.hpp

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

tests/unit_tests/dft/include/compute_out_of_place_real_real.hpp

tests/unit_tests/dft/include/compute_inplace_real_real.hpp

anantsrivastava30 · 2023-03-31T21:41:48Z

You're missing something similar to compute_signatures.cpp in the MKLGPU backend.

so compute signature is alreday a part of this work and was introduced in the previous PR's that have been merged.

…stava30/oneMKL into synced_mklcpu_backend

src/dft/backends/mklcpu/mklcpu_helpers.hpp

src/dft/backends/mklcpu/commit.cpp

FMarno · 2023-04-20T10:31:36Z

src/dft/backends/mklcpu/commit.cpp

+                MKL_LONG status[2] = { DFTI_BAD_DESCRIPTOR };
+
+                for (auto dir : { DIR::fwd, DIR::bwd })
+                    status[dir] = DftiCommitDescriptor(bidir_handle_obj[0][dir]);
+
+                if (status[0] != DFTI_NO_ERROR && status[1] != DFTI_NO_ERROR) {
+                    std::string err = std::string("DftiCommitDescriptor failed with status : ") +
+                                      DftiErrorMessage(status[0]) + std::string(", ") +
+                                      DftiErrorMessage(status[1]);
+                    throw oneapi::mkl::exception("dft/backends/mklcpu", "commit", err);
+                }


Suggested change

MKL_LONG status[2] = { DFTI_BAD_DESCRIPTOR };

for (auto dir : { DIR::fwd, DIR::bwd })

status[dir] = DftiCommitDescriptor(bidir_handle_obj[0][dir]);

if (status[0] != DFTI_NO_ERROR && status[1] != DFTI_NO_ERROR) {

std::string err = std::string("DftiCommitDescriptor failed with status : ") +

DftiErrorMessage(status[0]) + std::string(", ") +

DftiErrorMessage(status[1]);

throw oneapi::mkl::exception("dft/backends/mklcpu", "commit", err);

}

for (auto dir : { DIR::fwd, DIR::bwd }) {

auto status = DftiCommitDescriptor(bidir_handle_obj[0][dir]);

if (status != DFTI_NO_ERROR) {

std::string err = std::string("DftiCommitDescriptor failed with status : ") +

DftiErrorMessage(status);

throw oneapi::mkl::exception("dft/backends/mklcpu", "commit", err);

}

}

I'm not sure we should continue trying after the first one failed

no this cannot be done, as in case of real batched transformation, where resetting strides are essential, the 2nd descriptor would be of inconsistent configuration (because of the strides) and would error out.
the first one can be used for compute_forward, then we recommit and reset the strides, which invalidates the first descriptor and commits the second one.

@hjabird the reason we can't use the same logic to reset the strides is to maintain consistency with the gpu backend (which as it does not have 2 different descriptors does not have that capability)

It is a tricky issue.

Best I can suggest is that if 1 of the 2 fail, we save the exception and throw it if the user tries to use that. I'm happy for that to be another PR though.

That is a very awkward problem. Please add a comment describing this to your solution and also remember to revert the change from && to || if needed.

I think there is a similar issue in the cuFFT backend (I added a comment there) and forcing the two descriptors to be valid at the same time might result in failure to commit for valid real FFT problems.

One idea, @raphael-egan, @hparks-intel please let me know if you think of a case this would not work.
Can we try to commit the fwd descriptor with the given input/output stride, if successful commit the bwd descriptor with swapped input/output strides and check that both descriptor are valid?
Similarly if the first commit for forward descriptor fails, try again to commit fwd descriptor with swapped input/output strides and commit backward descriptor with original input/output stride. Fail to commit if both descriptors aren't valid?

Also I agree this might be better addressed in a separate PR.

We should definitely address this discrepancy in the oneMKL SYCL specification to have fwd and bwd strides instead of input and output stride.

I think it might be a bad idea to swap the input/output strides since they may be designed for one direction but valid in both (NB I'm not 100% sure of this since it's 6:30pm on a Friday). I think a scenario where the data is densely packed in the backward domain but has spaces between the data in the complex domain might do it.

@lhuot from my reading, you don't want any changes now so I will resolve this thread.

This is indeed a fairly awkward issue that underlines that there is room for major improvement to the specs' and product's APIs: the possibility of having a single set of configuration parameters unambiguously usable for either compute direction would require the {IN,OUT}PUT_STRIDES configuration parameters to be deprecated for the SYCL APIs and substituted by (new, yet-to-be-approved-and-implemented) {F,B}WD_STRIDES parameters. This is suggested in MKLD-15060.

As @FMarno points out, there might be cases where either direction is valid with sufficient padding. Consider a 4x4 real transforms with CCE storage, with input strides {5, 1, 0} and output strides {4,. 1, 0}, the user may be configuring the descriptor for a rfo4:5:4:x4:1:1 (padding of 1 real data point in fwd domain and 1 complex data point in bwd domain) or for a rbo4:5:4:x4:1:1 (padding of 2 complex data points in bwd domain, no padding in fwd domain) and either would be valid. Therefore, swapping strides for the bwd descriptor if the fwd descriptor committed successfully would result in incorrectness if the user actually meant to target rbo4:5:4x4:1:1 in that example.

In my personal opinion, it is best to not swap the strides and allow for one (not both!) of the descriptors to fail to commit and check thereafter that it is never used if it didn't commit successfully prior.

src/dft/backends/mklcpu/backward.cpp

examples/dft/compile_time_dispatching/CMakeLists.txt

hjabird

Re-reviewing since there has been a lot of change since the original review.

Only minor comments/questions!

examples/dft/compile_time_dispatching/complex_fwd_buffer_mklcpu.cpp

src/dft/backends/mklcpu/backward.cpp

src/dft/backends/mklcpu/commit.cpp

src/dft/backends/mklcpu/forward.cpp

src/dft/backends/mklcpu/mklcpu_helpers.hpp

tests/unit_tests/dft/include/compute_inplace.hpp

tests/unit_tests/dft/source/descriptor_tests.cpp

hjabird

There are quite a few trivial compiler warnings that I think should be sorted out. I've only highlighted one here.

src/dft/backends/mklcpu/commit.cpp

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

examples/dft/compile_time_dispatching/CMakeLists.txt

FMarno · 2023-04-26T11:07:11Z

examples/dft/compile_time_dispatching/CMakeLists.txt

-    list(APPEND ONEMKL_LIBRARIES_${domain} onemkl_${domain}_mklgpu)
+
+  set(ONEMKL_LIBRARIES_${domain} "")
+  if(domain STREQUAL "dft")


is domain ever not dft?
That applies to a lot of this file.
Otherwise it looks a lot better :)

run_example.sh

src/dft/backends/mklcpu/mklcpu_helpers.hpp

FMarno · 2023-04-27T09:49:45Z

@anantsrivastava30 can you address the merge conflict please.

src/dft/backends/mklcpu/commit.cpp

raphael-egan

THanks

FMarno

LGTM. Can you attach a test log and format these files before merging please:
src/dft/backends/mklcpu/backward.cpp
src/dft/backends/mklcpu/commit.cpp
src/dft/backends/mklcpu/commit_derived_impl.hpp
src/dft/backends/mklcpu/forward.cpp

* FFT_MKLCPU_staging + sync uxlfoundation#261 * fix real_real failures * tests for CPU backend * [DFT] fix rebase issues * [DFT] address reviews * command group handles only the DftiCommitDescriptor * set the distance to true default * fix testing issue with complex_complex * change real_real test to work for md, batch transforms * adderssing minor comments * add error messages for ordering, transpose, workspace * change error message from cpu to gpu * move the desc_buffer to the commit class to avoid descructor invoke * refactor forward declared derived class * extend buffer lifetime for backward * add error messaging for compute * patchfix for transform in/out distances to fwd/bwd * revert resetting dist in roundtrip * revert tests to match specs * clang-format * fix ct adding both backends to the linkline * FFT_MKLCPU_staging + sync uxlfoundation#261 * fix real_real failures * tests for CPU backend * [DFT] fix rebase issues * [DFT] address reviews * minor changes and error checks * command group handles only the DftiCommitDescriptor * move queue to pass by reference * set the distance to true default * fix testing issue with complex_complex * change real_real test to work for md, batch transforms * adderssing minor comments * add error messages for ordering, transpose, workspace * change error message from cpu to gpu * move the desc_buffer to the commit class to avoid descructor invoke * refactor forward declared derived class * extend buffer lifetime for backward * add error messaging for compute * patchfix for transform in/out distances to fwd/bwd * revert resetting dist in roundtrip * revert tests to match specs * fix ct adding both backends to the linkline * address reviews * move the derived class to its own file * address warnings, and change buffer type * Update src/dft/backends/mklcpu/commit.cpp * resovle dynamic dispatch issue * merge cmake for both device * remove bad file * removed old comment * clang-format

anantsrivastava30 mentioned this pull request Mar 6, 2023

[FFT] mklcpu backned #265

Closed

9 tasks

anantsrivastava30 changed the title ~~Synced mklcpu backend~~ [DFT] Synced mklcpu backend + Real_Real tests Mar 6, 2023

anantsrivastava30 mentioned this pull request Mar 10, 2023

[DFT] Initial DFT work & MKLGPU backend #259

Merged

1 task

anantsrivastava30 commented Mar 10, 2023

View reviewed changes

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

anantsrivastava30 added 3 commits March 13, 2023 08:34

FFT_MKLCPU_staging + sync uxlfoundation#261

68a98b1

fix real_real failures

64cfbd1

tests for CPU backend

1150033

anantsrivastava30 force-pushed the synced_mklcpu_backend branch from eb716f2 to 1150033 Compare March 13, 2023 15:44

[DFT] fix rebase issues

cb2655d

hjabird reviewed Mar 14, 2023

View reviewed changes

FMarno reviewed Mar 14, 2023

View reviewed changes

hjabird reviewed Mar 14, 2023

View reviewed changes

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

anantsrivastava30 added 2 commits March 14, 2023 09:40

Merge branch 'develop' of https://github.com/oneapi-src/oneMKL into s…

66a1aad

…ynced_mklcpu_backend

[DFT] address reviews

f09b61e

FMarno reviewed Mar 15, 2023

View reviewed changes

FMarno mentioned this pull request Mar 31, 2023

[DFT] Disable the MKLCPU DFT backend #301

Closed

anantsrivastava30 added 10 commits April 2, 2023 13:18

address reviews

9d08e22

sync w/ head

9ca597a

minor changes and error checks

04b0894

command group handles only the DftiCommitDescriptor

963670d

move queue to pass by reference

27e8af5

set the distance to true default

fbdf123

fix testing issue with complex_complex

08cdf26

change real_real test to work for md, batch transforms

36af25c

adderssing minor comments

1e725f2

add error messages for ordering, transpose, workspace

2e14329

anantsrivastava30 added 4 commits April 20, 2023 03:09

clang-format

859ff4b

fix ct adding both backends to the linkline

b056587

add line

728f6e2

Merge branch 'synced_mklcpu_backend' of https://github.com/anantsriva…

8eb616f

…stava30/oneMKL into synced_mklcpu_backend

FMarno reviewed Apr 20, 2023

View reviewed changes

hjabird reviewed Apr 20, 2023

View reviewed changes

anantsrivastava30 added 2 commits April 20, 2023 05:07

address reviews

86c1ec9

move the derived class to its own file

092ba93

hjabird reviewed Apr 20, 2023

View reviewed changes

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

address warnings, and change buffer type

89edd4d

FMarno reviewed Apr 20, 2023

View reviewed changes

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

anantsrivastava30 and others added 2 commits April 20, 2023 10:11

Update src/dft/backends/mklcpu/commit.cpp

0f0bac0

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

resovle dynamic dispatch issue

b690dad

mkrainiuk reviewed Apr 26, 2023

View reviewed changes

examples/dft/compile_time_dispatching/CMakeLists.txt Outdated Show resolved Hide resolved

merge cmake for both device

0463106

FMarno reviewed Apr 26, 2023

View reviewed changes

run_example.sh Outdated Show resolved Hide resolved

FMarno reviewed Apr 26, 2023

View reviewed changes

src/dft/backends/mklcpu/mklcpu_helpers.hpp Outdated Show resolved Hide resolved

anantsrivastava30 added 2 commits April 26, 2023 10:18

remove bad file

a118caf

removed old comment

826bb63

raphael-egan reviewed Apr 27, 2023

View reviewed changes

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

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

Merge branch 'develop' into synced_mklcpu_backend

8f63032

raphael-egan approved these changes Apr 27, 2023

View reviewed changes

FMarno approved these changes Apr 28, 2023

View reviewed changes

hjabird approved these changes Apr 28, 2023

View reviewed changes

clang-format

ba3146d

anantsrivastava30 merged commit 4c7e7eb into uxlfoundation:develop Apr 28, 2023

FMarno mentioned this pull request May 1, 2023

[DFT] Rearrange DFT compute tests so unimplemented always skips #311

Merged

4 tasks

Comments

Conversation

anantsrivastava30 commented Mar 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

All Submissions

New interfaces

New features

Bug fixes

Uh oh!

anantsrivastava30 commented Mar 6, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hjabird commented Mar 14, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anantsrivastava30 commented Mar 31, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

FMarno Apr 20, 2023

Choose a reason for hiding this comment

Uh oh!

anantsrivastava30 Apr 20, 2023 • 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!

hjabird Apr 20, 2023 • 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!

FMarno Apr 20, 2023

Choose a reason for hiding this comment

Uh oh!

lhuot Apr 21, 2023 • 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!

FMarno Apr 21, 2023

Choose a reason for hiding this comment

Uh oh!

FMarno Apr 26, 2023

Choose a reason for hiding this comment

Uh oh!

anantsrivastava30 commented Mar 6, 2023 •

edited

Loading

anantsrivastava30 Apr 20, 2023 •

edited

Loading

hjabird Apr 20, 2023 •

edited

Loading

lhuot Apr 21, 2023 •

edited

Loading