cholla-hydro / cholla

A GPU-based hydro code
https://github.com/cholla-hydro/cholla/wiki
MIT License
60 stars 32 forks source link

Define ``procID``, ``nproc``, & ``root`` even without MPI #354

Closed mabruzzo closed 6 months ago

mabruzzo commented 8 months ago

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:

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

Sounds good to me.

evaneschneider commented 8 months ago

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

mabruzzo commented 7 months ago

@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 commented 7 months ago

I made the comment then deleted it 5 minutes later when I realized it wasn’t possible; force pushing doesn’t delete GitHub comments.