Repository navigation
misc: fixes and cleanups found during multi-testing - #5742
Merged
Merged
Conversation
Collaborator
Author
|
test:mpich/ch4/ofi |
4 tasks done
raffenet
reviewed
Jan 4, 2022
raffenet
left a comment
raffenet
left a comment
Collaborator
There was a problem hiding this comment.
Looks good to me. Just one minor question.
| MPI_Aint lb, true_lb, true_extent, extent, type_size; | ||
| void *ori_buffer = buffer; | ||
| MPI_Datatype ori_datatype = datatype; | ||
| int cellsize = MPIDI_POSIX_RELEASE_GATHER_BCAST_CELLSIZE; |
Collaborator
There was a problem hiding this comment.
Is moving this setting later necessary? Unclear to me how this affects the behavior.
Collaborator
Author
There was a problem hiding this comment.
It is not "necessary", but for readability. cellsize is only used in MPIR_Algo_calculate_pipeline_chunk_info and the reason we need this variable is due to the exception under #ifdef HAVE_ERROR_CHECKING. So move them closer makes the code easier to read. I can split this change if you prefer.
Collaborator
Author
There was a problem hiding this comment.
I have split the commits.
Collaborator
Author
|
test:mpich/ch4/ofi ✔️ |
raffenet
approved these changes
Jan 6, 2022
Because we initialize the release_gather structure once for each comm, we need make sure the parameters are consistent throughout the comm lifetime. CVARs may get updated during runtime. Thus we need initialize and transfer the CVARs into per-comm setting at init time.
It was missing the newline in the output.
There are multiple tests and assertions in op_coll.c. Add line number in its assertion output so we can locate the failing test.
mpiversion option is not implemented in runtests. Simply add macro guard in the test code. This is consistent with other tests that require certain minimum MPI versions. TODO: The test should print some message, e.g. Skipped due to version requirement, so that the test report can reflect it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request Description
Split misc fixes and cleanups from #5725.
Fixes in
ch4//release_gather,maint/extractcvars,test/mpi/coll/op_coll.c,test//coll_cvars.txt, and in testlist syntax.[skip warnings]
Author Checklist
Particularly focus on why, not what. Reference background, issues, test failures, xfail entries, etc.
Commits are self-contained and do not do two things at once.
Commit message is of the form:
module: short descriptionCommit message explains what's in the commit.
Whitespace checker. Warnings test. Additional tests via comments.
For non-Argonne authors, check contribution agreement.
If necessary, request an explicit comment from your companies PR approval manager.