-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
1248 Subphase distributed label management #1255
base: develop
Are you sure you want to change the base?
Conversation
PR tests (clang-8, alpine, mpich)
|
PR tests (nvidia cuda 11.0, ubuntu, mpich)
|
PR tests (nvidia cuda 10.1, ubuntu, mpich)
|
2d2f591
to
c49e745
Compare
PR tests (clang-8, alpine, mpich)
|
PR tests (nvidia cuda 11.0, ubuntu, mpich)
|
PR tests (nvidia cuda 10.1, ubuntu, mpich)
|
PR tests (nvidia cuda 11.0, ubuntu, mpich)
|
Codecov Report
@@ Coverage Diff @@
## develop #1255 +/- ##
===========================================
+ Coverage 81.62% 81.74% +0.11%
===========================================
Files 733 738 +5
Lines 27901 28125 +224
===========================================
+ Hits 22774 22990 +216
- Misses 5127 5135 +8
|
PR tests (nvidia cuda 10.1, ubuntu, mpich)
|
static constexpr SubphaseType const seq_len_rooted = 31; | ||
|
||
/// Length of pure ID in subphase for collective subphases | ||
static constexpr SubphaseType const seq_len_collective = 63; |
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.
Given the relationship involved, I think these should sensibly use the values from the enum?
@@ -80,7 +81,9 @@ struct NextMsg; | |||
* rooted; collective hooks are invoked in the order in which they are | |||
* registered and are always run in a collective epoch. | |||
*/ | |||
struct PhaseManager : runtime::component::Component<PhaseManager> { | |||
struct PhaseManager | |||
: runtime::component::Component<PhaseManager>, subphase::SubphaseManager |
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.
Is there a specific reason to inherit here, rather than just composing with SubphaseManager
as a member? Is it to get the handlers or something?
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.
It's so the users can directly call: thePhase()->registerCollectiveSubphase
. I could just write some forwarding functions, but is there a reason why inheritance here isn't a good idea?
Fixes #1248