[Calyx-FIRRTL] Support for template FIRRTL primitives #1864
Conversation
|
The |
extmodule declarations in translated FIRRTL|
I fixed up the flag based on @rachitnigam 's suggestion and made all of the changes I wanted to make to this PR! Sorry for the confusion earlier. |
There was a problem hiding this comment.
Wahoo; looks wonderful!! This all makes a lot of sense to me. I have a few minor Python-level suggestions for the new generator script, but this seems to be playing the role we need it too.
There is one slightly strange thing about the way the new generate-firrtl-with-primitives.py tool works, and I wanted to bring it up even though I see why it has to be this way. Basically, before seeing this PR, I expected that the tool here could just generate a primitives file, and then we could combine it after the fact with the original "main" program. Like, the workflow would go like this:
$ calyx -b firrtl foo.calyx -o foo.firrtl
$ calyx -b primitive-uses foo.calyx -o foo.prims.json
$ gen_firrtl_primitives.py < foo.prims.json > prims.firrtl
$ cat foo.firtl prims.firrtl > complete.firrtl
But, sadly, cat doesn't typically suffice to combine two FIRRTL files. The problem is that FIRRTL files need to start with circuit declaration, and we need to somehow insert the primitive into the same circuit as the main Calyx-generated code. Hence the need for the Python script to take the main FIRRTL code as input, and the need for the somewhat funky interleaving of output lines in its prints.
Anyway, I think it would be cool to somehow simplify this… maybe with some other way of linking FIRRTL circuits that I haven't discovered yet? But I would not let that hold up this PR; we can look into that separately!
| if len(sys.argv) != 3: | ||
| args_desc = [ | ||
| "FIRRTL_FILE", | ||
| "PRIMITIVE_USES_JSON" | ||
| ] | ||
| print(f"Usage: {sys.argv[0]} {' '.join(args_desc)}") | ||
| return 1 | ||
| firrtl_file = open(sys.argv[1]) | ||
| primitive_uses_file = open(sys.argv[2]) |
There was a problem hiding this comment.
It might be slightly nice to refactor main a tiny bit to separate the argument parsing from the "real work." Like, there could be a separate function declared like this:
def generate(firrtl_file, primitive_uses_file):…that is then called by main, which otherwise just has the responsibility of looking in sys.argv.
| print(firrtl_file.readline().rstrip()) | ||
| # Display the primitive definitions. | ||
| primitive_insts = json.load(primitive_uses_file) | ||
| if len(primitive_insts) != 0 : |
There was a problem hiding this comment.
A Python shortcut is relevant here:
| if len(primitive_insts) != 0 : | |
| if primitive_insts: |
| tmp_file = open(tmp_file_name, "w") | ||
| # execute the subprocess containing m4 | ||
| subprocess.run(m4_args, stdout=tmp_file) | ||
| for line in open(tmp_file_name, "r"): | ||
| print(line.rstrip()) |
There was a problem hiding this comment.
It's possible to avoid the temporary file here, I think. The subprocess module has a capture_output option that lets you obtain the stdout as a string directly. So something like this is likely to suffice:
proc = subprocess.run(m4_args, capture_output=True)
print(proc.stdout)There was a problem hiding this comment.
I actually tried this before getting to the temporary file hack, but went for the temp file because I got a byte string from capture_output and didn't know I can convert byte strings to strings 😓 Fixed!
| primitive_name = inst["name"] | ||
| args = [] | ||
| # hack to replace the module name with the corresponding parameterized version | ||
| # FIXME: figure out a way to do substring replacement in m4 |
There was a problem hiding this comment.
As straightforward as this is, one maybe-bad suggestion here might be that we could just not use m4. I know I suggested it originally, and it is nice that it can do most of the work for us, but since we already need a little Python script wrapped around the m4 invocation, we could imagine just using Python's str.replace to similar effect. Even if it does mean a bit more "heavy lifting" in Python-land, it would be one less moving part overall!
There was a problem hiding this comment.
I honestly think this is not that bad of an idea :) I ran into some weird write races when using the m4 subprocess rig, so having everything be in the one python script would be nice. Let me make that change!
|
To start answering my own question about this: it looks like the FIRRTL spec very recently added a new concept called This seems to be the "official" mechanism for composing separate FIRRTL circuits together. Barring that, this text-level "interleaving" is probably the right thing to do! |
Yeah, the problem was that I couldn't output everything using one command, which caused some trouble with the I suppose I could've put these three commands into a single bash subprocess (via
Thank you for finding this! I'll keep things as is for now but I think it would be great to use public modules in the future once I've been able to test the straightline/hacky way of things :) |
|
I changed the |
| @@ -0,0 +1,66 @@ | |||
| import json | |||
| import os | |||
| import subprocess | |||
There was a problem hiding this comment.
The subprocess import is now unused.
Cool! FWIW, while I haven't checked thoroughly, I expect that the Scala-based FIRRTL compiler doesn't support |
* First pass for backend to produce json file containing primitive instantiation info * pretty print the json * Map from param names to values * Fix clippy error * usage message * Fix doc comments * Change JSON format for parameter names and values * First commit of python script to process json and generate primitives * Fix formatting error * Hack to generate module name * Add some primitives * Generate module name + primitive templates for all documented primities * fix comment * Changed naming and fixed unchanged name * Make parameter collection functional * Option for enabling extmodule declarations for primitives * Fix formatting error from extracted function * Fix test configurations to produce extmodule * Rename script and pass in FIRRTL file * Change commandline option to not use -x * Update tests to match new commandline option * Hack to get around problem of subprocess and print output competing in fud2 * Add error message if primitive template file does not exist * Fix bug and added error message. Also added std_reg and undef primitives * Fix script from PR comments * Refactor script to use python to replace strings instead of using m4 subprocess * Remove unnecessary import
As discussed briefly in #1860 , FIRRTL modules are monomorphic; i.e. you cannot pass parameters into modules in the same fashion that you can pass parameters to Calyx primitives. Therefore, we need to generate specific FIRRTL modules that represent a Calyx primitive usage that we later instantiate in our code. The first step towards this was writing a backend (primitive-uses) that generates a JSON file that contains all of the primitive uses in a particular Calyx program (#1860).
This PR contains two main changes: (1) a Python script (
generate-firrtl-with-primitives.py) that uses a base FIRRTL file and the primitive usage information (in the JSON file) to produce a complete FIRRTL program; (2) excluding the previousextmoduledeclarations for primitives (added in #1835 ) by default and instead adding a flag (--emit-primitive-extmodules) for them.Some more rationale about (2): we can't use
extmoduledeclarations for our purposes of using ESSENT. But for testing and other reasons, it would be nice to have the option to produceextmoduledeclarations and use the preexisting Verilog implementations instead.I'm currently adding fud2 support for the pipeline described below in (1)! As always, any suggestions would be very appreciated :)
Usage
(1) Producing a complete FIRRTL file
First, use the
firrtlbackend to produce the base FIRRTL file and theprimitive-usesbackend to produce the primitive uses information:Then, run
generate-firrtl-with-primitives.pyto produce the complete FIRRTL program:(2) To produce FIRRTL that contains
extmoduledeclarations, run thefirrtlbackend with the flag--emit-primitive-extmodules. ex)Running the
firrtlbackend without the option will produce the FIRRTL translation withoutextmoduledeclarations (note: this code will not compile in FIRRTL on its own).