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] Fix type cast to look through type alias for type interfaces #5642

Merged
merged 1 commit into from
Jul 21, 2023

Conversation

uenoku
Copy link
Member

@uenoku uenoku commented Jul 20, 2023

This fixes a bug that type_cast doesn't work well if a request type isTypeInterface.

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.

Seems like good step forward, thanks! Let's keep in mind (as discussed, correct me if I have this wrong) that aliases won't appear to implement the interfaces their alias-ee does when non-FIRRTL code checks, so common infra or upstream code (FieldIDTypeInterface would be great example, probably not unrelated to why alias type always implements it?) will do wrong thing unless knows to use our FIRRTL check.. is there some way to somewhat generically handle this?

Looks like we don't use type interfaces presently really beyond our FieldIDTypeInterface, but it'd be good to not have this lurking as a problem.

Anyway this gets our own interface checks working, which is good, thanks!

@uenoku
Copy link
Member Author

uenoku commented Jul 21, 2023

Let's keep in mind (as discussed, correct me if I have this wrong) that aliases won't appear to implement the interfaces their alias-ee does when non-FIRRTL code checks,

Yes, aliases doesn't inherit & implement interfaces of each alias-ee type. That being said currently BaseTypeAliasType implements FIeldIDInterface since BaseTypeAliasType is derived from a base type and base type is required to implement FieldIDInterface.

so common infra or upstream code (FieldIDTypeInterface would be great example, probably not unrelated to why alias type always implements it?) will do wrong thing unless knows to use our FIRRTL check.. is there some way to somewhat generically handle this?

No, currently not. Right now it's user responsibility to apply type_cast before passing types to utility (though currently base alias does implements FIeldIDInterface so there would be much less problem). To fix the issue entirely, I think we have to declare multiple type alias types which inherits primitive types and define type interfaces. I tried implementing that idea but there was still an issue with type interface and traits. It seems currently child type (alias type in our case) cannot automatically use interface implementation of interfaces so I gave up that implementation.

class TypeAliasType <string baseTypeName,
list<Trait> traits,
string storage = "AliasStorage<" # baseTypeName # "AliasType>">:
FIRRTLImplType<baseTypeName # "Alias", [TypeAliasInterface] # traits,
"::circt::firrtl::" # baseTypeName> {
let summary = "type alias for " # baseTypeName;
let parameters =
(ins "ArrayAttr":$names, TypeParameter<"::circt::firrtl::" # baseTypeName,
"An inner type">:$innerType);

// ----- Type definitions for Type Alias
foreach i = [UIntImpl, SIntImpl, ClockTypeImpl, ResetTypeImpl, AsyncResetTypeImpl, AnalogTypeImpl, // Int
BundleImpl, OpenBundleImpl, FVectorImpl, OpenVectorImpl, FEnumImpl, // Aggregates
RefImpl, ListImpl, MapImpl, // Non-hardware
/*BigIntImpl, StringImpl*/
] in {
def _#i#TypeAlias: TypeAliasType<i.typeName, []>;
}

@uenoku uenoku merged commit 4596c04 into main Jul 21, 2023
5 checks passed
@uenoku uenoku deleted the dev/uenoku/fix-type-cast branch April 11, 2024 16:44
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