-
Notifications
You must be signed in to change notification settings - Fork 10
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
Changes supporting GEOSX Trilinos/Tpetra wrapper #189
base: develop
Are you sure you want to change the base?
Conversation
Note: code in GEOSX that uses this is currently disabled behind an |
src/ChaiBuffer.hpp
Outdated
*/ | ||
LVARRAY_HOST_DEVICE inline constexpr | ||
T * data( MemorySpace const space ) const | ||
{ return static_cast< T * >( m_pointer_record->m_pointers[ internal::toChaiExecutionSpace( space ) ] ); } |
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.
Is this sort of error prone? by this I mean that it seems likely that we can get a pointer that is not current? It there an underlying check/move?
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 code that uses this in GEOSX calls a move before. Would you rather have a call to move
embedded in here? This sort of makes synchronization unavoidable, and I'm not sure we always want it.
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'm fine with this for the use case you describe above you don't want to move the data before getting the pointers our. Obviously you could really shoot yourself but this should only be used in a few places.
Could you
- Add a check to error out if the array wasn't allocated in that space
m_pointer_record->m_pointers[ space ] == nullptr
? - Add the following the
MallocBuffer
andStackBuffer
?
T * data( MemorySpace const space ) const
{
LVARRAY_ERROR_IF_NE( space, MemorySpace::CPU );
return data();
}
- Add a simple test to
testArrayView
.
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.
@corbett5 done
Do we need to update Trilinos? |
No, there's no new version available. And it's not something they seem to have fixed yet. All Tpetra build instructions, FAQs, etc. explicitly require configuring Kokkos to use UM as default memory space for CUDA. |
🤦 |
We can always just allocate the matrix with unified memory, it would be pretty easy to add that support. |
72c0f88
to
f142b04
Compare
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.
Thanks!
f142b04
to
deb79fb
Compare
9c98e61
to
5daf329
Compare
5daf329
to
95db802
Compare
This PR adds a simple function in
ArrayView
(andNewChaiBuffer
) to get the data pointer in a specificMemorySpace
, whether most current or not.While this should not be needed/used by most developers, Tpetra implements dual memory space semantics, meaning that
Tpetra::Vector
for example contains pointers in both host and device memory spaces, and can potentially move data between them as needed. Therefore, in order to support the zero-copy creation of parallel vectors fromArrayView
of local values, we have to be able to deliver pointers in both memory spaces.Related to GEOS-DEV/GEOS#1086