-
Notifications
You must be signed in to change notification settings - Fork 32
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
Define procID
, nproc
, & root
even without MPI
#354
Define procID
, nproc
, & root
even without MPI
#354
Conversation
Generally I'm a big fan of this. My only complaint is the location in |
9e96ad3
to
023a35d
Compare
Ideally I also think it could also make sense to keep them in So I moved them into |
Sounds good to me. |
Works for me! Agreed that limiting the number of global headers is preferable. |
023a35d
to
638211a
Compare
@bcaddy - I think you have left a comment suggesting that I use a We can't replace the following code-snippet (the formatting is modified for conciseness): void Init_Global_Parallel_Vars_No_MPI() { // equivalent to current version
#ifdef MPI_CHOLLA
CHOLLA_ERROR("This function should not be executed when compiled with MPI");
#endif
procID = 0; nproc = 1; root = 0;
} with this subsequent snippet using void Init_Global_Parallel_Vars_No_MPI() { // modified to use static_assert
#ifdef MPI_CHOLLA
static_assert(false, "This function should not be executed when compiled with MPI");
#endif
procID = 0; nproc = 1; root = 0;
} I didn't think this would be possible. To be sure, I actually tried to make this change and confirmed that it isn't possible. When the compiler encounters the second snippet of code, when configured with void Init_Global_Parallel_Vars_No_MPI() { // static_assert version compiled with -DMPI_CHOLLA
static_assert(false, "This function should not be executed when compiled with MPI");
procID = 0; nproc = 1; root = 0;
} Essentially, the compiler is not allowed to compile |
I made the comment then deleted it 5 minutes later when I realized it wasn’t possible; force pushing doesn’t delete GitHub comments. |
In more detail, these variables were previously declared in mpi_routines.h and were only ever defined when Cholla was compiled with MPI. This commit now makes it so that these variables are defined even when not compiling with MPI. In that scenario, these variables have default values of: - ``procID = 0`` - ``nproc = 1`` - ``root = 0`` To actually accomplish this, I moved these variable declarations to new files called ``global_parallel.h`` and ``global_parallel.cpp``. Based on the reviewer's preference, I would be equally happy to move these to ``global.h`` and ``global.cpp``. The motivation for this is simple. Throughout the codebase there are a number of places where we have code like the following ```c++ if (procID == 0) // or equivalently (procID == root) { // some logic... (possibly with additional #ifdef MPI_CHOLLA statements) } ``` We will now be able to rewrite these sections as ```c++ if (procID == 0) { // or equivalently (procID == root) // some logic... (possibly with #ifdef MPI_CHOLLA statements) } ``` This should let us remove a bunch of ``ifdef MPI_CHOLLA`` statments.
638211a
to
7e4d5bd
Compare
In more detail, these variables were previously declared in mpi_routines.h and were only ever defined when Cholla was compiled with MPI. This commit now makes it so that these variables are defined even when not compiling with MPI. In that scenario, these variables have default values of:
procID = 0
nproc = 1
root = 0
To actually accomplish this, I moved these variable declarations to new files called
global_parallel.h
andglobal_parallel.cpp
. Based on the reviewer's preference, I would be equally happy to move these toglobal.h
andglobal.cpp
.The motivation for this is simple. Throughout the codebase there are a number of places where we have code like the following
We will now be able to rewrite these sections as
This should let us remove a bunch of
ifdef MPI_CHOLLA
statments.