-
Notifications
You must be signed in to change notification settings - Fork 277
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
[OM] Extract interfaces for ClassLike ops and ClassFieldLike ops, NFC. #5595
Conversation
This adds interfaces around the existing ClassOp and ClassFieldOp. The interfaces are just abstracting over generated methods for the ClassOp. For the ClassFieldOp, the interface just has one method to get a type, which comes from the field's value. By adding these interfaces and using them for parsing, printing, verifying, etc., it will be simple to add new kinds of ClassLike operations with fields, that can share functionality with the original ClassOp. One immediate use case is a class declaration with only types in its fields, suitable for use to declare external classes.
InterfaceMethod<"Get the class-like body region", | ||
"mlir::Region &", "getBody", (ins)>, | ||
InterfaceMethod<"Get the class-like body block", | ||
"mlir::Block *", "getBodyBlockPointer", (ins), |
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 wanted to define this as getBodyBlock
, but the way ODS works wouldn't allow it. You can declare interface methods that are just implemented by ODS generated methods, but I couldn't see a way to declare an interface method that was implemented by an extraClassDeclaration on the op. If I move getBodyBlock
from an extraClassDeclaration to an interface method, anywhere you directly create a ClassOp, you will need to cast it to a ClassLike in order to call this helper, which is kind of inconvenient. So my solution was to keep the extraClassDeclaration for convenience, but also add a convenience method to the interface with a different name, which is useful for the generic functions that work with the interface. Curious if there is a better way, or a better name.
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.
Maybe I'm missing something but can't we just define getBodyBlock
to both ClassLike
and ClassOp
? Interface and class declaration should use different name space so I guess it compiles. firrtl::FModuleLike::getParameters()
and firrtl::FModuleOp::getParameters
are doing same thing.
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 think the one after methodBody
, defaultImplementation
is where you can put implementation that goes on the trait (=> accessible on the op)?
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.
Oh, I see, I was trying with methodBody
, let me try with defaultImplementation
, that would be great.
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.
It looks like I'm able to define getBodyBlock
on the interface with methodBody
, just like how getParameters
works for FModuleLike
and FModuleOp
. Thanks for the pointers.
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
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.
InterfaceMethod<"Get the class-like body region", | ||
"mlir::Region &", "getBody", (ins)>, | ||
InterfaceMethod<"Get the class-like body block", | ||
"mlir::Block *", "getBodyBlockPointer", (ins), |
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 think the one after methodBody
, defaultImplementation
is where you can put implementation that goes on the trait (=> accessible on the op)?
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.
Looks good.
This adds interfaces around the existing ClassOp and ClassFieldOp. The interfaces are just abstracting over generated methods for the ClassOp. For the ClassFieldOp, the interface just has one method to get a type, which comes from the field's value.
By adding these interfaces and using them for parsing, printing, verifying, etc., it will be simple to add new kinds of ClassLike operations with fields, that can share functionality with the original ClassOp. One immediate use case is a class declaration with only types in its fields, suitable for use to declare external classes.