Introduce ability to pass in subtypes as ref cells in invoke statements
#2085
Conversation
There was a problem hiding this comment.
Looks great, @nathanielnrn! I have a few low-level comments, but I think this overall looks ready to go.
One small thing: would you mind finding an appropriate place to document this behavior in the language reference? It seems useful to have a short paragraph describing what you are allowed to do with refs in this way.
| if canon.cell != ref_cell_name { | ||
| continue; | ||
| } | ||
| let in_cell_borrowed = in_cell.borrow(); |
There was a problem hiding this comment.
It's a common pattern in Rust to use shadowing for situations like this, where you "upgrade" the type of something but it still sorta means the same thing. As in:
let in_cell = in_cell.borrow();| let pr = in_cell_borrowed | ||
| .ports | ||
| .iter() | ||
| .find(|&in_cell_port| { | ||
| canon.cell == ref_cell_name | ||
| && in_cell_port.borrow().name == canon.port | ||
| }) | ||
| .unwrap_or_else(|| { | ||
| unreachable!( | ||
| "port `{}` not found in the cell `{}`", | ||
| canon, | ||
| in_cell.borrow().name() | ||
| ) | ||
| }); |
There was a problem hiding this comment.
Maybe it would be helpful to outline this to a function of its own… named something like find_cell_port, to make it clear that we are looking for the corresponding port on the passed-in cell.
There was a problem hiding this comment.
This is totally a non-urgent self-nit, but I find this stuff interesting because in general I struggle with idiomatic rust and want to get better at it.
A possible implementation could look like this (note in the signature we expect an RRC):
fn get_concrete_port(concrete_cell : RRC<ir::Cell>, canonical_port : &ir::Id) -> RRC<ir::Port>{
let concrete_cell = concrete_cell.borrow();
concrete_cell.ports.iter().find(|&concrete_cell_port| {
concrete_cell_port.borrow().name == canonical_port
}).unwrap_or_else(|| {
unreachable!(
"port `{}` not found in the cell `{}`",
canonical_port,
concrete_cell.name()
)
}).clone()
}
}With a function call like this
let concrete_port = Self::get_concrete_port(concrete_cell.clone(), &canon.port);Another option is to have a signature like this where instead of an RRC a "raw" reference is expected
fn get_concrete_port(concrete_cell : &ir::Cell, canonical_port : &ir::Id) -> RRC<ir::Port>{with a function call like this
let concrete_port = Self::get_concrete_port(&concrete_cell.borrow(), &canon.port);Curious if one is "better" than the other? My gut is that if we're already using RefCells we might as well keep them around, and so the first option is more in line with that.
calyx-opt/src/passes/well_formed.rs
Outdated
| ))) | ||
| } else { | ||
| Ok(()) | ||
| /// Returns true if cell_in is a subtype of the output cell. |
There was a problem hiding this comment.
Maybe just a couple of sentences here about what we do currently check: every port is compatible, same width, etc.
| enum Invoke<'a> { | ||
| StaticInvoke(&'a ir::StaticInvoke), | ||
| Invoke(&'a ir::Invoke), | ||
| } | ||
|
|
||
| impl Invoke<'_> { | ||
| fn get_ref_cells(&self) -> &Vec<(ir::Id, ir::RRC<Cell>)> { | ||
| match self { | ||
| Invoke::StaticInvoke(s) => &s.ref_cells, | ||
| Invoke::Invoke(s) => &s.ref_cells, | ||
| } | ||
| } | ||
|
|
||
| fn get_attributes(&self) -> &ir::Attributes { | ||
| match self { | ||
| Invoke::StaticInvoke(s) => s.get_attributes(), | ||
| Invoke::Invoke(s) => s.get_attributes(), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn require_subtype( | ||
| invoke: Invoke, | ||
| self_ref_cells: &HashMap<ir::Id, LinkedHashMap<ir::Id, Cell>>, | ||
| id: &ir::Id, | ||
| ) -> CalyxResult<()> { | ||
| let cell_map = &self_ref_cells[id]; | ||
| let mut mentioned_cells = HashSet::new(); | ||
| for (outcell, incell) in invoke.get_ref_cells().iter() { | ||
| if let Some(oc) = cell_map.get(outcell) { | ||
| if !subtype(oc, &incell.borrow()) { | ||
| return Err(Error::malformed_control(format!( | ||
| "The type passed in `{}` is not a subtype of the expected type `{}`.", | ||
| incell.borrow().prototype.surface_name().unwrap(), | ||
| oc.prototype.surface_name().unwrap() | ||
| )) | ||
| .with_pos(invoke.get_attributes())); | ||
| } else { | ||
| mentioned_cells.insert(outcell); | ||
| } | ||
| } else { | ||
| return Err(Error::malformed_control(format!( | ||
| "{} does not have ref cell named {}", | ||
| id, outcell | ||
| ))); | ||
| } | ||
| } | ||
| for id in cell_map.keys() { | ||
| if !mentioned_cells.contains(id) { | ||
| return Err(Error::malformed_control(format!( | ||
| "unmentioned ref cell: {}", | ||
| id | ||
| )) | ||
| .with_pos(invoke.get_attributes())); | ||
| } | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
|
Thanks a lot for these comments! I tried my best to address things. There was one thing I wasn't sure about that might be worth taking a look at. I'll merge this soon though, I think in general things are all right. |
|
In spite of a few minor outstanding points I'm wondering about, going to merge this! |
Closes #2015.
This PR changes the
compile_invokeandwell_formedpasses to allow cells that are subtypes of arefcell's type to be passed in to saidrefcell.A component
foois a subtype of componentbarif foo has at least the same port names & widths asbar.foocan have extra ports thatbardoes not and still be a subtype.One important thing to note is that I think the subtype check is actually incomplete.
Currently,
ais a subtype ofbif all of the ports ofbare found ina. Depending on how #2079 is addressed, we may want to inductively check if therefcells inbare subtypes of all of therefcells ina(basically, contravariance on the "arguments" [refcells] of our components). I think currently, without the ability to have nestedrefs the subtyping implemented works.There are a few
NOTE/XXXs in place I wasn't sure about things w.r.t rust and borrowing/cloning, but in general clippy seemed happy.This PR also replace some
HashMaps withLinkedHashMaps to get rid of some non-determinism introduced in assignment ordering