Skip to content
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

Merged
merged 4 commits into from
Dec 12, 2023

Conversation

mabruzzo
Copy link
Collaborator

@mabruzzo mabruzzo commented Nov 1, 2023

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

#ifdef MPI_CHOLLA
  if (procID == 0) { // or equivalently (procID == root)
#endif

    // some logic... (possibly with additional #ifdef MPI_CHOLLA statements)

#ifdef MPI_CHOLLA
  }
#endif

We will now be able to rewrite these sections as

  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.

@bcaddy
Copy link
Collaborator

bcaddy commented Nov 2, 2023

Generally I'm a big fan of this. My only complaint is the location in global_parallel.h/cpp. I think we should try to limit the number of global files that need to be imported everywhere. The current system of one for everything (global.h) and one for cuda code (global_cuda.h) is enough IMO. Given that these variables are fundamentally for managing MPI stuff I think it would be really nice if they could stay in mpi_routines.h/cpp, if that is too complicated to implement due to ifdefs then I think global.h/cpp is a better place for them. My other issue with it is that _parallel is vague when we have 3 different parallelization tools floating around (MPI, OpenMP, and CUDA), IMO global_mpi would be a better choice.

@mabruzzo
Copy link
Collaborator Author

mabruzzo commented Nov 3, 2023

Ideally I also think it could also make sense to keep them in mpi_routines.h/mpi_routines.cpp. But that seems a little involved at the moment (especially since mpi_routines.h is almost always conditionally included). And I would like to get this merged in as soon as possible.

So I moved them into globals.h/globals.cpp

@bcaddy
Copy link
Collaborator

bcaddy commented Nov 3, 2023

Sounds good to me.

@evaneschneider
Copy link
Collaborator

Works for me! Agreed that limiting the number of global headers is preferable.

@mabruzzo
Copy link
Collaborator Author

mabruzzo commented Nov 6, 2023

@bcaddy - I think you have left a comment suggesting that I use a static_assert instead of CHOLLA_ERROR inside of Init_Global_Parallel_Vars_No_MPI. I might be wrong about whether you actually suggested that (I performed a force push and that seems to have deleted your comments. I'll definitely avoid doing that in the future). Even if you didn't make that comment, I'll still address this anyways.

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 static_assert:

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 -DMPI_CHOLLA, the compiler aborts with an error. This is because the snippet looks like the following to the compiler:

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 static_assert inside of a concrete function.

@bcaddy
Copy link
Collaborator

bcaddy commented Nov 6, 2023

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.
@evaneschneider evaneschneider merged commit 977d04d into cholla-hydro:dev Dec 12, 2023
10 checks passed
@mabruzzo mabruzzo deleted the global-parallel-vars branch June 21, 2024 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants