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.

Sign up for GitHub

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

Jump to bottom

Calyx ConstantOp Support #7086

Merged
merged 7 commits into from
Oct 30, 2024
Merged

Calyx ConstantOp Support #7086

merged 7 commits into from
Oct 30, 2024

Conversation

jiahanxie353
Copy link
Contributor

This PR stacks on #6952 and is the first part of #6953 , in which I was asked to break down to smaller patches.

This PR adds support to the creation of Calyx ConstantOp in order to support floating point calculation in Calyx. In the meantime, we can support creating a Calyx RegisterOp by passing in a Type as an argument. I have also included a test case.

@jiahanxie353
Copy link
Contributor Author

I know that LLVM has tools  llvm/utils/update_mir_test_checks.py to update the tests by adding CHECK, CHECK-NEXT, etc. automatically. Is there a similar tool in CIRCT? I searched but couldn't find any.

@jiahanxie353 jiahanxie353 mentioned this pull request May 26, 2024
Merged
@rachitnigam rachitnigam mentioned this pull request May 27, 2024
Open
@jiahanxie353 jiahanxie353 force-pushed the calyx-const branch 2 times, most recently from 4ed164e to 9bb965b Compare May 29, 2024 15:28
@jiahanxie353 jiahanxie353 marked this pull request as draft June 13, 2024 20:47
@jiahanxie353 jiahanxie353 self-assigned this Jun 13, 2024
@rachitnigam rachitnigam added the Calyx The Calyx dialect label Jul 29, 2024
@rachitnigam
Copy link
Contributor

I think before merging this, we need to merge calyxir/calyx#2061. @jiahanxie353 can you get the changes on that resolved so we can move forward with this PR?

@jiahanxie353 jiahanxie353 force-pushed the calyx-const branch 2 times, most recently from e805fe7 to 0bee621 Compare October 30, 2024 13:35
@jiahanxie353 jiahanxie353 marked this pull request as ready for review October 30, 2024 13:35
@jiahanxie353
Copy link
Contributor Author

Changed the corresponding Calyx Constant Floating Point part reflecting the new changes. Marking it ready for review

cgyurgyik
cgyurgyik approved these changes Oct 30, 2024
View reviewed changes
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.

A small few comments, but in general LGTM.

lib/Dialect/Calyx/CalyxOps.cpp Show resolved Hide resolved
lib/Dialect/Calyx/Export/CalyxEmitter.cpp Outdated Show resolved Hide resolved
lib/Dialect/Calyx/CalyxOps.cpp Outdated Show resolved Hide resolved
lib/Dialect/Calyx/CalyxOps.cpp Outdated Show resolved Hide resolved
@jiahanxie353
Copy link
Contributor Author

Thank you @cgyurgyik ! I have made the changes based on the review.
I guess I can just click the button once all the tests finish running and pass?

@cgyurgyik
Copy link
Member

Thank you @cgyurgyik ! I have made the changes based on the review. I guess I can just click the button once all the tests finish running and pass?

Yes once tests pass feel free to merge. Great job!

andrewb1999
andrewb1999 approved these changes Oct 30, 2024
View reviewed changes
Copy link
Contributor

@andrewb1999 andrewb1999 left a comment

Choose a reason for hiding this comment

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

LGTM! Eventually we should maybe consider just using the calyx ConstantOp for everything instead of using both the calyx and hw constant ops.

@jiahanxie353 @cgyurgyik
setNameFn explicit cast to boolean
794dfac 
Co-authored-by: Chris Gyurgyik <Gyurgyikcp@gmail.com>
@jiahanxie353 jiahanxie353 merged commit b49d2b3 into llvm:main Oct 30, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cgyurgyik cgyurgyik cgyurgyik approved these changes

@andrewb1999 andrewb1999 andrewb1999 approved these changes

@mikeurbach mikeurbach Awaiting requested review from mikeurbach mikeurbach is a code owner

@mortbopet mortbopet Awaiting requested review from mortbopet mortbopet is a code owner

@rachitnigam rachitnigam Awaiting requested review from rachitnigam rachitnigam is a code owner

@fabianschuiki fabianschuiki Awaiting requested review from fabianschuiki

@maerhart maerhart Awaiting requested review from maerhart

Assignees

@jiahanxie353 jiahanxie353

Labels
Calyx The Calyx dialect
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jiahanxie353 @rachitnigam @cgyurgyik @andrewb1999