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

Enable use of execution space instances in ScatterView #3786

Merged
merged 8 commits into from
Feb 16, 2021
Merged

Enable use of execution space instances in ScatterView #3786

merged 8 commits into from
Feb 16, 2021

Conversation

PhilMiller
Copy link
Contributor

Fixes #3785

@dalg24-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@crtrott
Copy link
Member

crtrott commented Feb 10, 2021

I think this is pretty close we need to think about the constructor thing, will discuss this at todays meeting.

@PhilMiller
Copy link
Contributor Author

Any thoughts from the team's meeting on how this should go forward?

dalg24
dalg24 previously requested changes Feb 11, 2021
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.

Removing the old ScatterView::contribute_into(View const&) would;d be a breaking change

I don't think I have permission to push to DARMA-tasking:3785-scatterview-execspace
I pushed to dalg24/3785-scatterview-execspace instead.

DARMA-tasking/kokkos@3785-scatterview-execspace...dalg24:3785-scatterview-execspace

Looking into the constructors now.

containers/src/Kokkos_ScatterView.hpp Outdated Show resolved Hide resolved
containers/src/Kokkos_ScatterView.hpp Outdated Show resolved Hide resolved
containers/src/Kokkos_ScatterView.hpp Outdated Show resolved Hide resolved
@dalg24
Copy link
Member

dalg24 commented Feb 11, 2021

OK to test

PhilMiller
PhilMiller commented Feb 11, 2021
View reviewed changes
Copy link
Contributor Author

@PhilMiller PhilMiller 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 it looks like what we need; I just need to figure out what the calling code would look like now.

containers/src/Kokkos_ScatterView.hpp Show resolved Hide resolved
@dalg24 dalg24 changed the title #3785: ScatterView: Add explicit execution space arguments Enable use of execution space instances in ScatterView Feb 12, 2021
@dalg24 dalg24 marked this pull request as ready for review February 12, 2021 13:56
@dalg24 dalg24 dismissed their stale review February 12, 2021 13:56

Change requested have been addressed

crtrott
crtrott requested changes Feb 12, 2021
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.

Looks good so far. Adding the comment like Daniel asked seems a good idea and we need that simple compile test for both constructor variants working as Damien suggested.

@dalg24 dalg24 requested a review from crtrott February 15, 2021 18:22
DavidPoliakoff
DavidPoliakoff approved these changes Feb 15, 2021
View reviewed changes
Copy link
Contributor

@DavidPoliakoff DavidPoliakoff left a comment

Choose a reason for hiding this comment

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

This has the tests it needs, and the code looks good

masterleinad
masterleinad approved these changes Feb 15, 2021
View reviewed changes
Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@crtrott crtrott merged commit ffd67fc into kokkos:develop Feb 16, 2021
@PhilMiller PhilMiller deleted the 3785-scatterview-execspace branch February 17, 2021 14:38
@PhilMiller PhilMiller mentioned this pull request Mar 30, 2021
Merged
seamill pushed a commit to seamill/Trilinos that referenced this pull request Jul 28, 2021
@dc-snl
Merge kokkos/kokkos#3786 into kokkos
ae209ef 
Need to merge a hotfix from the Kokkos repo into EM-Plasma Trilinos'
kokkos.
seamill pushed a commit to seamill/Trilinos that referenced this pull request Jul 28, 2021
@dc-snl
Merge branch 'hotfix' into develop
b58c085 
* hotfix:
  Merge kokkos/kokkos#3786 into kokkos
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dalg24 dalg24 dalg24 left review comments

@masterleinad masterleinad masterleinad approved these changes

@DavidPoliakoff DavidPoliakoff DavidPoliakoff approved these changes

@crtrott crtrott crtrott approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Developer: Phil Miller
Done in or before release 3.5
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@PhilMiller @dalg24-jenkins @crtrott @dalg24 @masterleinad @DavidPoliakoff