Skip to content

Conversation

@fabianschuiki
Copy link
Contributor

The FIRRTL Dedup pass currently also deduplicates classes. This is problematic, since classes are nominally typed, which requires their names to be fixed and doesn't allow us to swap one class for another even if they are structurally equivalent. Downstream tools expect the classes to retain their names, which is problematic if they look for class "Foo", but "Foo" happened to be structurally equivalent to "Bar" and therefore got replaced with "Bar".

To fix this, remove any class handling from the dedup pass. It can no longer remove classes or change an object's class. As a result, we can now treat class types as opaque again, since the class name symbols they contain will not be changed by the pass.

An alternative approach would have been to remove the nominal typing property from classes. If names are not stable, the compiler would be free to arbitrarily deduplicate and rework classes. This would imply changes in downstream tools, and @mikeurbach pointed out that the intent behind FIRRTL's classes were for them to be nominally typed. This now puts the onus of not generating gratuitous class definitions on Chisel and other frontends that produce FIRRTL.

The FIRRTL Dedup pass currently also deduplicates classes. This is
problematic, since classes are nominally typed, which requires their
names to be fixed and doesn't allow us to swap one class for another
even if they are structurally equivalent. Downstream tools expect the
classes to retain their names, which is problematic if they look for
class "Foo", but "Foo" happened to be structurally equivalent to "Bar"
and therefore got replaced with "Bar".

To fix this, remove any class handling from the dedup pass. It can no
longer remove classes or change an object's class. As a result, we can
now treat class types as opaque again, since the class name symbols they
contain will not be changed by the pass.

An alternative approach would have been to remove the nominal typing
property from classes. If names are not stable, the compiler would be
free to arbitrarily deduplicate and rework classes. This would imply
changes in downstream tools, and @mikeurbach pointed out that the intent
behind FIRRTL's classes were for them to be nominally typed. This now
puts the onus of not generating gratuitous class definitions on Chisel
and other frontends that produce FIRRTL.
@fabianschuiki fabianschuiki added the FIRRTL Involving the `firrtl` dialect label Sep 25, 2025
@dtzSiFive
Copy link
Contributor

Thank you!

This was always known to be wrong re:nominal typing at root of design of classes but was done anyway at the cost of consistency and bugs throughout the stack (as you've found) with benefit of limping some other things along.
This was expedient to get us through early days but also distressing as it was just a matter of time before this bit us.

Thanks for fighting this and rooting up all this tech debt. Much appreciated!

@fabianschuiki
Copy link
Contributor Author

This does indeed break a few things in Chisel 😬. I'm looking at fixes for those.

@fabianschuiki
Copy link
Contributor Author

Closing this in favor of more gradually phasing out class dedup in subsequent PRs.

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.

3 participants