[HW] Enhance FlattenModules pass with configurable inlining heuristics #9224
Conversation
prithayan
changed the title
prithayan
force-pushed
the
dev/pbarua/hwinliner
branch
2 times, most recently
from
f3e4ccf to
c921ca7
Compare
seldridge
left a comment
seldridge
left a comment
There was a problem hiding this comment.
LGTM.
I didn't review the inner sym / hierarchical path update logic in much detail, though. I realize this was probably the most important thing to review. However, the tests are doing the right thing!
| InnerSymbolNamespace *ns, HWModuleOp parentModule, | ||
| HWModuleOp sourceModule, | ||
| DenseMap<StringAttr, StringAttr> *symMapping, | ||
| mlir::AttrTypeReplacer *replacer, HierPathTable *pathsTable, | ||
| hw::InnerRefAttr instanceRef) | ||
| : InlinerInterface(context), prefix(prefix), ns(ns), | ||
| parentModule(parentModule), sourceModule(sourceModule), | ||
| symMapping(symMapping), replacer(replacer), pathsTable(pathsTable), | ||
| instanceRef(instanceRef) {} |
There was a problem hiding this comment.
This constructor may be unnecessary if you use struct initialization. E.g., PrefixingInliner{a, b, c}. I don't know if there's any utility in adding a one-to-one constructor here as opposed to the {} one.
| // Test that public modules are not inlined | ||
| // CHECK-LABEL: hw.module @DontInlinePublic | ||
| hw.module @DontInlinePublic(in %x: i4, out y: i4) { | ||
| // CHECK-NEXT: hw.instance "pub" @PublicModule | ||
| %0 = hw.instance "pub" @PublicModule(a: %x: i4) -> (b: i4) | ||
| hw.output %0 : i4 | ||
| } | ||
| // CHECK: hw.module @PublicModule | ||
| hw.module @PublicModule(in %a: i4, out b: i4) { | ||
| %0 = comb.add %a, %a : i4 | ||
| hw.output %0 : i4 | ||
| } |
There was a problem hiding this comment.
Aside to think about: we could inline these still, just we can't delete them.
There was a problem hiding this comment.
Right, based on the options, if we inline it still keep the public module.
test/Dialect/HW/hw-inliner.mlir
Outdated
| // CHECK-NOT: hw.instance | ||
| // CHECK-NEXT: %[[V0:.+]] = comb.add %x, %x | ||
| // CHECK-NEXT: %[[W0:.+]] = hw.wire %[[V0]] sym @[[SYM:wire_op]] | ||
| // CHECK-NEXT: sv.verbatim "ref to {{[{][{]}}0{{[}][}]}}" {symbols = [#hw.innerNameRef<@InlineWithInnerRef::@[[SYM]]>]} |
There was a problem hiding this comment.
Mega-nit: There is only this one verbatim and the test could avoid the {{{{{}}}}}} shenanigans with just:
CHECK-NEXT: sv.verbatim {{.*}} {symbols = ...}
Or:
CHECK-NEXT: sv.verbatim
CHECK-SAME: {symbols = [#hw.innerNameRef<@InlineWithInnerRef::@[[SYM]]>]}
Ditto throughout.
Only evaluate the smallThreshold when inlineSmall is enabled. This avoids unnecessary computation when the small module heuristic is disabled and makes the code more efficient and clearer in intent.
Rename all pass options to follow the convention of prefixing with the dialect name: - inline-all -> hw-inline-all - inline-empty -> hw-inline-empty - inline-no-outputs -> hw-inline-no-outputs - inline-single-use -> hw-inline-single-use - inline-small -> hw-inline-small - small-threshold -> hw-small-threshold - inline-with-state -> hw-inline-with-state This makes the options consistent with other HW dialect passes and improves clarity when using multiple passes together.
prithayan
force-pushed
the
dev/pbarua/hwinliner
branch
from
c921ca7 to
ae0b612
Compare
|
@seldridge thanks a lot for the detailed review, and sorry for the long delay in addressing them. I'll merge this once the CI passes. |
This PR enhances the existing
--hw-flatten-modulespass with configurable heuristics, hierarchical path support, and module NLA protection for fine-grained control over module inlining.The FlattenModules pass now supports:
InnerSymbolNamespaceto avoid conflictsInlining Heuristics (7 new options)
This PR also adds new lit tests for testing these enhancements.