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

Some miscellaneous builder improvements #1296

Merged
merged 16 commits into from
Dec 5, 2022
Merged

Some miscellaneous builder improvements #1296

merged 16 commits into from
Dec 5, 2022

Conversation

sampsyo
Copy link
Contributor

@sampsyo sampsyo commented Dec 2, 2022

Just a few holes addressed on the journey to #1240. Not much exciting to see here. I also happened across a mistake in a fud error message and fixed that too.

...and also implicitly import the newish `memories` header, which has
the seq_mem components.
This thought it was a format string but it was not.
I wrote in my builder `mem.addr0 = reg` instead of `mem.addr0 =
reg.out`. The confusing error happened on code *generation*. This makes
the problem a bit more visible.
@rachitnigam
Copy link
Contributor

Ah, I ended up making some of these changes in #1295 as well. Also, the interpreter does not support sequential memories so importing it default might be breaking things? In general, I’d like to revisit the decision to have a default import list include at all

Comment on lines +445 to +446
if prim == "seq_mem_d1":
if port_name == "read_en":
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if prim == "seq_mem_d1":
if port_name == "read_en":
if prim == "seq_mem_d1" and port_name == "read_en":

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking this and then skipped over the simplification to emphasize the outer case, which adds a different set of ports (which happens to be a set of size 1)… hope that's cool!

@sampsyo
Copy link
Contributor Author

sampsyo commented Dec 3, 2022

Ah, I ended up making some of these changes in #1295 as well.

Ah, nice—sorry for the race condition!! I'll let that one go first, and then fix this one up to adapt.

Also, the interpreter does not support sequential memories so importing it default might be breaking things? In general, I’d like to revisit the decision to have a default import list include at all

Good idea. Maybe we can implicitly import the first time you use a thing. Or maybe we want to offer some way to explicitly access things from the various libraries. Or maybe just the simplest/dumbest thing, which is that you have to explicitly ask to build imports (that's probably the right way).

...and also support != for not-equals.
We now *implicitly* import the `binary_operators` whenever you use
`add`. Of course we should probably add more of these implicit imports
for other stuff... and someday rethink the whole interface for importing
stuff from libraries.
@sampsyo
Copy link
Contributor Author

sampsyo commented Dec 4, 2022

I added a new explicit function prog.import_ for adding import statements. That function also avoids duplicate imports, so it made it easy to implicitly import the binary_operator library when using std_add the first time. Of course there could be a much better system for importing libraries, but hopefully this at least avoids the proximate problem of importing the memories library when it's unused.

@@ -99,8 +99,14 @@ def comb_group(self, name: str) -> "GroupBuilder":
return builder

def cell(
self, name: str, comp: ast.CompInst, is_external=False, is_ref=False
self, name: str, comp: Union[ast.CompInst, "ComponentBuilder"],
is_external=False, is_ref=False
) -> "CellBuilder":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can this feature be used? Or maybe, how does it show up in code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's for instantiating components that you just recently built with the builder! Like this in AOC day 1:
https://github.com/cucapra/aoc2022-calyx/blob/e63cff9055fa7e183dabab6993f2dee26376e6ab/1/accelgen.py#L103

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Makes sense

@rachitnigam
Copy link
Contributor

LGTM!

@sampsyo sampsyo merged commit 806a9fc into master Dec 5, 2022
@sampsyo sampsyo deleted the more-builder branch December 5, 2022 15:45
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