-
Notifications
You must be signed in to change notification settings - Fork 313
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] fix calyx canonicalization. #7456
[calyx] fix calyx canonicalization. #7456
Conversation
Thanks for creating this @linuxlonelyeagle! I think we should add a text with the three cases you mentioned so that future PRs don't break it. I'm also wondering if we should start canonicalizing away empty control programs ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few smaller comments, but otherwise LGTM.
+1 |
Optimized code. Co-authored-by: Chris Gyurgyik <[email protected]>
Optimize error reporting. Co-authored-by: Chris Gyurgyik <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I see this as an incremental improvement. Thanks!
Can you help me merge codes? Thanks! |
See issue.
Honestly, I saw this bug a few months ago and I think I fixed it, just like this PR did. But I don't think it was enough.
The problem is as I said in the issue, solved he still reports an error because now
calyx.control
is empty after-canonicalize
.So a few months ago I tried to include stricter restrictions, but I didn't have too good an idea.
Now my idea, when checking calyx.control, that op must have
calyx.enable,calyx.invoke alive fsm.machine
present. but it also causes other problems.On the one hand, I want to discuss it, and on the other hand, I think this PR makes sense.There is no test file because there are no examples of errors reported after -canonicalise in the existing test file.