Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[oneMKL][DFT] Specification for external workspaces for DFTs #509

Merged
merged 12 commits into from
Dec 19, 2023
Merged

[oneMKL][DFT] Specification for external workspaces for DFTs #509

merged 12 commits into from
Dec 19, 2023

Conversation

hjabird
Copy link
Contributor

@hjabird hjabird commented Nov 1, 2023

This PR is paired with #508, and introduces external workspaces to the oneMKL DFT specification.

  • Add support for external workspaces
  • Add the following config_params:
    • WORKSPACE_PLACEMENT
    • WORKSPACE_EXTERNAL_BYTES_REQUIRED
  • Add descriptor::set_workspace
  • Describe typical usage of an external workspace

@hjabird
Copy link
Contributor Author

hjabird commented Nov 1, 2023

Note that this PR does not currently include any feature for estimating the required workspace size.

The usecase for this feature is not completely clear, and cannot be supported by the rocFFT backend in oneMKL interface library. However, it may nonetheless be required.

This was referenced Nov 1, 2023
Merged
Closed
Rbiessy
Rbiessy reviewed Nov 3, 2023
View reviewed changes
Copy link
Contributor

@Rbiessy Rbiessy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall

source/elements/oneMKL/source/domains/dft/descriptor.rst Outdated Show resolved Hide resolved
source/elements/oneMKL/source/domains/dft/descriptor.rst Outdated Show resolved Hide resolved
source/elements/oneMKL/source/domains/dft/config_params/workspace_placement.rst Show resolved Hide resolved
@hjabird
[MKL] Specification for external workspaces
958492a 
* Add support for external workspaces
* Add the following config_params:
  * WORKSPACE_PLACEMENT
  * WORKSPACE_EXTERNAL_BYTES_REQUIRED
* Add descriptor::set_workspace
* Describe typical usage of an external workspace
@hjabird
Romain suggested changes
be1aaf7
@hjabird
Copy link
Contributor Author

hjabird commented Nov 3, 2023

This PR has been rebased upon recent PRs, most importantly #506.

raphael-egan
raphael-egan reviewed Nov 3, 2023
View reviewed changes
source/elements/oneMKL/source/domains/dft/enums_and_config_params.rst Outdated Show resolved Hide resolved
source/elements/oneMKL/source/domains/dft/enums_and_config_params.rst Outdated Show resolved Hide resolved
source/elements/oneMKL/source/domains/dft/descriptor.rst Outdated Show resolved Hide resolved
source/elements/oneMKL/source/domains/dft/descriptor.rst Outdated Show resolved Hide resolved
source/elements/oneMKL/source/domains/dft/enums_and_config_params.rst Outdated Show resolved Hide resolved
source/elements/oneMKL/source/domains/dft/config_params/workspace_placement.rst Outdated Show resolved Hide resolved
source/elements/oneMKL/source/domains/dft/config_params/workspace_placement.rst Outdated Show resolved Hide resolved
source/elements/oneMKL/source/domains/dft/config_params/workspace_placement.rst Outdated Show resolved Hide resolved
source/elements/oneMKL/source/domains/dft/descriptor.rst Outdated Show resolved Hide resolved
source/elements/oneMKL/source/domains/dft/descriptor.rst Outdated Show resolved Hide resolved
@rscohn2
Copy link
Member

rscohn2 commented Nov 7, 2023

Is this ready to merge?

zhiweij1
zhiweij1 reviewed Nov 7, 2023
View reviewed changes
Copy link

@zhiweij1 zhiweij1 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @hjabird, this change LGTM.

@hjabird
Copy link
Contributor Author

hjabird commented Nov 8, 2023

Is this ready to merge?

Hello @rscohn2 , my understanding is that this should be presented to the Math SIG before it can be merged.

@rscohn2
Copy link
Member

rscohn2 commented Nov 8, 2023

@spencerpatty: Was this PR targeted for 1.3 release?

@spencerpatty
Copy link
Contributor

@spencerpatty: Was this PR targeted for 1.3 release?

@rscohn2 no, I discussed with the authors and they are not targeting 1.3 release.

@hjabird hjabird mentioned this pull request Nov 17, 2023
Merged
7 tasks
@hjabird
Copy link
Contributor Author

hjabird commented Dec 5, 2023

Following the Math SIG, I tried to find examples uses of cufft workspace estimates on Github. Based on a small number of samples:

  • cufftestimate* when used, often appears to have been used incorrectly. Users assume treat the value obtained as a conservative estimate, when it may return a smaller than the actual required work area.
  • In many (most?) cases, the workspace requirement is obtained during plan creation using the cufftMakePlan* set of functions.

I don't think that pre-commit workspace size estimates are needed in this spec PR.

@hjabird
Copy link
Contributor Author

hjabird commented Dec 5, 2023

@lhuot @Rbiessy I believe this is now ready to merge. Having not been available for the Math SIG meeting, is there anything you think I've missed?

Rbiessy
Rbiessy approved these changes Dec 5, 2023
View reviewed changes
Copy link
Contributor

@Rbiessy Rbiessy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ready as well.

@hjabird
Copy link
Contributor Author

hjabird commented Dec 19, 2023

@rscohn2 I believe this is ready to merge. It has approval from both the Codeplay and Intel FastFour team, and has been presented at the Math SIG. I don't have the capability to merge this myself.

@rscohn2 rscohn2 merged commit 276e0ef into uxlfoundation:main Dec 19, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raphael-egan raphael-egan raphael-egan left review comments

@zhiweij1 zhiweij1 zhiweij1 left review comments

@lhuot lhuot lhuot left review comments

@dbollapr dbollapr dbollapr approved these changes

@Rbiessy Rbiessy Rbiessy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@hjabird @rscohn2 @spencerpatty @Rbiessy @lhuot @dbollapr @zhiweij1 @raphael-egan