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][InferWidths] Handle foreign and prop types uniformly #5392

Merged
merged 1 commit into from
Jun 13, 2023

Conversation

youngar
Copy link
Member

@youngar youngar commented Jun 13, 2023

This changes InferWidths to use a common pattern to filter out foreign and property types. The getBaseType(FIRRTLType) function now takes a Type and returns null if it is not a FIRRTLType. declVars and all three constrain functions now use getBaseType and a null-check to skip over foreign and property types. This changes the behavior of maximumOfTypes and unifyTypes, but this is not a testable code path.

This changes InferWidths to use a common pattern to filter out foreign
and property types.  The `getBaseType(FIRRTLType)` function now takes a
`Type` and returns null if it is not a `FIRRTLType`.  `declVars` and all
three constrain functions now use `getBaseType` to skip over foreign and
property types.  This changes the behaviour of `maximumOfTypes` and
`unifyTypes`, but this is not a testable code path.
@youngar youngar added the FIRRTL Involving the `firrtl` dialect label Jun 13, 2023
@youngar youngar requested review from mikeurbach and rwy7 June 13, 2023 16:59
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!

inline FIRRTLBaseType getBaseType(FIRRTLType type) {
return TypeSwitch<FIRRTLType, FIRRTLBaseType>(type)
inline FIRRTLBaseType getBaseType(Type type) {
return TypeSwitch<Type, FIRRTLBaseType>(type)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this support a null type? Nice to be able to assume input is not null, but just curious re:how these might chain/be used.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think traditionally the dyn_cast_or_null function can handle a null argument, and we have getBaseTypeOrNull which I guess should be updated to null-check and then forward to this function. Also to make the name clearer we should rename this to follow the new upstream name for this dyn_cast_if_present, i.e. getBaseTypeIfPresent.

Copy link
Member Author

Choose a reason for hiding this comment

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

@youngar youngar merged commit fb7a78b into llvm:main Jun 13, 2023
@youngar youngar deleted the firrtl-prop-inferwidth branch June 13, 2023 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FIRRTL Involving the `firrtl` dialect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants