-
Notifications
You must be signed in to change notification settings - Fork 3
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
Bypass APF/SCOREC #68
Conversation
Disable other mesh outputs temporarily
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.
LGTM, based on my understanding
constexpr int Tag = 1000; | ||
|
||
std::vector<MPI_Request> largeIsend(const void* sendbuf, std::size_t sendsize, std::size_t senddisp, | ||
MPI_Datatype sendtype, int dest, int tag, MPI_Comm comm) { |
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.
Can you add a comment explaining in which way largeIsend differs from Isend and why it is needed? In the end, it is an extension of Isend to std::size_t?
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.
Yes; exactly. That's all it is, basically. We run multiple Isend
s, if the data is larger than the integer limit.
Other methods to handle that would have been to use extra data types.
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.
However, the actual main reason all these extra methods are needed are the offsets—i.e. to have a scatter/alltoall that is in total larger than the integer limit. Usually, we don't cross the integer boundary for a single send, i.e. the largeIsend
/largeIrecv
is more secondary.
@@ -0,0 +1,165 @@ | |||
#include "InsphereCalculator.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.
I guess you don't use the one from seissol:
https://github.com/SeisSol/SeisSol/blob/dd018b3398258a23ec2a33c74bd7f31b503dcca6/src/Initializer/time_stepping/GlobalTimestep.cpp#L22-L32
because you want to avoid another dependency?
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.
Yup—that was the main thing. Though it probably wouldn't be a big one anyways... I.e. it's thinkable I'd say.
IIRC I copied the calculation from there.
This PR adds functionality requested in #60 , where we had problems with the APF module. Thus, this PR makes it superfluous—we got all information at our fingertips already. So far, it's mostly tested with Simmodeler version 2023.0-231002. Also, all other mesh formats (excluding APF itself) now don't use the APF module anymore—a Simmodeler variant which still falls back to APF still exists here, though.
Changes:
SCOREC
in CMake, but it will only enable support for APF native and Simmodeler with APF—the other formats will not change back to using APF then)