-
Notifications
You must be signed in to change notification settings - Fork 62
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
Pcu object #388
base: develop
Are you sure you want to change the base?
Pcu object #388
Conversation
This commit adds a MPI state that's separate from the global variables to the internal pcu_mpi interfaces. The user level PCU interface remains unchanged. However, this change lays the groundwork for PCU2 that will not make use of global state. This is important for some use cases like working around the spectrum-mpi comm_dup bug. The removal of global state also improves testability and modularity of this code.
Throughout the code base it was assumed that the mpi functions were implemented through pmpi and so no other implementation really could have been implemented. This change just removes the complexity from having the vtable.
This commit allows the pcu_mpi to be initialized into a stack variable rather than allways allocating on the heap.
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.
Thank you. Nice work.
I just noticed this was a draft. What is left to do? It looks like CI tests are passing.
pcu/CMakeLists.txt
Outdated
@@ -9,7 +9,7 @@ message(STATUS "PCU_COMPRESS: " ${PCU_COMPRESS}) | |||
|
|||
# Package sources | |||
set(SOURCES | |||
pcu.c | |||
pcu.cc |
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.
extra whitespace?
pcu/CMakeLists.txt
Outdated
|
||
# Package headers | ||
set(HEADERS | ||
PCU.h | ||
pcu_io.h | ||
pcu_io.h |
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.
extra whitespace?
pcu/pcu.cc
Outdated
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.
nice!
I think it's good I just figured we may want to discuss/make sure we are comfortable before merging. |
meeting notes
|
This commit adds a MPI state that's separate from the global variables to the internal pcu_mpi interfaces. The user level PCU interface remains unchanged. However, this change lays the groundwork for PCU2 that will not make use of global state. This is important for some use cases like working around the spectrum-mpi comm_dup bug. The removal of global state also improves testability and modularity of this code.
Throughout the code base it was assumed that the mpi functions were implemented through pmpi and so no other implementation really could have been implemented. This change just removes the complexity from having the vtable.
This commit allows the pcu_mpi to be initialized into a stack variable rather than allways allocating on the heap.
pcu-object changes
…pcu-object Merging pcu-object
pcu-object fixed warnings
PhastaSharing* shr = new PhastaSharing(m); | ||
PCU_Comm_Begin(); | ||
PhastaSharing shr(m); | ||
m->getPCU()->Begin(); | ||
apf::MeshIterator* it = m->begin(dim); | ||
apf::MeshEntity* v; | ||
while ((v = m->iterate(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.
line 106 and 114 should be shr.
No description provided.