-
Notifications
You must be signed in to change notification settings - Fork 51
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
@fast
attribute to enable early (0-cycle) transitions between alternating dynamic/static groups
#2118
Conversation
I want to confirm the following: |
@rachitnigam @calebmkim update ping |
I think this all lgtm! (As long as the problems in #662 do not apply, which I think they don't, but I haven't read the issue carefully.) |
@rachitnigam update ping |
@@ -828,6 +834,7 @@ impl Schedule<'_, '_> { | |||
&if_stmt.tbranch, | |||
tru_transitions, | |||
early_transitions, | |||
false, |
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.
Why does this unconditionally pass false
instead of threading through early_transitions
? Is it because we want to enable this if we see it in a seq
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.
We only want to enable @fast
on the parent control, I believe, unless you had different semantics in mind?
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.
Okay, that makes sense! We should eventually get rid of the other early-transition
logic entirely in a follow-up PR.
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.
Hmmm, there may be more cases in which we can apply early-transition
, for example, if (as @sampsyo said) one could prove that two adjacent groups cannot be done
at the same time; here, we could reuse @fast
's functionality, but there might be another case where it does apply recursively. That can be dealt with later though, it's not too hard to just re-add a parameter to all the functions.
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.
Mostly looks good! One question about the implementation and one seemingly erroneously committed file. If the file is meant to be committed, then let's document what it is meant to be doing.
@ethanuppal can you finalize the changes and answer the questions so we can merge this PR? |
Sorry for not getting around to this sooner!jk:wq |
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.
Awesome, thanks for implementing this! LGTM!
Will eventually close #1828 once finalized and thoroughly tested (the former of which may take a while).