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][Inliner][NFC] Cleanup with some scoped state structs. #5812

Merged
merged 1 commit into from
Aug 10, 2023

Conversation

dtzSiFive
Copy link
Contributor

Group many arguments, give this a bit of structure, automatically perform fixups.

Clarify use and scope for all the plumbed datastructures,
and perform cleanups at appropriate points automatically.

Inlining methods could probably be moved to these, but use them
as simple containers for now.
Copy link
Member

@seldridge seldridge left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to clean this up. The function arity was out of control here. 😓

The use of the desctructor to finalize everything is clever.

Comment on lines -589 to -595
/// Clone and rename an operation. Return the new operation.
Operation *cloneAndRename(StringRef prefix, OpBuilder &b, IRMapping &mapper,
Operation &op,
const DenseMap<Attribute, Attribute> &symbolRenames,
const DenseSet<Attribute> &localSymbols,
ModuleNamespace &moduleNamespace,
InnerRefToNewNameMap &relocatedInnerSyms);
Copy link
Member

Choose a reason for hiding this comment

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

Seeing this 8-parameter (!) function get removed sure looks good.

@dtzSiFive
Copy link
Contributor Author

Thanks for the review! 👍

@dtzSiFive dtzSiFive merged commit e138434 into llvm:main Aug 10, 2023
5 checks passed
@dtzSiFive dtzSiFive deleted the feature/inliner-state-structs branch August 10, 2023 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants