-
Notifications
You must be signed in to change notification settings - Fork 297
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
[FIRRTL] Generic intrinsic parsing/emitter support #6897
[FIRRTL] Generic intrinsic parsing/emitter support #6897
Conversation
7aa2c4c
to
7482d71
Compare
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.
Nice refactoring of the parameter parsing! Looks good to me. Gonna let others comment on the syntax.
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.
I took a quick look through it, and the implementation looks good to me. I'm happy to approve as-is, not sure if anyone else wants to weigh in or have a deeper design review. To me, the fact that the parsing logic is relatively straightforward and not going through crazy gyrations means the syntax is effective.
Allows use of "intrinsic" as an identifier when not followed by the left-paren, e.g. `connect intrinsic.i, i`.
Added commit moving to the left-paren keyword specifically so that this is safe w.r.t. backwards-compatibility with designs that parse today as it only changes what happens when someone writes |
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
Co-authored-by: Robert Young <rwy0717@gmail.com>
Landing, thanks for the reviews/feedback all! Syntax is not official, but let's keep this train moving and modify as needed 👍 |
Add parsing and emitter support for FIRRTL intrinsic. Refactor parameter parsing on (ext/int)modules as these use same syntax. See tests and PR for more details. Thanks for the grammar fix, Rob! Co-authored-by: Robert Young <rwy0717@gmail.com>
Add parsing and emitter support for FIRRTL intrinsic.
Refactor parameter parsing on (ext/int)modules as these use same syntax.
Snippet from parse-basic.fir without FileCheck directives:
And somewhat similarly from the emit-basic test: