[Calyx-FIRRTL backend] Guards and non-primitive Cells #1817
Conversation
calyx-backend/src/firrtl.rs
Outdated
| // TODO: use extmodules | ||
| writeln!( | ||
| f, | ||
| "{}; FIXME: attempting to instantiate primitive cell {}", |
There was a problem hiding this comment.
Excellent; seems like a great way to defer this part!
calyx-backend/src/firrtl.rs
Outdated
| writeln!( | ||
| f, | ||
| "{}; FIXME: attempting to instantiate primitive cell {}", | ||
| SPACING.repeat(2), | ||
| cell_borrowed.name() | ||
| )? |
There was a problem hiding this comment.
This is super minor, but it kinda looks like some indentation was lost here?
There was a problem hiding this comment.
Yup, the indentation was a bit wonky here 😅 Thanks for pointing it out!!!
calyx-backend/src/firrtl.rs
Outdated
| if !dst_set.contains(&dst_canonical_str) { | ||
| // if we don't have a "is invalid" statement yet, then we have to write one. | ||
| let _ = write_invalid_initialization(&asgn.dst, f); | ||
| dst_set.insert(dst_canonical_str); | ||
| } |
There was a problem hiding this comment.
This uses a "lazy" policy to initialize all ports that are assigned to at least once. Makes sense and no need to change it, but I just wanted to mention that we could consider an "eager" policy as well. Basically, we'd enumerate all possible ports (which would consist of all our children's input ports and our output ports) and do an is invalid to all of them up front.
This isn't necessarily any better than the lazy approach, except maybe that it doesn't require the dst_set tracking. So let's not do it, but I thought I'd record the idea in case it becomes relevant in the future.
There was a problem hiding this comment.
Ah that makes sense!! Let me leave a comment in the code so that I don't forget as well :)
| }; | ||
| format!("{}({}, {})", op_str, l_str, r_str) | ||
| } | ||
| ir::Guard::Port(port) => get_port_string(&port.borrow().clone(), false), |
There was a problem hiding this comment.
This clone seems off to me?
Since get_port_string only requires a &ir::Port as the first argument I don't see a good reason to clone anything when you can just pass a reference to the port you have. See the invocations you have above for CompOp
There was a problem hiding this comment.
Thanks for the catch!! Indeed the cloning was unnecessary 😅
* Initial commit: added barebones firrtl.rs as a backend option. * Fix formatting errors and try to leverage VerilogBackend * Fix formatting * first pass on inputs and outputs * fix CI formatting issue * more CI formatting fixes * temporary commit before fixing up starter code * Clean up starter firrtl translation code * Fix CI errors * Barebones test containing input/output ports and single assignment * Fix test failure caused by whitespace * clean up unnecessary comments * Port guard support and some refactoring for converting ports to FIRRTL * First steps for recursive guard building + more refactoring * Guard support and tests * Updating firrtl.rs to support guards * Fix formatting issues * additional formatting fix * Added default assignment to 0 for guard failure and fixed expected output * Added default initialization statements for assignments with guards * adding test to make sure that there's only one invalid initialization per unique dest * fixing attributes and "is invalid" initialization * First steps to support non-primitive cells * Fixing hardcoding of main as the top level component * Fix formatting errors * More formatting fixes * Remove unnecessary FIXME * Fix indentation and add comment from PR feedback * Fix small format bug
I made progress on tackling the TODOs (here: #1805 ) towards a Calyx-to-FIRRTL translation! The backend can now translate Guards and non-primitive Cells. For now, the backend emits some comments when trying to translate primitive cell instantiations, but I will be implementing primitive cells next so this should be fixed in the next PR.
I'd appreciate all feedback on this, thank you in advance 🙂