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

[Calyx] Add Repeat Op #5606

Merged
merged 7 commits into from
Jul 19, 2023
Merged

[Calyx] Add Repeat Op #5606

merged 7 commits into from
Jul 19, 2023

Conversation

calebmkim
Copy link
Contributor

@calebmkim calebmkim commented Jul 17, 2023

We recently added a non-static repeat op to native Calyx.

This PR adds it to the CIRCT Calyx Dialect. It was pretty straightforward, given we had already added a static repeat.

Dynamic repeat statement work the same as static repeat statements, except the body can be dynamically timed.

@calebmkim calebmkim added the Calyx The Calyx dialect label Jul 17, 2023
@cgyurgyik
Copy link
Member

Agh, the count is still static for a dynamic repeat; I've removed some of my comments :)

Copy link
Contributor

@mikeurbach mikeurbach left a comment

Choose a reason for hiding this comment

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

This seems reasonable to me.

include/circt/Dialect/Calyx/CalyxControl.td Outdated Show resolved Hide resolved
include/circt/Dialect/Calyx/CalyxControl.td Outdated Show resolved Hide resolved
@@ -145,7 +145,7 @@ PortInfo calyx::getPortInfo(BlockArgument arg) {

/// Returns whether the given operation has a control region.
static bool hasControlRegion(Operation *op) {
return isa<ControlOp, SeqOp, IfOp, WhileOp, ParOp, StaticRepeatOp,
return isa<ControlOp, SeqOp, IfOp, RepeatOp, WhileOp, ParOp, StaticRepeatOp,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not something to change in this PR, but these isa checks should probably check if op is ControlLike.

include/circt/Dialect/Calyx/CalyxControl.td Show resolved Hide resolved
Copy link
Member

@cgyurgyik cgyurgyik left a comment

Choose a reason for hiding this comment

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

Thank you!

include/circt/Dialect/Calyx/CalyxControl.td Outdated Show resolved Hide resolved
@calebmkim calebmkim merged commit d03a80c into llvm:main Jul 19, 2023
5 checks passed
@calebmkim
Copy link
Contributor Author

Ok, merging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Calyx The Calyx dialect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants