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

ir::Builder should provide mutable access to LibrarySignatures #1583

Open
bcarlet opened this issue Jul 3, 2023 · 6 comments
Open

ir::Builder should provide mutable access to LibrarySignatures #1583

bcarlet opened this issue Jul 3, 2023 · 6 comments
Labels
C: Internal Changes to compiler internals C: Library Calyx's standard library S: Available Can be worked upon

Comments

@bcarlet
Copy link
Contributor

bcarlet commented Jul 3, 2023

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).

@rachitnigam rachitnigam added S: Discussion needed Issues blocked on discussion C: Library Calyx's standard library C: Internal Changes to compiler internals labels Jul 3, 2023
@rachitnigam
Copy link
Contributor

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.

@sampsyo
Copy link
Contributor

sampsyo commented Jul 5, 2023

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.

@rachitnigam
Copy link
Contributor

@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 rachitnigam added S: Needs Triage Issue needs some thinking and removed S: Discussion needed Issues blocked on discussion labels Jul 8, 2023
@bcarlet
Copy link
Contributor Author

bcarlet commented Jul 12, 2023

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 Support creating a workspace from multiple top-level files #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.

@rachitnigam
Copy link
Contributor

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
Copy link
Contributor

#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 rachitnigam changed the title Better support for primitives in ir::Builder ir::Builder should provide mutable access to LibrarySignatures Aug 11, 2023
@rachitnigam rachitnigam added S: Available Can be worked upon and removed S: Needs Triage Issue needs some thinking labels Aug 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: Internal Changes to compiler internals C: Library Calyx's standard library S: Available Can be worked upon
Projects
None yet
Development

No branches or pull requests

3 participants