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 invoke pass #5635

Merged
merged 14 commits into from
Aug 7, 2023
Merged

Conversation

linuxlonelyeagle
Copy link
Member

This PR adds the lower invoke operation part to the lower-calyx-to-fsm pass.

@linuxlonelyeagle
Copy link
Member Author

@rachitnigam I've added the pass for lower invoke here.:smile:

@rachitnigam
Copy link
Contributor

Seems reasonable to me for the most part after from the potential name-conflict problem

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.

Some initial comments.

lib/Conversion/CalyxToFSM/CalyxToFSM.cpp Outdated Show resolved Hide resolved
lib/Conversion/CalyxToFSM/CalyxToFSM.cpp Outdated Show resolved Hide resolved
lib/Conversion/CalyxToFSM/CalyxToFSM.cpp Outdated Show resolved Hide resolved
lib/Conversion/CalyxToFSM/CalyxToFSM.cpp Outdated Show resolved Hide resolved
lib/Conversion/CalyxToFSM/CalyxToFSM.cpp Outdated Show resolved Hide resolved
lib/Conversion/CalyxToFSM/CalyxToFSM.cpp Outdated Show resolved Hide resolved
lib/Conversion/CalyxToFSM/CalyxToFSM.cpp Outdated Show resolved Hide resolved
lib/Conversion/CalyxToFSM/CalyxToFSM.cpp Outdated Show resolved Hide resolved
lib/Conversion/CalyxToFSM/CalyxToFSM.cpp Outdated Show resolved Hide resolved
lib/Conversion/CalyxToFSM/CalyxToFSM.cpp Outdated Show resolved Hide resolved
lib/Conversion/CalyxToFSM/CalyxToFSM.cpp Outdated Show resolved Hide resolved
test/Conversion/CalyxToFSM/lower-invoke.mlir Show resolved Hide resolved
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.

I left a couple comments about some simple improvements, but I think this is looking pretty good.

lib/Conversion/CalyxToFSM/CalyxToFSM.cpp Show resolved Hide resolved
lib/Conversion/CalyxToFSM/CalyxToFSM.cpp Outdated Show resolved Hide resolved
lib/Conversion/CalyxToFSM/CalyxToFSM.cpp Outdated Show resolved Hide resolved
lib/Conversion/CalyxToFSM/CalyxToFSM.cpp Outdated Show resolved Hide resolved
linuxlonelyeagle and others added 3 commits July 22, 2023 00:17
Co-authored-by: Chris Gyurgyik <37983775+cgyurgyik@users.noreply.github.com>
mikeurbach
mikeurbach previously approved these changes Jul 21, 2023
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 good to me.

@mikeurbach mikeurbach dismissed their stale review July 21, 2023 19:42

I think my most recent comment should be taken into account.

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.

Thanks, this looks good to me now.

@linuxlonelyeagle
Copy link
Member Author

@rachitnigam @mortbopet I think you can continue to review the code, and if you have any suggestions, please let me know.:smile:

@linuxlonelyeagle
Copy link
Member Author

@rachitnigam @mortbopet I'll be honest, you're forgetting that there's code to review here.

@cgyurgyik
Copy link
Member

Hi, it is common for folks to take weekends off.

Also, I have a few unaddressed comments.

@linuxlonelyeagle
Copy link
Member Author

Sorry, I don't have the sense to take weekends off here. You have questions, feel free to let me know, as I recall I took all your previous suggestions, if there are new ones, feel free to let me know .

Copy link
Contributor

@mortbopet mortbopet left a comment

Choose a reason for hiding this comment

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

I am also one of those people who take weekends - please be respectful and mindful of other people's time. Reviewing this part of CIRCT is something we do on our own time, which sometimes leads to multiple days of delay. Make sure that you invest in correctly addressing the feedback you've been granted in prior reviews, and give people time, before pinging them.
Doing so builds good relationships, failing to do so will make it harder to get people to review your stuff in the future.

lib/Conversion/CalyxToFSM/CalyxToFSM.cpp Outdated Show resolved Hide resolved
lib/Conversion/CalyxToFSM/CalyxToFSM.cpp Outdated Show resolved Hide resolved
lib/Conversion/CalyxToFSM/CalyxToFSM.cpp Show resolved Hide resolved
lib/Conversion/CalyxToFSM/CalyxToFSM.cpp Outdated Show resolved Hide resolved
lib/Conversion/CalyxToFSM/CalyxToFSM.cpp Outdated Show resolved Hide resolved
@linuxlonelyeagle
Copy link
Member Author

linuxlonelyeagle commented Jul 25, 2023

I'm really sorry about that, and to be honest with you, I really didn't think about the point about weekend vacations because it doesn't feel like it's taken seriously in the people around me, I hadn't considered this point and it's really something I did wrong. On the other hand, I have my own problems, to be honest and honest, as a fan of llvm, I was desperate to be involved in the llvm project. I will pay more attention in these aspects in the future. Sorry again.

  • Make sure that you invest in correctly addressing the feedback you've been granted in prior reviews, and give people time, before pinging them.
    This is clear to me, I know what I did before may not be right, but for you to put forward the suggestion I am very serious consideration(It's also possible that my English is not that good and I don't understand you correctly), I honestly, as a student, I know I have a lot of places to do not good enough, I'm sorry, if there is a do not do a good job (no matter whether it's to write the code or other aspects), in fact, you guys say that I've been very try to do it.

@cgyurgyik cgyurgyik self-requested a review July 25, 2023 17:16
@rachitnigam rachitnigam added the Calyx The Calyx dialect label Aug 1, 2023
@rachitnigam
Copy link
Contributor

Hey @cgyurgyik, just checking to see if the new changes look good and if we should proceed with a merge of this

lib/Conversion/CalyxToFSM/CalyxToFSM.cpp Outdated Show resolved Hide resolved
test/Conversion/CalyxToFSM/lower-invoke.mlir Show resolved Hide resolved
lib/Conversion/CalyxToFSM/CalyxToFSM.cpp Outdated Show resolved Hide resolved
lib/Conversion/CalyxToFSM/CalyxToFSM.cpp Outdated Show resolved Hide resolved
lib/Conversion/CalyxToFSM/CalyxToFSM.cpp Outdated Show resolved Hide resolved
lib/Conversion/CalyxToFSM/CalyxToFSM.cpp Outdated Show resolved Hide resolved
lib/Conversion/CalyxToFSM/CalyxToFSM.cpp Outdated 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.

Hi! A few smaller comments to address before submission. Apologies @linuxlonelyeagle, these comments have been pending for a while; I thought they were submitted.

Thank you for your work!

@rachitnigam
Copy link
Contributor

Okay, looks like @linuxlonelyeagle has addressed all the comments. Thank you so much for your work @linuxlonelyeagle! The first PR in a new open source project always ends up taking some time to merge because there are a lot of style-related things to learn but we hope the process was useful for you! Looking forward to your future contributions!

@rachitnigam rachitnigam merged commit d0af7d5 into llvm:main Aug 7, 2023
5 checks passed
@linuxlonelyeagle
Copy link
Member Author

Thank you all for your guidance.I learned a lot and took the opportunity to contribute to LLVM and CIRCT. The part about calyx.invoke should still have ref functionality to be done, if I have time I'll make it.

@cgyurgyik
Copy link
Member

Great! Looking forward to it :). I would also suggest looking at the LLVM Programmer's Manual, since CIRCT falls under LLVM:

https://llvm.org/docs/ProgrammersManual.html

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.

5 participants