mpi: use mpi get_accumulate as proxy for atomic add #100
Conversation
| type &value, type &retVal, int offset, int pe, const MPI_Win &win) { \ | ||
| assert(win != MPI_WIN_NULL); \ | ||
| int _typesize; \ | ||
| MPI_Type_size(mpi_type, &_typesize); \ |
There was a problem hiding this comment.
| mpi_type_p(tmp, offset, pe, *win); | ||
| // forward the pinky promise of not updating "val" of mpi. | ||
| mpi_type_atomic_add(const_cast<non_const_value_type &>(val), tmp, offset, | ||
| pe, *win); | ||
| return tmp; |
There was a problem hiding this comment.
The value of tmp is undefined because there is no flush here. For single value operations MPI provides MPI_Fetch_and_op and it should be followed by MPI_Win_flush_local.
More generally, there is no way to hide the network latency for operators that return a result of an atomic op.
Also, you cannot guarantee that val will stay valid. If I write a += 1 then val will almost certainly break the "pinky promise" and go out of scope/get changed.
And why only implement this for operator+= and not operator*=?
There was a problem hiding this comment.
Okay according to https://www.mpi-forum.org/docs/mpi-4.1/mpi41-report.pdf Page 577, line 47, MPI_FETCH_AND_OP is faster than GET_ACCUMULATE. I should use fetch_and_add where possible.
vmiheer
force-pushed
the
vmiheer/mpi-get-accumulate
branch
from
56c3f9c
to
f385ad7
Compare
|
Superseded by #103 |
|
Closing. |
Partly fixes #99.
I think for other functions ideally the imples for atomic should be deleted giving compile time error message to the user of krs/mpi. But atleast it should print warnings saying the library is using non-atomic operations for now.
Get_accumulate might be slightly inefficient than just Accumulate but c++ opeartor += should return the updated value, maybe it could return "optional" value, the access to which would error out.