-
Notifications
You must be signed in to change notification settings - Fork 292
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 Inline Formal Test ops #7374
Conversation
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.
Awesome! Parser and Op definitions generally look great. It would be also necessary to modify firrtl-spec. Regarding FIRRTL dialect representation there are several things to consider since FormalOp is a first operation which is not FModuleOp but could contain mots of FIRRTL ops:
- InstanceGraph -- FormalOp has an instance op so we would need to create an edge from FormalOp to the referenced module. Probably need to implement ModuleOpInterface.
- FModuleOp pass -- LowerIntrinsics, ExpandWhen etc. are currently FModuleOp pass so we need to find a way to run them on FormalOp
Co-authored-by: Hideto Ueno <uenoku.tokotoko@gmail.com>
@uenoku commented:
Feel free to reach out to me if you want to offload the FIRRTL spec work to me. |
ed0e993
to
a5b6f2a
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.
The new design looks fantastic! Thank you for iterating on this! The implementation looks great to me except for few nits regarding ReferableDeclOp.
"firrtl::FModuleOp", "firrtl::LayerBlockOp", | ||
"firrtl::WhenOp", "firrtl::MatchOp", "sv::IfDefOp"]>]> {} | ||
|
||
def FormalOp : FIRRTLOp<"formal", [ | ||
HasParent<"firrtl::CircuitOp">, | ||
DeclareOpInterfaceMethods<SymbolUserOpInterface>, |
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 it needs Symbol as well.
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, can you check whether we should first change FIRRTL-spec or merge the PR?
Firrtl spec is updated, so I guess we're good to go! |
The goal of this PR is to add a new set of inline formal test ops to FIRRTL:
These new ops will then be lowered to the existing
verif.formal
op in a future PR.