-
Notifications
You must be signed in to change notification settings - Fork 37
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
Update Kokkos version to 4.4.1 #1191
Merged
Merged
Changes from all commits
Commits
Show all changes
27 commits
Select commit
Hold shift + click to select a range
00c936c
bump Kokkos version
jonahm-LANL 0f38624
changelog
jonahm-LANL 86783d4
Update Kokkos version to 4.4.1
b3d926c
Bring in develop; uniformize CHANGELOG
b4ab05f
Fix View of View dealloc
pgrete e333a7b
Move view_alloc to host
pgrete 094880e
Temp disable rocthrust on amd ci container
pgrete 0e0ef22
Fix doc creation. Rollback to ubuntu22-04
pgrete 343b009
Fix more view of views
pgrete 4999a80
Chasing more ViewOfView on host
pgrete c799d02
Fix ViewAlloc func and add doc
pgrete d8b8be8
Merge remote-tracking branch 'origin/develop' into bumo-kokkos-42
pgrete ebbcc17
Disable Ascent in testing
pgrete 11e1e86
Revert "Disable Ascent in testing"
pgrete e632f6b
Disable CUDA IPC
pgrete 120ceef
Fix more view of views
pgrete eb44166
Fix missig update of private var
pgrete eb03328
Make linter happy
pgrete e00017c
Merge branch 'develop' into bumo-kokkos-42
pgrete 0c3ee59
Add ParArray#DRaw and hide Kokkos interface more
pgrete 9f639a6
Add doc
pgrete 4dd0801
Debug build in check compilers without symbols
pgrete 5cf7c4c
Merge remote-tracking branch 'origin/develop' into bumo-kokkos-42
pgrete 4e92f8f
Only check compilers prior to merge
pgrete e8b7c65
Fix typo
pgrete 9c47692
Bump rocm image and change arch for debug hip build
pgrete 9ef4908
Fix comment that broke the command line
pgrete File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Submodule Kokkos
updated
1031 files
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this preferred over just making the inner
View
s unmanaged in aView
ofView
s? My understanding is that the unmanaged view doesn't call the destructors, so doing this would also solve the problem since I don't think we ever make a view of views that actually manages the inner views (maybe this is wrong?). I think it would be trivial to make anUnmanagedParArray#D
by just changing the view type it is templated on. Then we wouldn't have to have raw Kokkos views floating around in the code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I don't know and potentially am afraid of some unknown side effects.
What happens to the reference counter of the existing managed view when we assign it to an unmanaged element of the outer view.
Similarly, for the boundary info and prolong/restrict info outer view that contain objects which implicitly contain
ParArray#D
s. If they're not reference counted, we'd manually have to destruct those, don't we (or how else would the inner object know that they don't need to exist anymore)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the Kokkos docs:
I think this implies that the unmanaged view doesn't increment the reference count. So it is possible that the other view pointing to the same memory deallocates the memory while the unmanaged view still points to it, but the unmanaged view will never try to call the destructor.
I believe that in
BndInfo
and prolong/restrict info we are never creating aParArray#D
that is only held by those objects. Rather, we are essentially "pointing" toParArray#D
s that are held elsewhere (e.g. the communication buffer, variable data). I think the other places those objects persist longer than the packs,BndInfo
, etc. so it is safe to have the internal Views in views of views unmanaged.I think my real concern is that it is unclear (at least to me) what
SequentialHostInit
is doing.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR that added this feature to Kokkos is just a few lines (kokkos/kokkos#7229 excluding the test code).
AFAIK the property effectively tells Kokkos to explicitly call the destructor (line 229 https://github.com/kokkos/kokkos/pull/7229/files#diff-0d719ff2418eb0512b065dca1765d2788ddd4a524734f8f96b7af34a22f4b560) which results in a clean deallocation of the inner view.
Otherwise, the default dtor of the outer view would call the dtor of the inner views in a parallel region, which is illegal.
But maybe I'm also wrong.
But if that's correct, then I somehow feel more comfortable with this way (for the moment) as I understand that concept whereas it's unclear to me if there are any side effects (not necessarily from a Kokkos point of view but from the Parthenon usage point of view) in using unmanaged inner views.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what I didn't understand was that it seemed like I could only set
SequentialHostInit
for host space views of device space views and it seemed like we actually needed this for device space views of device space views. But it is very possible that I just don't understand what is going on.