calyx icon indicating copy to clipboard operation
calyx copied to clipboard

`ir::Builder` should provide mutable access to `LibrarySignatures`

Open bcarlet opened this issue 2 years ago • 6 comments
trafficstars

There are currently several limitations in the Rust builder library that make working with primitives inconvenient.

Currently, to instantiate an ir::Builder you must first construct an ir::LibrarySignatures containing all the primitive definitions you will need when building the component. Any subsequent calls to add_primitive will panic if you attempt to add a primitive not in the library, and there is no way to modify the library once it is created.

This makes it awkward to use inlined primitives, which are supposed to enable the generation of Calyx and Verilog code simultaneously. It also makes it less convenient to use the standard library, since you must import all the files you will ever need up front. For comparison, the Python builder library transparently accumulates any required imports as new primitives are instantiated.

The simple solution to this is to expose an interface to mutate the ir::LibrarySignatures. The ir::Builder would need to expose a mutable reference to the library, and the ir::LibrarySignatures would need to expose a function to add new primitives.

It might also make sense to allow forward declarations of primitives in the ir::Builder (i.e., make a version of add_primitive that behaves more like add_component). That would offer more flexibility as to when the primitives get created, and something like this might also be a prerequisite for more ambitious proposals such as #419 (at least in the setting where it's not easy to tell ahead of time which instantiations you'll need).

bcarlet avatar Jul 03 '23 21:07 bcarlet

Thanks for the thoughtful write-up Ben! I agree that there is room for improvement here. I think the immutable by default nature of libraries comes from the original use-case: program compilation. It was not obvious that program compilation should create new primitives.

However, you're exactly right about #419 needing this capability. For example, if we have a program, that during compilation wants to expose the requirement that it needs log_17 primitive, we need a way to "forward declare" the primitive and have the backend/module linker generate that primitive for us.

rachitnigam avatar Jul 03 '23 23:07 rachitnigam

Indeed; thanks for the discussion! I too like the "forward declaration" idea. This seems like a good reflection of the needs of frontends that need to bring in their own custom primitives, and it doesn't seem like too radical of a change either.

sampsyo avatar Jul 05 '23 15:07 sampsyo

@bcarlet can you summarize the outcomes from the synchronous meeting we had about this? IIRC, the top-level points were:

  1. We should give mutable access to LibrarySignatures
  2. We should make it easier to use primitives, maybe using the encoding you used in the calyx-numbers repo

If any of these are actionable, we should open issues for those

rachitnigam avatar Jul 08 '23 02:07 rachitnigam

From my notes, the issues that we agreed were immediately actionable were:

  • Adding an add_primitive() function to ir::LibrarySignatures and giving mutable access in ir::Builder. That should be covered by the present issue.
  • Making it easier to consume standard library files by allowing the parsing of multiple files in the frontend. I created an issue for that in #1599.

The right approach for encoding the standard primitives was more of an open issue, but we considered adding the calyx-numbers stdlib definitions to the main Calyx repository as a stopgap. I think there was consensus that a long-term solution should introduce a more automated way of keeping the definitions available from the ir::Builder and those in the primitives/*.futil files in sync.

bcarlet avatar Jul 12 '23 20:07 bcarlet

I think with #1596, we can consider adding all the "truly stable" primitives in the stdlib definition in a Rust crate since we're promising to support them in the future anyways.

rachitnigam avatar Jul 12 '23 21:07 rachitnigam

#1609 adds those methods to LibrarySignatures. Another, follow-on PR can address exposing them through the builder. Unfortunately that is going to be a pervasive change because we'll need to change passes to pass a mutable reference to LibrarySignatures as well.

rachitnigam avatar Jul 16 '23 21:07 rachitnigam