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

Generating calyx files as text via Rust too! #2157

Merged
merged 14 commits into from
Jun 21, 2024
Merged

Generating calyx files as text via Rust too! #2157

merged 14 commits into from
Jun 21, 2024

Conversation

ethanuppal
Copy link
Member

The Python eDSL is useful for generating and emitting calyx designs as text. I wrote a rust library that achieves the same goals. Here's an example of my initial attempt at creating it in usage. You can even encode comments!
image
I hacked this together in a few hours, and it's missing a lot of features (such as invokes), but it supports a very large subset of calyx right now.

@ethanuppal ethanuppal removed the request for review from anshumanmohan June 18, 2024 18:59
@ethanuppal
Copy link
Member Author

TODO: add more of the niceties of the eDSL, like helpers for common groups.

@anshumanmohan
Copy link
Contributor

Super cool! Sadly I'm the wrong person to review this for you, since I speak no Rust (yet) and am a little too busy to learn via your PR. Thanks for this work, and I hope someone will give this a look soon!

@ethanuppal ethanuppal requested a review from sampsyo June 19, 2024 17:04
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.

Hey sorry I missed this somehow! You should tag me in the future if you'd like review of rust code. I'll get a review in today

@EclecticGriffin EclecticGriffin self-requested a review June 19, 2024 19:58
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.

Neat! Left a bunch of notes here and there. It's a fair bit of code so lmk if there's something in particular you'd like me to look at that I didn't

tools/calyx-writer/lib.rs Show resolved Hide resolved
tools/calyx-writer/lib.rs Outdated Show resolved Hide resolved
tools/calyx-writer/lib.rs Outdated Show resolved Hide resolved
tools/calyx-writer/lib.rs Outdated Show resolved Hide resolved
Rc::new(RefCell::new(value))
}

struct IndentFormatter<'a, 'b: 'a> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not convinced you need two distinct lifetimes here. Is there a particular
part of code that doesn't work if you just have the one?

Copy link
Member Author

Choose a reason for hiding this comment

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

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

my intuition is that you should be able to have &'a mut fmt::Formatter<'a> but if not don't worry about it. I'd have to poke the code directly, which we could do together if desired but I could be wrong here

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I tried that and got this error :(

tools/calyx-writer/lib.rs Outdated Show resolved Hide resolved

/// Constructs a new cell named `name` that instatiates `inst` with
/// arguments `args`.
pub fn cell<S: ToString, T: ToString>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Recommend using a where construct here for readability

Copy link
Collaborator

Choose a reason for hiding this comment

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

You could also use more descriptive generic names if desired

tools/calyx-writer/lib.rs Outdated Show resolved Hide resolved
tools/calyx-writer/lib.rs Outdated Show resolved Hide resolved
tools/calyx-writer/lib.rs Show resolved Hide resolved
@ethanuppal
Copy link
Member Author

Thanks for the extensive review! I'll get to implementing these changes, and I left a few clarifying questions on some of your comments.

tools/calyx-writer/lib.rs Show resolved Hide resolved
tools/calyx-writer/lib.rs Outdated Show resolved Hide resolved
tools/calyx-writer/lib.rs Show resolved Hide resolved
tools/calyx-writer/lib.rs Outdated Show resolved Hide resolved
Comment on lines +692 to +696
for output in [Port::new_in_comp("done", 1)
.with_attribute(Attribute::num("done", 1))]
{
new_self.outputs.push(output);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see why this needs to be a for loop.

Copy link
Member Author

Choose a reason for hiding this comment

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

In case there are other special output ports needed later?

tools/calyx-writer/lib.rs Outdated Show resolved Hide resolved
tools/calyx-writer/lib.rs Outdated Show resolved Hide resolved
tools/calyx-writer/lib.rs Show resolved Hide resolved
@ethanuppal
Copy link
Member Author

@EclecticGriffin anything pressing to be resolved?

@EclecticGriffin
Copy link
Collaborator

LGTM modulo the one note I left. Merge when ready

@ethanuppal ethanuppal merged commit 96b740a into main Jun 21, 2024
18 checks passed
@ethanuppal ethanuppal deleted the calyx-writer branch June 21, 2024 20:56
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