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

Logical Memory Spaces #3546

Merged
merged 15 commits into from
Nov 12, 2020
Merged

Logical Memory Spaces #3546

merged 15 commits into from
Nov 12, 2020

Conversation

DavidPoliakoff
Copy link
Contributor

@DavidPoliakoff DavidPoliakoff commented Oct 29, 2020 •
edited

I'm putting this Draft PR up so everybody (especially @dalg24 and @crtrott) can familiarize themselves with the design. Things I know need to happen

  1. Put this in Experimental (done)
  2. Actual tests (done)
  3. make it work with things other than HostSpace (they just need a friend declaration available) (edit: done)
  4. Delegate to an impl allocate/deallocate method to allow specifying a SpaceHandle for MemorySpaces #3530 needs to merge (done)

But this should help you guys see the point behind this design. The only files that matter are core/src/LogicalSpaces.hpp and core/unit_test/tools/TestLogicalMemorySpaces.cpp, which I'll likely expand in a few future commits

@DavidPoliakoff DavidPoliakoff marked this pull request as ready for review October 29, 2020 18:27
@dalg24 dalg24 added this to the Tentative 3.4 Release milestone Nov 4, 2020
@DavidPoliakoff
Copy link
Contributor Author

Retest this please

dalg24
dalg24 reviewed Nov 6, 2020
View reviewed changes
core/unit_test/tools/TestLogicalSpaces.hpp Outdated Show resolved Hide resolved
core/unit_test/tools/TestLogicalSpaces.hpp Show resolved Hide resolved
core/unit_test/tools/TestLogicalSpaces.hpp Outdated Show resolved Hide resolved
core/src/Kokkos_LogicalSpaces.hpp Outdated Show resolved Hide resolved
core/src/Kokkos_LogicalSpaces.hpp Outdated Show resolved Hide resolved
core/src/Kokkos_LogicalSpaces.hpp Outdated Show resolved Hide resolved
core/src/Kokkos_LogicalSpaces.hpp Outdated Show resolved Hide resolved
core/src/Kokkos_LogicalSpaces.hpp Outdated Show resolved Hide resolved
core/src/Kokkos_LogicalSpaces.hpp Outdated Show resolved Hide resolved
core/unit_test/tools/TestLogicalSpaces.hpp Show resolved Hide resolved
@DavidPoliakoff
Copy link
Contributor Author

Retest this please

dalg24
dalg24 reviewed Nov 9, 2020
View reviewed changes
core/src/Kokkos_LogicalSpaces.hpp Outdated Show resolved Hide resolved
core/src/Kokkos_LogicalSpaces.hpp Outdated Show resolved Hide resolved
core/src/Kokkos_LogicalSpaces.hpp Outdated Show resolved Hide resolved
core/src/Kokkos_LogicalSpaces.hpp Outdated Show resolved Hide resolved
core/src/Kokkos_LogicalSpaces.hpp Outdated Show resolved Hide resolved
core/src/Kokkos_LogicalSpaces.hpp Show resolved Hide resolved
core/src/Kokkos_LogicalSpaces.hpp Outdated Show resolved Hide resolved
core/src/Kokkos_LogicalSpaces.hpp Outdated Show resolved Hide resolved
core/src/Kokkos_LogicalSpaces.hpp Show resolved Hide resolved
core/unit_test/tools/TestLogicalSpaces.hpp Show resolved Hide resolved
@DavidPoliakoff
Copy link
Contributor Author

Retest this please

crtrott
crtrott requested changes Nov 10, 2020
View reviewed changes
Copy link
Member

@crtrott crtrott left a comment

Choose a reason for hiding this comment

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

I am basically good with this. The only thing is the unfortunate naming of the template parameter DefaultExecutionSpace which shadows a type in the Kokkos namespace.

nliber
nliber approved these changes Nov 10, 2020
View reviewed changes
core/src/Kokkos_LogicalSpaces.hpp Outdated Show resolved Hide resolved
core/unit_test/tools/TestLogicalSpaces.hpp Show resolved Hide resolved
dalg24
dalg24 requested changes Nov 11, 2020
View reviewed changes
Copy link
Member

@dalg24 dalg24 left a comment •
edited

Choose a reason for hiding this comment

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

All <Kokkos_FooSpace.hpp> headers seem to include <Kokkos_Core_fwd.hpp>. Please move the forward declaration there.

@DavidPoliakoff
Copy link
Contributor Author

@dalg24, handled via 4e8c77d

dalg24
dalg24 reviewed Nov 11, 2020
View reviewed changes
Copy link
Member

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

Drop the outdated comment. Looks good to me other than that.

core/src/Kokkos_HIP_Space.hpp Outdated Show resolved Hide resolved
@crtrott crtrott merged commit dad5262 into kokkos:develop Nov 12, 2020
masterleinad added a commit to masterleinad/kokkos that referenced this pull request Nov 17, 2020
@masterleinad
Revert "Merge pull request kokkos#3546 from DavidPoliakoff/feature/lo…
3551067 
…gical-memspaces"

This reverts commit dad5262, reversing
changes made to 5883096.
@PhilMiller PhilMiller mentioned this pull request Jun 8, 2021
Merged
@dalg24 dalg24 mentioned this pull request Oct 17, 2022
Merged
@dalg24 dalg24 mentioned this pull request Nov 1, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhollman dhollman dhollman left review comments

@PhilMiller PhilMiller PhilMiller left review comments

@masterleinad masterleinad masterleinad left review comments

@dalg24 dalg24 dalg24 approved these changes

@nliber nliber nliber approved these changes

@crtrott crtrott crtrott approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Tentative 3.3 Release
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@DavidPoliakoff @dhollman @PhilMiller @dalg24 @nliber @masterleinad @crtrott