[FIRRTL] Update more symbol-sensitive ops after dedup #9016
Conversation
The Dedup pass currently handles module instances and class objects explictly in order to update the referenced module/class after deduplication. This does not consider types and attributes on any other operations, such as wires with a `!firrtl.class<...>` type. These also need to be updated. Instead of adding more special cases, make use of the attribute/type walker and replacer infrastructure of MLIR that now works properly with FIRRTL types. When hashing a module, take note of all operations that have a symbol name nested somewhere in their attributes of types, and store them in a `symbolSensitiveOps` list as part of the module info. Once deduplication is done, visit those ops and replace all occurrences of outdated symbols with their post-dedup replacement. This can get rid of the special handling of ClassOp and ObjectOp, and also covers WireOp and attributes/types on any other op. This came up as a dedup failure in a real-world design.
There was a problem hiding this comment.
The mutation of hasSeenSymbol is a bit difficult to follow. I was trying to think if there was a better way to handle this like adding a return value to the update methods. This is non-blocking and only for consideration.
| /// This fixes up ClassLikes with ClassType ports, when the classes have | ||
| /// deduped. For each ClassType port, if the object reference being assigned is | ||
| /// a different type, update the port type. Returns true if the ClassOp was | ||
| /// updated and the associated ObjectOps should be updated. |
There was a problem hiding this comment.
Getting rid of this is great!
|
@mikeurbach pointed out that classes were supposed to be nominally typed, and we have added them to FIRRTL's dedup as a hack at some point. So instead of requiring class/object op fixups, these ops should never participate in dedup in the first place. I'm going to give that a spin in parallel. |
|
I think I'm going to land this, since it fixes bugs in the current implementation. We'll probably want an option on firtool to disable class deduplication entirely, such that various flows can migrate over gracefully. |
The Dedup pass currently handles module instances and class objects explictly in order to update the referenced module/class after deduplication. This does not consider types and attributes on any other operations, such as wires with a
!firrtl.class<...>type. These also need to be updated.Instead of adding more special cases, make use of the attribute/type walker and replacer infrastructure of MLIR that now works properly with FIRRTL types. When hashing a module, take note of all operations that have a symbol name nested somewhere in their attributes of types, and store them in a
symbolSensitiveOpslist as part of the module info. Once deduplication is done, visit those ops and replace all occurrences of outdated symbols with their post-dedup replacement. This can get rid of the special handling of ClassOp and ObjectOp, and also covers WireOp and attributes/types on any other op.This came up as a dedup failure in a real-world design.