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.

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

Desul: Add ScopeCaller #4690

Merged
merged 3 commits into from
Feb 1, 2022
Merged

Desul: Add ScopeCaller #4690

merged 3 commits into from
Feb 1, 2022

Conversation

crtrott
Copy link
Member

@crtrott crtrott commented Jan 19, 2022

In Serial only builds Kokkos::atomics will use ScopeCaller

@crtrott
Copy link
Member Author

crtrott commented Jan 19, 2022

Corresponding PR in Desul: desul/desul#50

Note I applied desul clang-format too

@crtrott crtrott added the Blocks Promotion Overview issue for release-blocking bugs label Jan 19, 2022
@crtrott crtrott added this to In progress in Kokkos Release 3.6 via automation Jan 19, 2022
@crtrott crtrott moved this from In progress to Awaiting Feedback in Kokkos Release 3.6 Jan 19, 2022
In Serial only builds Kokkos::atomics will use ScopeCaller
@crtrott crtrott force-pushed the desul-caller-scope2 branch 3 times, most recently from d3367ec to 16b098a Compare January 21, 2022 18:07
@crtrott crtrott force-pushed the desul-caller-scope2 branch 2 times, most recently from 1cc02c6 to 2934794 Compare January 25, 2022 17:54
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.

Would you do the formatting part in a separate PR?

Comment on lines 37 to 38
// error: large atomic operation may incur significant performance penalty
// [-Werror,-Watomic-alignment] https://godbolt.org/z/G7YhqhbG6
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// error: large atomic operation may incur significant performance penalty
// [-Werror,-Watomic-alignment] https://godbolt.org/z/G7YhqhbG6
// clang-format off
// error: large atomic operation may incur significant performance penalty [-Werror,-Watomic-alignment]
// clang-format on
// https://godbolt.org/z/G7YhqhbG6

Uggg. Did we merge that into Desul?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah

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.

I'd prefer if you submitted the changes to <desul/atomics/Generic.hpp> in another PR

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 OK to me (despite that the name feels unintuitive at least for the current use case to me). We at least have the Windows build and the OSX builds to test this.

@dalg24
Copy link
Member

dalg24 commented Feb 1, 2022

I'll cut you a deal. I close my eyes and merge this PR as is and you agree to split up your PR on the Desul side.
What do you say?

@crtrott
Copy link
Member Author

crtrott commented Feb 1, 2022

Sure Damien :-)

@dalg24 dalg24 merged commit a1b499f into kokkos:develop Feb 1, 2022
Kokkos Release 3.6 automation moved this from Awaiting Feedback to Done Feb 1, 2022
@ajpowelsnl
Copy link
Contributor

Morning @dalg24 and @crtrott -- is this issue still a blocker (given that it's been merged)?

@dalg24 dalg24 removed the Blocks Promotion Overview issue for release-blocking bugs label Feb 8, 2022
@ajpowelsnl
Copy link
Contributor

Good morning @dalg24 and @crtrott -- Can we close this issue?

@dalg24
Copy link
Member

dalg24 commented Feb 21, 2022

This is a pull request not an issue and it has been merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants