-
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 4.0.0 public modules #6448
[firrtl] Add 4.0.0 public modules #6448
Conversation
7b5be71
to
53ef63f
Compare
I integrated the changes from #6179 related to top module scalarization. I replaced this with public module scalarization. All this should be removed with changes on the FIRRTL spec side to include the ABI in the public module definition. |
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 LGTM, but please fix the behavior re:public on non-modules (see review comment).
As an aside, thoughts on when to advance the FIRVersion constants for this future release?
(related TODO is to add Emitter support)
auto intModuleOp = | ||
builder.create<FIntModuleOp>(info.getLoc(), name, portList, intName, | ||
annotations, parameters, internalPaths); | ||
SymbolTable::setSymbolVisibility(intModuleOp, |
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 we should have these private by default? Not for this PR.
3a28066
to
dfb047f
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.
LGTM!
Does this fully cover #6176?
It does, yes. Proper attribution via |
Add support for FIRRTL 4.0.0 feature, "public modules". This does not provide complete FIRRTL 4.0.0 support as this requires removing circuit names. This will be done in a later commit. Co-authored-by: Will Dietz <will.dietz@sifive.com> Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Remove the -scalarize-top-module option to firtool (and the parser). Replace this with -scalarize-public-modules. This is done in preparation for FIRRTL 4.0.0. This is also largely unnecessary once FIRRTL 4.0.0 figures out how to attach the ABI to each public module. Co-authored-by: Will Dietz <will.dietz@sifive.com> Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
dfb047f
to
80b3edb
Compare
Add support for FIRRTL 4.0.0 feature, "public modules". This does not provide complete FIRRTL 4.0.0 support as this requires removing circuit names. This will be done in a later commit.
This should get unified with: #6176
Fixes #6176.