[FIRRTL] Various small reduction pattern tweaks #8984
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
fabianschuiki
commented
Sep 18, 2025
fabianschuiki
commented
- Make the eager inliner not remove instances that are part of an NLA.
- Make the annotation remover not remove NLAs that have remaining uses after some annotations referencing it were removed.
- Make the constantifier not match on existing constants, and not replace operations that have the "inner_sym" attribute.
fabianschuiki
added
FIRRTL
seldridge
approved these changes
Sep 18, 2025
Comment on lines
-959
to
+969
| struct EagerInliner : public OpReduction<firrtl::InstanceOp> { | ||
| struct EagerInliner : public OpReduction<InstanceOp> { |
Comment on lines
+1044
to
+1068
| /// Check if inlining the referenced module into the parent module would cause | ||
| /// inner symbol collisions. | ||
| bool hasInnerSymbolCollisions(FModuleOp referencedModule, | ||
| FModuleOp parentModule) { | ||
| // Get the inner symbol tables for both modules | ||
| auto &targetTable = innerSymTables->getInnerSymbolTable(referencedModule); | ||
| auto &parentTable = innerSymTables->getInnerSymbolTable(parentModule); | ||
|
|
||
| // Check if any inner symbol name in the target module already exists | ||
| // in the parent module. Return failure() if a collision is found to stop | ||
| // the walk early. | ||
| LogicalResult walkResult = targetTable.walkSymbols( | ||
| [&](StringAttr name, | ||
| const hw::InnerSymTarget &target) -> LogicalResult { | ||
| // Check if this symbol name exists in the parent module | ||
| if (parentTable.lookup(name)) { | ||
| // Collision found, return failure to stop the walk | ||
| return failure(); | ||
| } | ||
| return success(); | ||
| }); | ||
|
|
||
| // If the walk failed, it means we found a collision | ||
| return failed(walkResult); | ||
| } |
Member
There was a problem hiding this comment.
I kind of like this style of eagerly exit if the reduction pattern isn't trivially applicable. This is in contrast with trying to do a bunch of work to clean this up. I think this makes sense because the inner symbols will get dropped with other reductions.
Contributor
Author
There was a problem hiding this comment.
Totally agreed! It's also very annoying if the pattern applies too eagerly and then leads to invalid IR in a lot of cases. That just sends the reducer into busy work rabbit holes that don't really improve the reduction. 😞
fabianschuiki
force-pushed
the
fschuiki/even-more-vibe-reductions
branch
from
1719f3f to
b509ec6
Compare
fabianschuiki
force-pushed
the
fschuiki/dedup-class-types
branch
2 times, most recently
from
7f021be to
59096e8
Compare
fabianschuiki
force-pushed
the
fschuiki/even-more-vibe-reductions
branch
from
b509ec6 to
2fec994
Compare
- Make the eager inliner not remove instances that are part of an NLA. - Make the annotation remover not remove NLAs that have remaining uses after some annotations referencing it were removed. - Make the constantifier not match on existing constants, and not replace operations that have the "inner_sym" attribute.
fabianschuiki
force-pushed
the
fschuiki/even-more-vibe-reductions
branch
from
2fec994 to
ac4f5c3
Compare
Avoid pruning unused operations that have inner symbols.
Add a helper struct that collects all inner references under a root op into a set. This is pretty pessimistic and specific to the reducer. Make use of this in the `NodeSymbolRemover` FIRRTL reduction to only remove symbols from ops if there are no references to them.
Do not externalize modules if they are involved in inner refs. Use the new `InnerRefUses` helper for this, with a few extensions.
Since most reduction patterns need information about whether an op is referenced by its symbol name or inner symbol name, also keep track of regular symbol references in `InnerSymbolUses`. This will allow us to get rid of many uses of `SymbolUserMap`.
Add a reduction pattern for the Emit dialect that simply deletes all Emit ops with high priority, since these often do not affect whether a circt-reduce test case is interesting.
Use the new `InnerSymbolUses` helper to make the OperationPruner and UnusedSymbolPruner more accurate by not deleting operations that still have references to them, which is very likely to break the IR.
Run canonicalization before CSE, since CSE will otherwise do a limited form of canonicalization that keeps finding minimal reductions, sending the reducer into a rabbit hole of minimal incremental improvements.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.