-
Notifications
You must be signed in to change notification settings - Fork 68
Restore the coll/sync module and provide a test to verify its operation #1331
Conversation
|
Test PASSed. |
|
Per telecon, shifting to the 2.0.2 milestone |
|
@rhc54 the collective behaves as expected. However, the default priority is too high and gets selected by default. I should be lowered so it is not selected unless explicitly requested by the user. |
|
Adjusted as directed 👍 |
|
Test FAILed. |
|
Test FAILed. |
|
Test PASSed. |
|
Test PASSed. |
|
Build Failed with XL compiler! Please review the log, and get in touch if you have questions. Gist: https://gist.github.com/754f6a96d3712e9b2db3e055ce2e4ae0 |
|
bot:ibm:retest |
|
Test PASSed. |
| { | ||
| memset(&(module->c_coll), 0, sizeof(module->c_coll)); | ||
| module->before_num_operations = 0; | ||
| module->after_num_operations = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rhc54 Where do these variables ever get set to non-zero values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gets updated in the COLL_SYNC macro, with caveat of the bug I noted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I think it's ok -- I don't think you have a bug. I was mistaking the counts on the components, not the counters on the modules.
|
👍 |
|
OMPIBot error: Label "reviewed" is already set on issue 1331. |
|
We actually do still have an error in the macro, but I'll send you a fix for that as it is trivial. |
(cherry picked from commit open-mpi/ompi@9888615) Update to match v2.x definitions Adjust priority downward Set the default value of the barrier counters to zero so the coll/sync component is off by default Fix typo in the COLL_SYNC macro
|
Talked it through on the phone -- yes, I see the macro issue. And @rhc54 just pushed a commit to fix it. We're good to go now. |
|
Test PASSed. |
(cherry picked from commit open-mpi/ompi@9888615)
Update to match v2.x definitions
@matcabral Can you please review/test?