[DFT] Add FWD/BWD_STRIDES to public API, deprecate INPUT/OUTPUT_STRIDES #528

hjabird · 2024-07-05T13:20:34Z

Description

Adds the FWD/BWD_STRIDES API from the interface spec
Deprecates old INPUT/OUTPUT_STRIDES using the deprecated attribute.
Modifies DFT tests to use FWD/BWD_STRIDES API
- The old API is no longer thoroughly tested
Adds additional descriptor tests

Fixes #488

Checklist

All Submissions

Do all unit tests pass locally? Attach a log.
- amdtest.txt
- intel.txt
- nvidiadft.txt
- portfft2.txt
  - PortFFT has failures that also exist in develop.
Have you formatted the code using clang-format?

New interfaces

What version of oneAPI spec the interface is targeted?
- Feature from uxlfoundation/oneAPI-spec@3717a9a

New features

Have you provided motivation for adding a new feature?
Have you added relevant tests?
- Tests updated to use new functionality

* Adds the FWD/BWD_STRIDES API from the interface spec * Deprecates old INPUT/OUTPUT_STRIDES using the deprecated attribute. * Modifies DFT tests to use FWD/BWD_STRIDES API * The old API is no longer thoroughly tested * Adds additional descriptor tests

hjabird · 2024-07-05T19:17:32Z

Pinging @lhuot for DFT changes.
I'll be addressing comments from the week of the 15th.

Rbiessy

Looks good to me! The strides in cuFFT are pretty difficult to get its head around but that was already the case before... We have pretty good tests so this should be fine!

anantsrivastava30

Reviewed the MKLGPU/MKLCPU part of addition and deprication, looks good to me, only thing I was mulling over was the get_strides_api, if in/out_strides are set then fwd/bwd_strides (config_value) must be zero, (which is a internal property of the DftiSetValue (in cpu codebase), set_value(gpu codebase)). But here you are checking config_value.fwd_strides is zero or not! Are we explicitly setting fwd_strides to zero in this PR when we set input_strides or vice-versa.

anantsrivastava30

Approved, can have this #528 (review) discussing without blocking.

hjabird · 2024-07-17T19:24:45Z

Reviewed the MKLGPU/MKLCPU part of addition and deprication, looks good to me, only thing I was mulling over was the get_strides_api, if in/out_strides are set then fwd/bwd_strides (config_value) must be zero, (which is a internal property of the DftiSetValue (in cpu codebase), set_value(gpu codebase)). But here you are checking config_value.fwd_strides is zero or not! Are we explicitly setting fwd_strides to zero in this PR when we set input_strides or vice-versa.

In this PR we set fwd/bwd_strides to zero on setting input/output_strides, and vice versa.
- Tested here and here.
We check if config_value.fwd_strides is zero when trying to determine which stride API was used.
- If no strides have been set, the input/output strides and fwd/bwd strides will contain identical values, so it doesn't matter which set of strides gets used.
- If the input/output API was used, then these will have been zeroed. It will therefore test for the usage of the input/output stride API.

raphael-egan

My review is coming a little late but I'd appreciate it if one could give it consideration.

src/dft/descriptor.cxx

src/dft/backends/mklcpu/mklcpu_helpers.hpp

src/dft/backends/rocfft/commit.cpp

src/dft/backends/cufft/commit.cpp

raphael-egan · 2024-07-18T16:26:18Z

src/dft/backends/cufft/commit.cpp

+            for (int r = 1; r < rank; ++r) {
+                valid = valid && (inner_nfwd <= inner_sfwd) && (inner_nbwd <= inner_sbwd);
+                inner_nfwd *= n_copy[rank - r - 1];
+                inner_nbwd *= n_copy[rank - r - 1];
+                inner_sfwd *= strides_fwd[rank - r - 1];
+                inner_sbwd *= strides_bwd[rank - r - 1];
            }


Does cuFFT support negative stride values? If yes, wouldn't these checks be invalid with negative strides?

I think this is correct.

For CUFFT, strides_fwd and strides_bwd correspond to inembed and onembed, where

n[i] ≤ inembed[i], n[i] ≤ onembed[i]

The strides can be negative, if you set a_stride and b_stride negative.

I see. So this particular logic is likely adequate w.r.t. the (i|o)nembed values but there are other bits (determining the (i|o)nembed) that might need revision before we reach this point then, in my opinion. For instance, a_min and b_min would be the "most negative" value as of now (not the minimal stride in absolute value) which is not what we'd want in this case, unless I'm wrong. This is rather orthogonal to the task targeted herein though, I just wanted to point out a possible concern.

Rbiessy · 2024-07-18T16:41:49Z

I am so sorry @raphael-egan I forgot you were also planning to review it! I merged it as Hugh was going on holiday. I am sure he will be able to address your comments once he is back.

raphael-egan · 2024-07-18T16:43:20Z

I am so sorry @raphael-egan I forgot you were also planning to review it! I merged it as Hugh was going on holiday. I am sure he will be able to address your comments once he is back.

No problem, I'm sure he will.

…ES (oneapi-src#528)

hjabird added 2 commits July 5, 2024 14:13

PortFFT reversed in/out

2bab1d5

hjabird marked this pull request as ready for review July 5, 2024 19:13

Rbiessy approved these changes Jul 12, 2024

View reviewed changes

mkrainiuk requested a review from a team July 15, 2024 19:18

anantsrivastava30 reviewed Jul 17, 2024

View reviewed changes

This comment was marked as duplicate.

Sign in to view

anantsrivastava30 approved these changes Jul 17, 2024

View reviewed changes

Rbiessy merged commit f2d2dcb into oneapi-src:develop Jul 18, 2024
6 checks passed

raphael-egan reviewed Jul 18, 2024

View reviewed changes

normallytangent pushed a commit to normallytangent/oneMKL that referenced this pull request Aug 6, 2024

[DFT] Add FWD/BWD_STRIDES to public API, deprecate INPUT/OUTPUT_STRID…

78fac71

…ES (oneapi-src#528)

hjabird mentioned this pull request Aug 7, 2024

[DFT] Corrections to FWD/BWD_STRIDES API #549

Merged

2 tasks

hjabird mentioned this pull request Aug 19, 2024

[ROCFFT] RocFFT fails tests when using ROCm 6.0 or later #559

Open

s-Nick mentioned this pull request Aug 27, 2024

[DFT][portFFT] Update GIT_TAG for portFFT download #560

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DFT] Add FWD/BWD_STRIDES to public API, deprecate INPUT/OUTPUT_STRIDES #528

[DFT] Add FWD/BWD_STRIDES to public API, deprecate INPUT/OUTPUT_STRIDES #528

hjabird commented Jul 5, 2024 •

edited

Loading

hjabird commented Jul 5, 2024

Rbiessy left a comment

anantsrivastava30 left a comment

This comment was marked as duplicate.

anantsrivastava30 left a comment

hjabird commented Jul 17, 2024

raphael-egan left a comment

raphael-egan Jul 18, 2024

hjabird Jul 29, 2024

raphael-egan Jul 29, 2024

Rbiessy commented Jul 18, 2024

raphael-egan commented Jul 18, 2024

[DFT] Add FWD/BWD_STRIDES to public API, deprecate INPUT/OUTPUT_STRIDES #528

[DFT] Add FWD/BWD_STRIDES to public API, deprecate INPUT/OUTPUT_STRIDES #528

Conversation

hjabird commented Jul 5, 2024 • edited Loading

Description

Checklist

All Submissions

New interfaces

New features

hjabird commented Jul 5, 2024

Rbiessy left a comment

Choose a reason for hiding this comment

anantsrivastava30 left a comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

anantsrivastava30 left a comment

Choose a reason for hiding this comment

hjabird commented Jul 17, 2024

raphael-egan left a comment

Choose a reason for hiding this comment

raphael-egan Jul 18, 2024

Choose a reason for hiding this comment

hjabird Jul 29, 2024

Choose a reason for hiding this comment

raphael-egan Jul 29, 2024

Choose a reason for hiding this comment

Rbiessy commented Jul 18, 2024

raphael-egan commented Jul 18, 2024

hjabird commented Jul 5, 2024 •

edited

Loading