Skip to content
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

[Calyx-FIRRTL backend] Primitive Cells #1835

Merged
merged 39 commits into from
Jan 3, 2024
Merged

Conversation

ayakayorihiro
Copy link
Contributor

Note: I'm making this PR now so that I don't have any unfinished business left in 2023 😅 Please feel free to ignore until 2024!

I added support for cells that instantiate primitives! The approach involves (1) converting each unique primitive instantiation into a unique identifier (ex. std_add(32)std_add_32), (2) creating an extmodule declaration at the beginning of the FIRRTL file for such identifiers, and (3) converting each cell instantiation to a corresponding inst ... statement in FIRRTL.
I also incorporated @EclecticGriffin 's feedback from the last PR into this one. Thanks again for the feedback!!

Copy link
Collaborator

@EclecticGriffin EclecticGriffin left a 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! I only had very minor nits. Merge this whenever you're ready

@ayakayorihiro ayakayorihiro enabled auto-merge (squash) January 3, 2024 17:13
@ayakayorihiro ayakayorihiro merged commit 4a4aa7c into main Jan 3, 2024
@ayakayorihiro ayakayorihiro deleted the firrtl-primitive-cells branch January 3, 2024 18:43
@ayakayorihiro
Copy link
Contributor Author

Thanks for the comments @EclecticGriffin ! Fixed and merged :)

@sampsyo
Copy link
Contributor

sampsyo commented Jan 4, 2024

Wow! This is AWESOME!!! Just calling out the important bit in the generated FIRRTL:

    extmodule std_add_32:
        defname = std_add
        parameter WIDTH = 32

Wahoo! This means, of course, is that "all we need" is an implementation of std_add to link against.

To summarize something @ayakayorihiro and I discussed synchronously, it probably makes sense to approach this in two steps:

  1. Try compiling the FIRRTL to Verilog using official tooling (firtool, perhaps?) and link it against our existing Verilog implementations of the primitives. This gives us a scenic route to yet another a Verilog backend! 🤪 This at least lets us differentially test against the existing "real" Verilog backend in Calyx.
  2. ESSENT does not support Verilog primitives (maybe that's obvious). So we need to generate primitive implementations in FIRRTL. The straightforward way to do this would be to write them by hand in Chisel and compile them to LoFIRRTL. That would effectively replace these extmodule declarations with proper modules with real implementations. The annoying part about this will be aligning the naming conventions so the two bits of FIRRTL (compiled from Calyx and compiled from Chisel) agree on what std_add_32 means.

@ayakayorihiro
Copy link
Contributor Author

Thanks @sampsyo !! In my attempts to get Step 1 (from your above comment) going I realized there's a bug in the generated extmodule 😅 I didn't list out the inputs and outputs of the primitives in the extmodule declaration. I'm working on a fix right now, will make another smaller PR once that's done!

rachitnigam pushed a commit that referenced this pull request Feb 16, 2024
* 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

* Create extmodule name from primitive information. Need to extract into a function

* extracted function to get a FIRRTL module name given internal primitive data

* emit extmodule declarations

* Fix formatting errors

* Add test for instantiating cells and creating extmodules for primitive cells

* merged main and made changes based on PR comments

* Fix typo

* add another simple test

* Fixes from PR comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants