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] Introduce a helper ODS class for type declaration, NFC #5399

Merged
merged 2 commits into from
Jun 15, 2023

Conversation

uenoku
Copy link
Member

@uenoku uenoku commented Jun 14, 2023

This commit adds ODS class FIRRTLPrimitiveType which inherits FIRRTLDialectType to reduce boilerplate. At the same time, tweak summary to make it more consistent

This is a preparation commit for introducing type alias.

@uenoku uenoku changed the title [FIRRTL] Introduce a helper ODS class for type declaration [FIRRTL] Introduce a helper ODS class for type declaration, NFC Jun 14, 2023
This commit adds ODS class `FIRRTLPrimitiveType` which inherits
FIRRTLDialectType to reduce boilerplate. At the same type, tweak summary
to make it more consistent

This is a preparation commit for introducing type alias.
@uenoku
Copy link
Member Author

uenoku commented Jun 14, 2023

Not sure FIRRTLPrimitiveType is a best name so I'm open to a suggestion. I alternatively considered FIRRTLSimpleType or FIRRTLTypeDef or whatever but I felt they didn't fit well.

Copy link
Contributor

@dtzSiFive dtzSiFive left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the cleanup, summary touchups, and breaking this out!

Not sure what to best call these, primitive feels little wrong but 🤔 . Changes otherwise look good.

uenoku added a commit that referenced this pull request Jun 15, 2023
As mentioned in https://discourse.llvm.org/t/preferred-casting-style-going-forward/68443 and https://mlir.llvm.org/deprecation/, `x.dyn_cast<T>/x.cast<T>/x.isa<T>` are deprecated. This commit replaces cast member functions in FIRRTL dialect with `dyn_cast<T>(x)/cast<T>(x)/isa<T>(x)`. 

This will make it a lot easier to migrate into type alias aware type_cast. We also need to refactor tablegen files as well but it will be done in a follow-up to avoid the conflict with #5399
@uenoku
Copy link
Member Author

uenoku commented Jun 15, 2023

Changed to FIRRTLDialectTypeHelper which makes it explicit that the class is just a helper.

@uenoku uenoku merged commit 7c9a535 into main Jun 15, 2023
5 checks passed
@uenoku uenoku deleted the dev/uenoku/introduce-helper-class branch June 15, 2023 06: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