[FIRRTL] Handle reference ports when Classes dedup. #6770
Conversation
In #6582, initial support for classes and objects was added in Dedup. However, since classes and objects are new constructs, not every possibility was handled in the initial implementation. This specifically handles the case where references to objects are passed through class ports, and the class type of such objects has changed because their classes deduped. In the final fixup pass through the instance graph, if we find a class, we check if it has any reference ports that need to be updated, and if so update them. When this happens, we also update an objects of the class to reflect the newly updated class type. Fixes #6603.
mikeurbach
requested review from
dtzSiFive,
darthscsi and
seldridge
as code owners
|
@uenoku since you pointed this out on the initial PR, do you mind taking a look? Or maybe @youngar as the resident dedup expert. Since the whole notion of references to instances is a new concept, I wasn't totally sure where to implement the change. It seemed appropriate in the final fixupAllModules walk of the instance graph. |
There was a problem hiding this comment.
Looking great! I think for class op it correctly deduplicates ports. I'm curious if we need something similar for operations in module ops, especially wire ops? For example I expect the test still successfully dedups with the following patch, but it currently fails. Probably it's outside of the scope of the PR so it would be fine to fix in a follow-up.
diff --git a/test/firtool/classes-dedupe.fir b/test/firtool/classes-dedupe.fir
index 5b0bbe78a..b7fef164c 100644
--- a/test/firtool/classes-dedupe.fir
+++ b/test/firtool/classes-dedupe.fir
@@ -33,7 +33,9 @@ circuit Test : %[[
output om_out : AnyRef
object om of OM_1
- propassign om_out, om
+ wire tmp: Inst<OM_1>
+ propassign tmp, om
+ propassign om_out, tmp
; CHECK: hw.instance "fetch_1" sym [[SYM2]]
inst fetch_1 of Fetch_1
@@ -49,7 +51,9 @@ circuit Test : %[[
output om_out : AnyRef
object om of OM_2
- propassign om_out, om
+ wire tmp: Inst<OM_2>
+ propassign tmp, om
+ propassign om_out, tmp
inst fetch_1 of Fetch_2
inst fetch_2 of Fetch_2
|
Yeah, that is a good point, this isn't handled in this PR. I think we can file a separate issue to handle that. It's legal IR, but we do have a very early pass that removes wires holding properties, since properties require SSA anyway, so by the time we get to dedup we shouldn't see IR like this in practice. It would be nice if this invariant was enforced, so later passes could assume there are no property wires. |
In #6582, initial support for classes and objects was added in Dedup. However, since classes and objects are new constructs, not every possibility was handled in the initial implementation.
This specifically handles the case where references to objects are passed through class ports, and the class type of such objects has changed because their classes deduped.
In the final fixup pass through the instance graph, if we find a class, we check if it has any reference ports that need to be updated, and if so update them. When this happens, we also update an objects of the class to reflect the newly updated class type.
Fixes #6603.