-
Notifications
You must be signed in to change notification settings - Fork 404
Add sv.module.verbatim op and use for firrtl.extmodule with BlackBoxInlineAnno/BlackBoxPathAnno #9131
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
Add sv.module.verbatim op and use for firrtl.extmodule with BlackBoxInlineAnno/BlackBoxPathAnno #9131
Conversation
d883171 to
98810c1
Compare
98810c1 to
bc5fb1d
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 implementation direction makes sense, could you include tests for ExportVerilog, lower-to-hw, and sv dialect? It's a bit unclear to me how additional_files are used.
Yep, will do.
Sorry, I meant to talk about that in the PR description. I've added it but I'll paste it here too:
|
I added some tests for LowerToHW and ExportVerilog. I wasn't sure what SV dialect tests should exercise that isn't covered by the LowerToHW, ExportVerilog, or BlackBox tests. If you have something in mind please let me know. |
uenoku
left a comment
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.
Generally looks great! Could you add round-trip test for SV dialect?
uenoku
left a comment
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, mostly style comments but please fix DenseMap iteration (non-determinism) and remove getnearestSymbolTable in verifySymbolUses.
#9131 was recently merged but does not correctly handle `firrtl.extmodule`s with inout ports. This implements the same logic used in `HWModuleOp` in the lowering for `sv.verbatim.module`.
…x extmodules (#9227) A second attempt of #9131. Changes from the first PR were made in order to support parametrized black boxes. Because parameters can affect the port interface of a module, we need two ops: 1. sv.verbatim.source to represent the shared verbatim definition of a module 2. sv.verbatim.module to represent a particular parametrization of that module that can be instantiated The pipeline is largely the same: - BlackBoxInlineAnno and BlackBoxPathAnno are converted to VerbatimBlackBoxAnno by BlackBoxReader for consumption by LowerToHW. - LowerToHW will lower firrtl.extmodules with a VerbatimBlackBoxAnno to a sv.verbatim.source (only one per file) and a sv.verbatim.module.
This PR adds an
sv.verbatim.moduleop and lowers inline black boxes (firrtl.extmodulewithBlackBoxInlineAnnoorBlackBoxPathAnno) to the new op.Emitted verilog is semantically the same (there can be a difference in emission order)--so, this is mostly an IR-only change.
Before:
firrtl.extmodulewithBlackBoxInlineAnnoorBlackBoxPathAnno, createemit.fileandemit.verbatimoperations for the inline contentfirrtl.extmodule, creates ahw.module.externoperationAfter:
firrtl.extmodulewith verbatim annotations:BlackBoxInlineAnnoandBlackBoxPathAnnoto calculate the verbatim content and determine the output file pathBlackBoxInlineAnno/BlackBoxPathAnnoannotationsfirrtl.extmodulewith the new verbatim annotation:sv.verbatim.moduleoperationadditional_filesattribute of the existingsv.verbatim.moduleoperationhw.module.externfor modules with verbatim contentBlackBoxReader
It is a bit strange to partially lower the
BlackBoxInlineAnnoBlackBoxPathAnno. The intention here was to preserve the fragile logic around calculating output directories for black boxes.Eventually, I think we can remove BlackBoxReader entirely and rely on AssignOutputDirs for this. Then, by the time we get to LowerToHW, we don't need to special case any of this stuff for black boxes.
Supporting multiple files
firtool currently accepts multiple instances of
BlackBoxInlineAnnoandBlackBoxPathAnnoannotations. This is often used to include additional SV libraries, C files, or headers. In the future, we should disallow this and ensure that one module corresponds to one SV file. Before we do this however, we need to provide another mechanism for specifying external dependencies.The
addition_filesattribute is a way to support the existing behavior in the meantime. Once we have addressed this tech-debt, we can just remove that attribute.