-
Notifications
You must be signed in to change notification settings - Fork 53
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
Promotion Heurisitcs (subsumes #1750) #1758
Conversation
@@ -21,7 +21,7 @@ static<15> component main(@go go: 1, @clk clk: 1, @reset reset: 1) -> (@done don | |||
} | |||
} | |||
control { | |||
@bound(5) static repeat 5 { | |||
static repeat 5 { |
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.
Should the promoted control statements take i) ir::Attributes::Default()
, or ii) the attributes of the old control statement? Maybe it should take ii): then I should prob change this before we merge.
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.
I think we should preserve the attributes and remove the @promotable
(or whatever its called)
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.
Ok, I've done that. I also remove @bound
once we promote to a static repeat, since it's kind of @bound
is redundant if we have a static repeat.
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!
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.
LGTM! Couple of style suggestions and one question.
self.convert_vec_to_static(builder, std::mem::take(stmts)); | ||
let latency = | ||
static_stmts.iter().map(|s| s.get_latency()).sum(); | ||
Self::check_latencies_match(latency, inferred_latency); |
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.
This method probably can be defined at the file-level instead of defining on Self
?
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.
I tried doing this, but realized there's some dependencies: self.static_group_name
(which maps dynamic group names -> corresponding promoted static groups) and self.static_component_latencies
(which maps static components -> latencies) are used in this method.
ir::StaticControl::static_if(Rc::clone(port), Box::new(static_tbranch), Box::new(static_fbranch), latency) | ||
} | ||
ir::Control::Static(_) => c.take_static_control(), | ||
ir::Control::Invoke(_) => unreachable!( |
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 should invokes be compiled away before promotion?
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 was just bc compile-invokes
was earlier in the pipeline than promotion. But I've added support for invokes now.
On a side note, when/if we overhaul with
and comb groups
(i.e., we choose to remove them from if
and while
control in the langauge) we should consider allowing static invoke
to support comb groups; it make make sense in some situations.
Subsumes #1750. Same as #1750, but also adds the option
-x static-promotion:stop_loop
which stops the promotion of while/repeat loops.