Add a new MachEnv class to hold various machine and messaging parameters #26
Conversation
| } | ||
|
|
||
| // Set task 0 as master | ||
| NewEnv.MasterTask = 0; |
There was a problem hiding this comment.
Just wondering if we should make master rank an optional argument with a default value of zero?
| MPI_Group_range_incl(InGroup, NRanges, Range, &NewGroup); | ||
|
|
||
| // Create the communicator for the new group | ||
| MPI_Comm_create(InComm, NewGroup, &(NewEnv.Comm)); |
There was a problem hiding this comment.
Perhaps we can do a error check for comm_create so that it exits here than somewhere further down stream.
There was a problem hiding this comment.
I can add, but it turns out that there are few instances where an actual error is returned. Most often, it will return a MPI_COMM_NULL which can be for a variety of valid reasons.
| //------------------------------------------------------------------------------ | ||
| // Set task ID for the master task (if not 0) | ||
|
|
||
| int MachEnv::setMasterTask(const int TaskID) { |
There was a problem hiding this comment.
I guess the expectation is that we can dynamically change master even after comm creation. Would be great for adaptive/dynamic load balancing if we can make it work throughout the model.
There was a problem hiding this comment.
Yup, this could be part of load-balancing, especially if the default master is particularly over-loaded.
| #ifdef OMEGA_VECTOR_LENGTH | ||
| constexpr int VecLength = OMEGA_VECTOR_LENGTH; | ||
| #else | ||
| constexpr int VecLength = 1; |
There was a problem hiding this comment.
I'm overthinking this (not enough coffee) for future-proofing GPU architectures but should we make an equivalent GPU_VECTOR_LENGTH and set to 1?
Ignore if that's a trivial change anyway.
There was a problem hiding this comment.
Good to think about, but we can probably hold off on that level of granular configuration for GPUs until needed.
There was a problem hiding this comment.
Since this vector length will often appear as vector blocking of the inner loop, we will generally want it to be 1 for GPU and a separate GPU_VECTOR_LENGTH would be redundant. (Basically like the pack they're doing in the atmosphere). Though I think Trey was exploring use of non-unity values to match warps/thread group sizes and we could set this vector length to an appropriate value in that case. I do think we eventually want some GPU-related optimizing parameters, but Brian is correct that we want to figure out what those should be and can add later.
this change uncovered a problem with the original implementation that required some refactoring Also added a print function for debugging
|
I've made the changes suggested in the reviews, namely adding an optional argument for selecting a different master task and adding error checking on Comm_create. The former uncovered a problem with the implementation so I've had to refactor a bit. And I added a print function that was useful for debugging. Documentation has been changed to reflect these changes. All still passes the unit tests, including a new unit test for the added optional argument. |
This adds a new class MachEnv to hold a number of parameters that describe the machine environment, including MPI communicators and task info, number of threads if threaded, vector length for CPUs and potentially other GPU and node-level configuration info. This is described in the included documentation for User's and Developer's Guides.
This has been tested on Chrysalis using a provided unit test. This unit test has not yet been integrated into Cmake, but will be soon in an upcoming PR. In the meantime, you can build the unit test in the test/base directory using
mpicc (or other MPI-wrapped compiler) -I../../src/base -DOMEGA_VECTOR_LENGTH=16 ../../src/base/MachEnv.cpp MachEnvTest.cpp -lstdc++
and run using:
srun (or other mpi launcher) -n 8 ./a.out
Note that the unit test requires at least 8 MPI tasks.
Checklist