Conversation
These just perform remote address translation and then call the `*_indirect` variants, as donw with contiguous RMA. Add some comments to improve readability
rouson
left a comment
rouson
left a comment
There was a problem hiding this comment.
LGTM. I made a few comments/suggestions.
|
|
||
| result_ = succeed("") | ||
| result_ = result_ .and. assert_equals(size(expected,1), size(actual,1)) | ||
| result_ = result_ .and. assert_equals(size(expected,2), size(actual,2)) |
There was a problem hiding this comment.
What's the reason for not writing this as one statement?
result_ = assert_equals(size(expected,1), size(actual,1)) .and. &
assert_equals(size(expected,2), size(actual,2))
There was a problem hiding this comment.
No real reason, just a difference in style (incremental assignment &= vs simple assignment =).
The loop below repeatedly incrementally updates result_, so it seemed natural and consistent to use incremental assignment throughout the function.
I recently fixed a defect in a complicated test (prif_teams_test) that directly resulted from mixing the two styles: at some point someone reordered a simple assignment below an incremental one, which had the inadvertent effect of silently erasing all results computed earlier in the function, which was hiding real failures. As a result I think it's a safer practice to stick to one style per function: a single simple assignment in functions that compute a result_t from a single expression, or incremental assignments throughout functions that need to repeatedly update a result_t.
There was a problem hiding this comment.
I would actually like it even better if result_t provided a concise operator to encapsulate the incremental assignment result_ = result_ .and. expr, maybe something like result_.and(expr).
Given that, I would even go so far as to make it an error to simple assign into a result_t more than once, at least without first performing some kind of explicit reset operation (e.g. result_.reset()), in order to prevent accidental overwrites.
ktras
left a comment
ktras
left a comment
There was a problem hiding this comment.
LGTM. I don't have any feedback beyond the comments from Damian.
Co-authored-by: Damian Rouson <rouson@lbl.gov>
3 participants
This PR implements
prif_{get,put}_strided{,_indirect}.Notify variants remain unimplemented (as with contiguous RMA) until we implement
prif_notify_type.GASNet does all the heavy lifting for the actual data communication, and the metadata hookup was relatively straightforward.
The only tedious part was manually writing tests in PRIF to invoke each flavor of non-contiguous RMA.