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

[FIRRTL] Add ObjectSubfieldOp #5768

Merged
merged 1 commit into from
Aug 10, 2023
Merged

[FIRRTL] Add ObjectSubfieldOp #5768

merged 1 commit into from
Aug 10, 2023

Conversation

rwy7
Copy link
Contributor

@rwy7 rwy7 commented Aug 3, 2023

This op indexes into objects (values of type ClassType).

@rwy7 rwy7 added the FIRRTL Involving the `firrtl` dialect label Aug 3, 2023
@rwy7 rwy7 requested review from mikeurbach and youngar August 3, 2023 02:38
Copy link
Contributor

@dtzSiFive dtzSiFive left a comment

Choose a reason for hiding this comment

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

Looking good! Added some comments. A test showing it in IR and some error checks would be great 👍 .

include/circt/Dialect/FIRRTL/FIRRTLExpressions.td Outdated Show resolved Hide resolved
include/circt/Dialect/FIRRTL/FIRRTLExpressions.td Outdated Show resolved Hide resolved
lib/Dialect/FIRRTL/FIRRTLOps.cpp Outdated Show resolved Hide resolved
lib/Dialect/FIRRTL/FIRRTLOps.cpp Outdated Show resolved Hide resolved
lib/Dialect/FIRRTL/FIRRTLOps.cpp Outdated Show resolved Hide resolved
lib/Dialect/FIRRTL/FIRRTLOps.cpp Outdated Show resolved Hide resolved
include/circt/Dialect/FIRRTL/FIRRTLExpressions.td Outdated Show resolved Hide resolved
include/circt/Dialect/FIRRTL/FIRRTLExpressions.td Outdated Show resolved Hide resolved
@rwy7 rwy7 force-pushed the classes branch 2 times, most recently from 61472f9 to 3a2da71 Compare August 3, 2023 14:09
@rwy7 rwy7 requested a review from dtzSiFive August 3, 2023 14:48
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 have a nitpicky comment about the op naming / description. It is not crucial, and I think the actual implementation looks great.

Copy link
Member

@youngar youngar left a comment

Choose a reason for hiding this comment

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

Nits, would be great to get some error tests for ObjectSubfieldOp verifiers.

lib/Dialect/FIRRTL/FIRRTLOps.cpp Show resolved Hide resolved
lib/Dialect/FIRRTL/FIRRTLOps.cpp Outdated Show resolved Hide resolved
lib/Dialect/FIRRTL/FIRRTLOps.cpp Outdated Show resolved Hide resolved
include/circt/Dialect/FIRRTL/FIRRTLExpressions.td Outdated Show resolved Hide resolved
@mikeurbach
Copy link
Contributor

I didn't mention it before, but I also echo the comments about adding success/failure tests for the new op.

@rwy7 rwy7 force-pushed the classes branch 3 times, most recently from 4d75823 to d89e202 Compare August 4, 2023 14:30
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 looks good to me. Might be worth adding a couple extra tests to cover some of the verifier failures.

lib/Dialect/FIRRTL/FIRRTLOps.cpp Outdated Show resolved Hide resolved
lib/Dialect/FIRRTL/FIRRTLOps.cpp Outdated Show resolved Hide resolved
@rwy7 rwy7 force-pushed the classes branch 2 times, most recently from 6657c28 to d195a71 Compare August 9, 2023 19:03
@rwy7 rwy7 merged commit 0346f20 into llvm:main Aug 10, 2023
5 checks passed
@rwy7 rwy7 deleted the classes branch August 10, 2023 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FIRRTL Involving the `firrtl` dialect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants