[IMDCE] Remove dead hierpath and annotations in IMDCE #5226
Conversation
7b891c1 to
242c536
Compare
I tesed large cores, but it increases 1% of entire execution time, so I'd consider acceptable. |
242c536 to
fddfd88
Compare
As it will require more discussion, this comit should not introduce the concept of discrdable annotation. For now, just add `type` field to OMIRTracker and hard code the logic into IMDCE so that we don't have to worry about other than IMDCE
|
I updated the PR not to introduce the notion of "discardable/weak/value tap annotation" which requires more discussion (#5240). Instead, I modified OMIRTracker annotations to contain tracker types and IMDCE pass will determine whether it's valid to drop the tracker annotations. Eventually the logic should be replaced with "circt.discardbale" whatever. |
|
Computing IRN should be fairly fast, esp if there are many cores available, but good question. We presently compute it as part of verification between passes (that don't preserve everything), although there's no re-use from the verifier-computed version. |
| @@ -117,7 +117,7 @@ def RemoveUnusedPorts : Pass<"firrtl-remove-unused-ports", "firrtl::CircuitOp"> | |||
| ]; | |||
| } | |||
|
|
|||
| def IMDeadCodeElim : Pass<"firrtl-imdeadcodeelim", "firrtl::CircuitOp"> { | |||
There was a problem hiding this comment.
I think hierpath are now in the circuit namespace.
There was a problem hiding this comment.
I followed the implementation of InnerSymbolDCE that checks all uses of inner symbols not only within circuit op.
circt/lib/Dialect/FIRRTL/Transforms/InnerSymbolDCE.cpp
Lines 107 to 109 in a6c8cb3
If it's assumed that there is no user of inner symbol outer circuit op, it makes sense to me to make these passes circuit op.
| if (!anno.isClass(omirTrackerAnnoClass)) | ||
| return false; | ||
|
|
||
| auto tpe = anno.getMember<StringAttr>("type"); |
There was a problem hiding this comment.
This is NOT acceptable. No pass should be interpreting annotations except passes explicitly dealing with those annotations.
There was a problem hiding this comment.
Yes, I intended this to be very short term implementation until #5240 is merged (#5226 (comment)). Originally I implemented by adding "circt.discardable" attribute to annotation to indicate the annotation has weak reference, but I separated the PR into #5240.
This PR adds functionality to find and remove dead hierpaths and weak referencing annotations in IMDCE. Even though it's not explicitly defined yet, some annotations (for example OMIRTracker) are expected to be removed when annotation targets are dead. In many cases OMIRTracker is represented as non-local annotations associated with hierpaths which are users of inner symbols. So this PR imports some of InnerSymbolDCE logic into IMDCE and aggressively removes inner symbols.
This PR adds functionality to find and remove dead hierpaths and weak referencing annotations in IMDCE. Even though it's not explicitly defined yet, some annotations (for example OMIRTracker) are expected to be removed when annotation targets are dead. In many cases OMIRTracker is represented as non-local annotations associated with hierpaths which are users of inner symbols. So this PR imports some of InnerSymbolDCE logic into IMDCE and aggressively removes inner symbols.
This PR adds functionality to find and remove dead hierpath in IMDCE. There are discardable annotations (e.g. OMTracker) which use hierpath and prevent IMDCE from deleting instances with inner symbols. This PR imports some of InnerSymbolDCE logic into IMDCE.