-
Notifications
You must be signed in to change notification settings - Fork 60
Experimental pass to reduce cycle inefficiencies of if with combinational group
#2596
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
base: main
Are you sure you want to change the base?
Conversation
|
This is awesome @ayakayorihiro! I particularly love the very rigorous benchmarking and analysis! |
EclecticGriffin
left a comment
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.
Looks good to me! My only floating thought is that I don't know if it is fully necessary to call rewrite_control on the entire block, since the only thing we actually are trying to rewrite is the condition port. Anything else within the if block shouldn't be referencing anything inside the with comb group, so such programs are already exhibiting bad behavior and we needn't worry about preservation of their already ambiguous semantics
| &cond_group_asgn.dst.borrow().parent | ||
| { | ||
| let c_ref = c.upgrade(); | ||
| let c_name = c_ref.borrow().name(); |
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.
Random note, if you want to remove a layer of indentation, you could use a
let-else with an early return rather than an if-let
let Some(cond_group_ref) = &s.cond else {
return Ok(Action::Continue);
};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.
For some cursed reason this comment ended up on the wrong line. As the code hopefully makes clear, this was supposed to be about line 72
| ..Default::default() | ||
| }; | ||
| // move all assignments in cond group to continuous and rewrite cells | ||
| for cond_group_asgn in &cond_group_ref.borrow_mut().assignments { |
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 don't think this needs to be borrow_mut since you only clone the assignment
| let mut new_if = calyx_ir::Control::if_( | ||
| s.port.clone(), | ||
| None, | ||
| Box::new(s.tbranch.take_control()), | ||
| Box::new(s.fbranch.take_control()), | ||
| ); | ||
| rewrite.rewrite_control(&mut new_if); |
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.
Forgive me if this is a misunderstanding of how the visitor system works, but
given that we're just replacing the current if block with a near clone, wouldn't
it make more sense to mutate the if directly? You would just need to remove the
cond and do whatever (if any?) rewriting is necessary
This PR contains:
simplify-if-combto address the cycle inefficiency ofifwith comb groups (Cycle inefficiency ofifwith comb group #2595).simplify-if-combThis pass transforms
ifwith combinational groups. It replaces the combinational group with a series of conditional assignments. The pass creates a new cell for any cell being written to in an assignment in the combinational group. For example, the code snippetgets converted into:
Cycle count effects
Comparing this pass with Calyx's default compilation flow on the queues programs (using

compare-experimental-argsdescribed below), we found maximum of 17.85% cycle reduction!Next steps
This pass is currently experimental (not used in any of the default pass aliases) because we don't know its effects on area and critical path. Our guess is that area and critical path will be negatively affected, since we are creating new cells and adding continuous assignments, but we need to actually run some experiments before deciding whether to add this pass to default compilation!
compare-experimental-argsThis script compares cycle counts between running Calyx with default arguments and running Calyx with new, experimental arguments. The motivation for this script was to compare the cycle count effects of
simplify-if-comb. It (currently) runs a subset of the queues tests listed inqueues-tests.txt.Usage
Note: This script assumes that you have
fud2set up and Verilator installed.From the base Calyx directory (it's not necessary to run from this directory):
where
<EXP_ARGS>should be a string with quotation marks around any new argumentsex)
The output of the script is the
outdirectory, which contains a JSON result file for each file and each configuration, aresults.csvthat shows the cycle reduction between the original Calyx compilation (og) and the experimental arguments (exp).Next steps
Some things we may want to add to this script: