[SVExtractTestCode] Fix a bug that instance inlining doesn't update inner symbols and clone sv.bind #5679
Conversation
There was a problem hiding this comment.
This looks really good to me, thanks for fixing this up!
I know there is some ongoing improvement around the interfaces/utilities for inner symbols. Not sure yet if those are ready to use yet, or if this is the way for now. @dtzSiFive what do you think?
| emitWarning(oldMod.getLoc()) << "module " << oldMod.getModuleName() | ||
| << " is an input only module but cannot " | ||
| "be inlined because a signal " | ||
| << port.name << " is referred by names"; |
There was a problem hiding this comment.
This error message is great, but I got confused at the very end with names (plural). I think it makes sense to just say referred by name, was that the intent?
| emitWarning(op.getLoc()) | ||
| << "module " << oldMod.getModuleName() | ||
| << " is an input only module but cannot be inlined because signals " | ||
| "are referred by names"; |
There was a problem hiding this comment.
Same comment about names plural. Also, could you print the innerSym in the error message here like is done for ports?
| // declaration with an inner symbol referred by non-bind ops (e.g. hierpath). | ||
| for (auto port : oldMod.getAllPorts()) { | ||
| if (port.sym) { | ||
| if (!port.sym.getSymName()) |
There was a problem hiding this comment.
| if (!port.sym.getSymName()) | |
| if (!port.sym.empty()) |
There was a problem hiding this comment.
Actually, maybe leave as-is since none of this works for per-field symbols presently anyway.
There was a problem hiding this comment.
Yep, I added this logic to reject per-field symbols.
| } | ||
| } | ||
| for (auto &op : *oldMod.getBodyBlock()) { | ||
| if (auto innerSym = op.getAttrOfType<StringAttr>("inner_sym")) |
There was a problem hiding this comment.
-> InnerSymbolOpInterface once HW uses any of the inner symbol bits. This is probably okay as-is.
| if (auto innerSym = op.getAttrOfType<StringAttr>("inner_sym")) { | ||
| auto newName = b.getStringAttr(nameSpace.newName(innerSym.getValue())); | ||
| auto result = symMapping.insert({innerSym, newName}); | ||
| assert(result.second); |
There was a problem hiding this comment.
Yeah, was wondering if the rename-multiple-times bugs/concerns we've encountered in ModuleInliner would apply here. Is that possible, do you know?
There was a problem hiding this comment.
I think this is fine unless the original module doesn't contain non-unique inner symbols. Though there is no verification for IR so probably we need better messages.
| // contain bound instances so create a set of inner refs used by non bind op | ||
| // in order to allow bind ops. | ||
| DenseSet<hw::InnerRefAttr> innerRefUsedByNonBindOp; | ||
| top.walk([&](hw::InnerRefUserOpInterface op) { |
There was a problem hiding this comment.
I don't think the majority of the current inner-ref users in HW/related dialects implement this interface "yet".
Notably BindInterfaceOp, FWIW, but also in OM and such from a quick grep. Might need to just walk everywhere for now, but probably this works "for now" as-is?
There was a problem hiding this comment.
Yep, good point. I'll just remove InnerRefUserOpInterface here.
|
Nit:
SVExtractTestCode, FWIW |
| static void | ||
| inlineInputOnly(hw::HWModuleOp oldMod, hw::InstanceGraph &instanceGraph, | ||
| BindTable &bindTable, SmallPtrSetImpl<Operation *> &opsToErase, | ||
| DenseSet<hw::InnerRefAttr> &innerRefUsedByNonBindOp) { |
There was a problem hiding this comment.
It seems DenseSetImpl cannot be used for that purpose unlike SmallVectorImpl
| emitWarning(oldMod.getLoc()) << "module " << oldMod.getModuleName() | ||
| << " is an input only module but cannot " | ||
| "be inlined because a signal " | ||
| << port.name << " is referred by names"; |
There was a problem hiding this comment.
Nit: oldMod.emitWarning() should be fine as any production use should be running with -mlir-print-op-on-diagnostic=false.
Same comment elsewhere.
| attr.getValue().walk([&](hw::InnerRefAttr attr) { | ||
| innerRefUsedByNonBindOp.insert(attr); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Can this be called multiple times on the same module (causing performance issues)?
There was a problem hiding this comment.
No, it called just once for each module. It's still a bit concerning to walk the entire circuit (and its attributes) but I think there is no other way around.
uenoku
force-pushed
the
dev/uenoku/etc-fix-inlining
branch
from
bedcac3
to
f59b2ac
Compare
…ner symbols and clone sv.bind Close #5665. This commit fixes a bug that bound instances are accidentally erased. When inlining input only modules it's necessary to clone bind statements for bound instances in the module. Previously single bind op is shared by multiple instances, as a result ExportVerilog only emits a bind statement for one of them.
uenoku
force-pushed
the
dev/uenoku/etc-fix-inlining
branch
from
f59b2ac
to
ccf7168
Compare
uenoku
changed the title
uenoku
changed the title
…nner symbols and clone sv.bind (#5679) Close #5665. This commit fixes a bug that bound instances are accidentally erased. When inlining input only modules it's necessary to clone bind statements for bound instances in the module. Previously single bind op is shared by multiple instances, as a result ExportVerilog only emits a bind statement for one of them.
…nner symbols and clone sv.bind (#5679) Close #5665. This commit fixes a bug that bound instances are accidentally erased. When inlining input only modules it's necessary to clone bind statements for bound instances in the module. Previously single bind op is shared by multiple instances, as a result ExportVerilog only emits a bind statement for one of them.
Close #5665. This commit fixes a bug that bound instances are accidentally erased. When inlining input only modules it's necessary to clone bind statements for bound instances in the module. Previously single bind op is shared by multiple instances, as a result ExportVerilog only emits a bind statement for one of them.